Patchwork [V5,15/29] pci: typedef pcibus_t as uint64_t instead of uint32_t.

login
register
mail settings
Submitter Isaku Yamahata
Date Oct. 9, 2009, 6:28 a.m.
Message ID <1255069742-15724-16-git-send-email-yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/35566/
State Under Review
Headers show

Comments

Isaku Yamahata - Oct. 9, 2009, 6:28 a.m.
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(-)
Michael S. Tsirkin - Oct. 11, 2009, 10:43 a.m.
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
Isaku Yamahata - Oct. 13, 2009, 1:31 p.m.
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);
>
Michael S. Tsirkin - Oct. 13, 2009, 2:39 p.m.
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
Isaku Yamahata - Oct. 14, 2009, 4:35 a.m.
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
Michael S. Tsirkin - Oct. 14, 2009, 8:55 a.m.
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

Patch

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);