| Message ID | 1255069742-15724-16-git-send-email-yamahata@valinux.co.jp |
|---|---|
| State | Under Review |
| Headers | show |
On Fri, Oct 09, 2009 at 03:28:48PM +0900, Isaku Yamahata wrote: > This patch is preliminary for 64bit bar. > For 64bit bar support, change pcibus_t which represents > pci bus addr/size from uint32_t to uint64_t. > And also change FMT_pcibus for printf. > > In pci_update_mapping() checks 32bit overflow. > So the check must be updated too. > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> That's all fine, but if you look at users implementing map io, they do: cpu_register_physical_memory() on the address they are given. And if target_phys_addr_t is 32 bit, this will silently truncate the address. So I would like to understand how this will all work on 32 bit systems. > --- > hw/pci.c | 9 ++++++++- > hw/pci.h | 4 ++-- > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index c0ae66a..e7f8fb4 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -524,7 +524,14 @@ static void pci_update_mappings(PCIDevice *d) > mappings, we handle specific values as invalid > mappings. */ > if (last_addr <= new_addr || new_addr == 0 || > - last_addr == PCI_BAR_UNMAPPED) { > + last_addr == PCI_BAR_UNMAPPED || > + > + /* Now pcibus_t is 64bit. > + * Check if 32 bit BAR wrap around explicitly. > + * Without this, PC ide doesn't work well. > + * TODO: remove this work around. > + */ > + last_addr >= UINT32_MAX) { > new_addr = PCI_BAR_UNMAPPED; > } > } else { > diff --git a/hw/pci.h b/hw/pci.h > index 8a187c2..8fbd45e 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -71,8 +71,8 @@ extern target_phys_addr_t pci_mem_base; > #define PCI_DEVICE_ID_VIRTIO_BALLOON 0x1002 > #define PCI_DEVICE_ID_VIRTIO_CONSOLE 0x1003 > > -typedef uint32_t pcibus_t; > -#define FMT_PCIBUS PRIx32 > +typedef uint64_t pcibus_t; > +#define FMT_PCIBUS PRIx64 > > typedef void PCIConfigWriteFunc(PCIDevice *pci_dev, > uint32_t address, uint32_t data, int len); > -- > 1.6.0.2
On Sun, Oct 11, 2009 at 12:43:12PM +0200, Michael S. Tsirkin wrote: > On Fri, Oct 09, 2009 at 03:28:48PM +0900, Isaku Yamahata wrote: > > This patch is preliminary for 64bit bar. > > For 64bit bar support, change pcibus_t which represents > > pci bus addr/size from uint32_t to uint64_t. > > And also change FMT_pcibus for printf. > > > > In pci_update_mapping() checks 32bit overflow. > > So the check must be updated too. > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > That's all fine, but if you look at users implementing > map io, they do: cpu_register_physical_memory() > on the address they are given. And if target_phys_addr_t is 32 bit, > this will silently truncate the address. > > So I would like to understand how this will all > work on 32 bit systems. The case is . BAR is memory 64bit and . target_phys_addr_t is 32bit and . bar is set to >4G. Hmm, the case isn't checked. It would be checked by - last_addr <= new_addr + (target_phys_addr_t)last_addr <= new_addr I'll fix it with comments added. Nice catch. > > > --- > > hw/pci.c | 9 ++++++++- > > hw/pci.h | 4 ++-- > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/hw/pci.c b/hw/pci.c > > index c0ae66a..e7f8fb4 100644 > > --- a/hw/pci.c > > +++ b/hw/pci.c > > @@ -524,7 +524,14 @@ static void pci_update_mappings(PCIDevice *d) > > mappings, we handle specific values as invalid > > mappings. */ > > if (last_addr <= new_addr || new_addr == 0 || > > - last_addr == PCI_BAR_UNMAPPED) { > > + last_addr == PCI_BAR_UNMAPPED || > > + > > + /* Now pcibus_t is 64bit. > > + * Check if 32 bit BAR wrap around explicitly. > > + * Without this, PC ide doesn't work well. > > + * TODO: remove this work around. > > + */ > > + last_addr >= UINT32_MAX) { > > new_addr = PCI_BAR_UNMAPPED; > > } > > } else { > > diff --git a/hw/pci.h b/hw/pci.h > > index 8a187c2..8fbd45e 100644 > > --- a/hw/pci.h > > +++ b/hw/pci.h > > @@ -71,8 +71,8 @@ extern target_phys_addr_t pci_mem_base; > > #define PCI_DEVICE_ID_VIRTIO_BALLOON 0x1002 > > #define PCI_DEVICE_ID_VIRTIO_CONSOLE 0x1003 > > > > -typedef uint32_t pcibus_t; > > -#define FMT_PCIBUS PRIx32 > > +typedef uint64_t pcibus_t; > > +#define FMT_PCIBUS PRIx64 > > > > typedef void PCIConfigWriteFunc(PCIDevice *pci_dev, > > uint32_t address, uint32_t data, int len); >
On Tue, Oct 13, 2009 at 10:31:33PM +0900, Isaku Yamahata wrote: > On Sun, Oct 11, 2009 at 12:43:12PM +0200, Michael S. Tsirkin wrote: > > On Fri, Oct 09, 2009 at 03:28:48PM +0900, Isaku Yamahata wrote: > > > This patch is preliminary for 64bit bar. > > > For 64bit bar support, change pcibus_t which represents > > > pci bus addr/size from uint32_t to uint64_t. > > > And also change FMT_pcibus for printf. > > > > > > In pci_update_mapping() checks 32bit overflow. > > > So the check must be updated too. > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > > That's all fine, but if you look at users implementing > > map io, they do: cpu_register_physical_memory() > > on the address they are given. And if target_phys_addr_t is 32 bit, > > this will silently truncate the address. > > > > So I would like to understand how this will all > > work on 32 bit systems. > > The case is > . BAR is memory 64bit and > . target_phys_addr_t is 32bit and > . bar is set to >4G. > Hmm, the case isn't checked. > > It would be checked by > - last_addr <= new_addr > + (target_phys_addr_t)last_addr <= new_addr That's pretty tricky. Can we just convert everything into 64 bit unconditionally and just do simple range checks? > > I'll fix it with comments added. Nice catch. Is this the right thing to do though? I think 32 bit CPU might address something like 64G memory of highmem support, so a 64 bit value might actually be valid. Let's step back and understand what the motivation is? Maybe declaring all bars as 32 bit for 32 bit targets is enough? > > > > > > --- > > > hw/pci.c | 9 ++++++++- > > > hw/pci.h | 4 ++-- > > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/pci.c b/hw/pci.c > > > index c0ae66a..e7f8fb4 100644 > > > --- a/hw/pci.c > > > +++ b/hw/pci.c > > > @@ -524,7 +524,14 @@ static void pci_update_mappings(PCIDevice *d) > > > mappings, we handle specific values as invalid > > > mappings. */ > > > if (last_addr <= new_addr || new_addr == 0 || > > > - last_addr == PCI_BAR_UNMAPPED) { > > > + last_addr == PCI_BAR_UNMAPPED || > > > + > > > + /* Now pcibus_t is 64bit. > > > + * Check if 32 bit BAR wrap around explicitly. > > > + * Without this, PC ide doesn't work well. > > > + * TODO: remove this work around. > > > + */ > > > + last_addr >= UINT32_MAX) { > > > new_addr = PCI_BAR_UNMAPPED; > > > } > > > } else { > > > diff --git a/hw/pci.h b/hw/pci.h > > > index 8a187c2..8fbd45e 100644 > > > --- a/hw/pci.h > > > +++ b/hw/pci.h > > > @@ -71,8 +71,8 @@ extern target_phys_addr_t pci_mem_base; > > > #define PCI_DEVICE_ID_VIRTIO_BALLOON 0x1002 > > > #define PCI_DEVICE_ID_VIRTIO_CONSOLE 0x1003 > > > > > > -typedef uint32_t pcibus_t; > > > -#define FMT_PCIBUS PRIx32 > > > +typedef uint64_t pcibus_t; > > > +#define FMT_PCIBUS PRIx64 > > > > > > typedef void PCIConfigWriteFunc(PCIDevice *pci_dev, > > > uint32_t address, uint32_t data, int len); > > > > -- > yamahata
On Tue, Oct 13, 2009 at 04:39:15PM +0200, Michael S. Tsirkin wrote: > On Tue, Oct 13, 2009 at 10:31:33PM +0900, Isaku Yamahata wrote: > > On Sun, Oct 11, 2009 at 12:43:12PM +0200, Michael S. Tsirkin wrote: > > > On Fri, Oct 09, 2009 at 03:28:48PM +0900, Isaku Yamahata wrote: > > > > This patch is preliminary for 64bit bar. > > > > For 64bit bar support, change pcibus_t which represents > > > > pci bus addr/size from uint32_t to uint64_t. > > > > And also change FMT_pcibus for printf. > > > > > > > > In pci_update_mapping() checks 32bit overflow. > > > > So the check must be updated too. > > > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > > > > That's all fine, but if you look at users implementing > > > map io, they do: cpu_register_physical_memory() > > > on the address they are given. And if target_phys_addr_t is 32 bit, > > > this will silently truncate the address. > > > > > > So I would like to understand how this will all > > > work on 32 bit systems. > > > > The case is > > . BAR is memory 64bit and > > . target_phys_addr_t is 32bit and > > . bar is set to >4G. > > Hmm, the case isn't checked. > > > > It would be checked by > > - last_addr <= new_addr > > + (target_phys_addr_t)last_addr <= new_addr > > That's pretty tricky. Can we just convert everything into > 64 bit unconditionally and just do simple range checks? > > > > > I'll fix it with comments added. Nice catch. > > Is this the right thing to do though? > I think 32 bit CPU might address something like 64G > memory of highmem support, so a 64 bit value might > actually be valid. > > Let's step back and understand what the motivation is? > Maybe declaring all bars as 32 bit for 32 bit targets is enough? It is independent of guest OS for PCI device to have 64 bit BAR. It is valid to use PCI card with 64bit bar on 32 bit OS. In that case the OS will set the 64 bit bar within addressable region. And it is allowed for 32 bit OS to set 64bit BAR to >4GB. (which doesn't make sense, though.) How about adding the following check? last_addr >= TARGET_PHYS_ADDR_MAX
On Wed, Oct 14, 2009 at 01:35:49PM +0900, Isaku Yamahata wrote: > On Tue, Oct 13, 2009 at 04:39:15PM +0200, Michael S. Tsirkin wrote: > > On Tue, Oct 13, 2009 at 10:31:33PM +0900, Isaku Yamahata wrote: > > > On Sun, Oct 11, 2009 at 12:43:12PM +0200, Michael S. Tsirkin wrote: > > > > On Fri, Oct 09, 2009 at 03:28:48PM +0900, Isaku Yamahata wrote: > > > > > This patch is preliminary for 64bit bar. > > > > > For 64bit bar support, change pcibus_t which represents > > > > > pci bus addr/size from uint32_t to uint64_t. > > > > > And also change FMT_pcibus for printf. > > > > > > > > > > In pci_update_mapping() checks 32bit overflow. > > > > > So the check must be updated too. > > > > > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > > > > > > That's all fine, but if you look at users implementing > > > > map io, they do: cpu_register_physical_memory() > > > > on the address they are given. And if target_phys_addr_t is 32 bit, > > > > this will silently truncate the address. > > > > > > > > So I would like to understand how this will all > > > > work on 32 bit systems. > > > > > > The case is > > > . BAR is memory 64bit and > > > . target_phys_addr_t is 32bit and > > > . bar is set to >4G. > > > Hmm, the case isn't checked. > > > > > > It would be checked by > > > - last_addr <= new_addr > > > + (target_phys_addr_t)last_addr <= new_addr > > > > That's pretty tricky. Can we just convert everything into > > 64 bit unconditionally and just do simple range checks? > > > > > > > > I'll fix it with comments added. Nice catch. > > > > Is this the right thing to do though? > > I think 32 bit CPU might address something like 64G > > memory of highmem support, so a 64 bit value might > > actually be valid. > > > > Let's step back and understand what the motivation is? > > Maybe declaring all bars as 32 bit for 32 bit targets is enough? > > It is independent of guest OS for PCI device to have 64 bit BAR. > It is valid to use PCI card with 64bit bar on 32 bit OS. Yes, but qemu does not support this yet. > In that case > the OS will set the 64 bit bar within addressable region. > And it is allowed for 32 bit OS to set 64bit BAR to >4GB. > (which doesn't make sense, though.) Yes, it does. E.g. with high memory, a 32 bit OS can address more than 4G RAM. > > How about adding the following check? > last_addr >= TARGET_PHYS_ADDR_MAX And then what? > -- > yamahata
diff --git a/hw/pci.c b/hw/pci.c index c0ae66a..e7f8fb4 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -524,7 +524,14 @@ static void pci_update_mappings(PCIDevice *d) mappings, we handle specific values as invalid mappings. */ if (last_addr <= new_addr || new_addr == 0 || - last_addr == PCI_BAR_UNMAPPED) { + last_addr == PCI_BAR_UNMAPPED || + + /* Now pcibus_t is 64bit. + * Check if 32 bit BAR wrap around explicitly. + * Without this, PC ide doesn't work well. + * TODO: remove this work around. + */ + last_addr >= UINT32_MAX) { new_addr = PCI_BAR_UNMAPPED; } } else { diff --git a/hw/pci.h b/hw/pci.h index 8a187c2..8fbd45e 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -71,8 +71,8 @@ extern target_phys_addr_t pci_mem_base; #define PCI_DEVICE_ID_VIRTIO_BALLOON 0x1002 #define PCI_DEVICE_ID_VIRTIO_CONSOLE 0x1003 -typedef uint32_t pcibus_t; -#define FMT_PCIBUS PRIx32 +typedef uint64_t pcibus_t; +#define FMT_PCIBUS PRIx64 typedef void PCIConfigWriteFunc(PCIDevice *pci_dev, uint32_t address, uint32_t data, int len);
This patch is preliminary for 64bit bar. For 64bit bar support, change pcibus_t which represents pci bus addr/size from uint32_t to uint64_t. And also change FMT_pcibus for printf. In pci_update_mapping() checks 32bit overflow. So the check must be updated too. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/pci.c | 9 ++++++++- hw/pci.h | 4 ++-- 2 files changed, 10 insertions(+), 3 deletions(-)