Patchwork pci: Add generic PCI device option to disable 64bit MMIO BARs

login
register
mail settings
Submitter Alex Williamson
Date Feb. 1, 2012, 10:57 p.m.
Message ID <20120201225203.26724.82424.stgit@bling.home>
Download mbox | patch
Permalink /patch/139019/
State New
Headers show

Comments

Alex Williamson - Feb. 1, 2012, 10:57 p.m.
As we start to enable 64bit I/O devices, there's a good chance
we'll find bugs and compatibility issues.  This allows a user
to toggle off (default on) 64bit PCI MMIO BARs, downgrading
them to 32bit BARs.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

Should this be an "x-mem64" option implying that it's really
only for debugging and may go away or could this have some
longevity?

 hw/pci.c |   18 ++++++++++++++++++
 hw/pci.h |    4 ++++
 2 files changed, 22 insertions(+), 0 deletions(-)
Michael S. Tsirkin - Feb. 1, 2012, 11:14 p.m.
On Wed, Feb 01, 2012 at 03:57:02PM -0700, Alex Williamson wrote:
> As we start to enable 64bit I/O devices, there's a good chance
> we'll find bugs and compatibility issues.  This allows a user
> to toggle off (default on) 64bit PCI MMIO BARs, downgrading
> them to 32bit BARs.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> Should this be an "x-mem64" option implying that it's really
> only for debugging and may go away or could this have some
> longevity?

I'd rename it x-force_32bit, for clarify.
Can this go into assigned devices?
Then the error handling below won't be a problem.

>  hw/pci.c |   18 ++++++++++++++++++
>  hw/pci.h |    4 ++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 57ec104..9afddb0 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -63,6 +63,8 @@ struct BusInfo pci_bus_info = {
>                          QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
>          DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
>                          QEMU_PCI_CAP_SERR_BITNR, true),
> +        DEFINE_PROP_BIT("mem64", PCIDevice, cap_present,
> +                        QEMU_PCI_CAP_MEM64_BITNR, true),
>          DEFINE_PROP_END_OF_LIST()
>      }
>  };
> @@ -957,6 +959,22 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>          exit(1);
>      }
>  
> +    if (!(type & PCI_BASE_ADDRESS_SPACE_IO)) {
> +        if (!(pci_dev->cap_present & QEMU_PCI_CAP_MEM64)) {
> +            type &= ~PCI_BASE_ADDRESS_MEM_TYPE_64;
> +        }
> +
> +        /* 32bit BARs are limited to 2GB */
> +        if (size >= 0x80000000U && !(type & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
> +            fprintf(stderr, "Device %04x:%02x:%02x.%x BAR %d is %ld "
> +                    "GB, 64bit memory type required\n",
> +                    pci_find_domain(pci_dev->bus), pci_bus_num(pci_dev->bus),
> +                    PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn),
> +                    region_num, (long)(size >> 30));
> +            exit(1);

That's an unfriendly way to handle errors, especially
for hotplug.  If you move the option to device itself,
then it can just fail init.

errors should also go to the monitor nowdays, so no fprintf.

> +        }
> +    }
> +
>      r = &pci_dev->io_regions[region_num];
>      r->addr = PCI_BAR_UNMAPPED;
>      r->size = size;
> diff --git a/hw/pci.h b/hw/pci.h
> index 4220151..17fd996 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -126,6 +126,10 @@ enum {
>      /* command register SERR bit enabled */
>  #define QEMU_PCI_CAP_SERR_BITNR 4
>      QEMU_PCI_CAP_SERR = (1 << QEMU_PCI_CAP_SERR_BITNR),
> +
> +    /* expose 64bit MMIO BARs when available */
> +#define QEMU_PCI_CAP_MEM64_BITNR 5
> +    QEMU_PCI_CAP_MEM64 = (1 << QEMU_PCI_CAP_MEM64_BITNR),
>  };
>  
>  typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector,
Alex Williamson - Feb. 1, 2012, 11:25 p.m.
On Thu, 2012-02-02 at 01:14 +0200, Michael S. Tsirkin wrote:
> On Wed, Feb 01, 2012 at 03:57:02PM -0700, Alex Williamson wrote:
> > As we start to enable 64bit I/O devices, there's a good chance
> > we'll find bugs and compatibility issues.  This allows a user
> > to toggle off (default on) 64bit PCI MMIO BARs, downgrading
> > them to 32bit BARs.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> > Should this be an "x-mem64" option implying that it's really
> > only for debugging and may go away or could this have some
> > longevity?
> 
> I'd rename it x-force_32bit, for clarify.
> Can this go into assigned devices?
> Then the error handling below won't be a problem.

That was kinda the point of putting it in common code so we don't end up
with pci-assign.mem64 vs ivshmem.dontuse64bitbars vs
virtio-net-pci.heyonlyuse32bitbars.  Do we think there's zero chance
that other drivers will want this?

> >  hw/pci.c |   18 ++++++++++++++++++
> >  hw/pci.h |    4 ++++
> >  2 files changed, 22 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 57ec104..9afddb0 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -63,6 +63,8 @@ struct BusInfo pci_bus_info = {
> >                          QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
> >          DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
> >                          QEMU_PCI_CAP_SERR_BITNR, true),
> > +        DEFINE_PROP_BIT("mem64", PCIDevice, cap_present,
> > +                        QEMU_PCI_CAP_MEM64_BITNR, true),
> >          DEFINE_PROP_END_OF_LIST()
> >      }
> >  };
> > @@ -957,6 +959,22 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
> >          exit(1);
> >      }
> >  
> > +    if (!(type & PCI_BASE_ADDRESS_SPACE_IO)) {
> > +        if (!(pci_dev->cap_present & QEMU_PCI_CAP_MEM64)) {
> > +            type &= ~PCI_BASE_ADDRESS_MEM_TYPE_64;
> > +        }
> > +
> > +        /* 32bit BARs are limited to 2GB */
> > +        if (size >= 0x80000000U && !(type & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
> > +            fprintf(stderr, "Device %04x:%02x:%02x.%x BAR %d is %ld "
> > +                    "GB, 64bit memory type required\n",
> > +                    pci_find_domain(pci_dev->bus), pci_bus_num(pci_dev->bus),
> > +                    PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn),
> > +                    region_num, (long)(size >> 30));
> > +            exit(1);
> 
> That's an unfriendly way to handle errors, especially
> for hotplug.  If you move the option to device itself,
> then it can just fail init.

It's unfriendly that we have so many void functions so we can't return a
proper error.  Hitting this error is so incredibly unlikely that I don't
think it deserves some grand rework...

> errors should also go to the monitor nowdays, so no fprintf.

Ok.

> > +        }
> > +    }
> > +
> >      r = &pci_dev->io_regions[region_num];
> >      r->addr = PCI_BAR_UNMAPPED;
> >      r->size = size;
> > diff --git a/hw/pci.h b/hw/pci.h
> > index 4220151..17fd996 100644
> > --- a/hw/pci.h
> > +++ b/hw/pci.h
> > @@ -126,6 +126,10 @@ enum {
> >      /* command register SERR bit enabled */
> >  #define QEMU_PCI_CAP_SERR_BITNR 4
> >      QEMU_PCI_CAP_SERR = (1 << QEMU_PCI_CAP_SERR_BITNR),
> > +
> > +    /* expose 64bit MMIO BARs when available */
> > +#define QEMU_PCI_CAP_MEM64_BITNR 5
> > +    QEMU_PCI_CAP_MEM64 = (1 << QEMU_PCI_CAP_MEM64_BITNR),
> >  };
> >  
> >  typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector,
Alex Williamson - Feb. 1, 2012, 11:44 p.m.
On Wed, 2012-02-01 at 15:57 -0700, Alex Williamson wrote:
> As we start to enable 64bit I/O devices, there's a good chance
> we'll find bugs and compatibility issues.  This allows a user
> to toggle off (default on) 64bit PCI MMIO BARs, downgrading
> them to 32bit BARs.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> Should this be an "x-mem64" option implying that it's really
> only for debugging and may go away or could this have some
> longevity?
> 
>  hw/pci.c |   18 ++++++++++++++++++
>  hw/pci.h |    4 ++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 57ec104..9afddb0 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -63,6 +63,8 @@ struct BusInfo pci_bus_info = {
>                          QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
>          DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
>                          QEMU_PCI_CAP_SERR_BITNR, true),
> +        DEFINE_PROP_BIT("mem64", PCIDevice, cap_present,
> +                        QEMU_PCI_CAP_MEM64_BITNR, true),
>          DEFINE_PROP_END_OF_LIST()
>      }
>  };
> @@ -957,6 +959,22 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
>          exit(1);
>      }
>  
> +    if (!(type & PCI_BASE_ADDRESS_SPACE_IO)) {
> +        if (!(pci_dev->cap_present & QEMU_PCI_CAP_MEM64)) {
> +            type &= ~PCI_BASE_ADDRESS_MEM_TYPE_64;
> +        }
> +
> +        /* 32bit BARs are limited to 2GB */
> +        if (size >= 0x80000000U && !(type & PCI_BASE_ADDRESS_MEM_TYPE_64)) {

Oops, this should just be >, not >=.  I'll wait to see if this has any
legs before respinning.

> +            fprintf(stderr, "Device %04x:%02x:%02x.%x BAR %d is %ld "
> +                    "GB, 64bit memory type required\n",
> +                    pci_find_domain(pci_dev->bus), pci_bus_num(pci_dev->bus),
> +                    PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn),
> +                    region_num, (long)(size >> 30));
> +            exit(1);
> +        }
> +    }
> +
>      r = &pci_dev->io_regions[region_num];
>      r->addr = PCI_BAR_UNMAPPED;
>      r->size = size;
> diff --git a/hw/pci.h b/hw/pci.h
> index 4220151..17fd996 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -126,6 +126,10 @@ enum {
>      /* command register SERR bit enabled */
>  #define QEMU_PCI_CAP_SERR_BITNR 4
>      QEMU_PCI_CAP_SERR = (1 << QEMU_PCI_CAP_SERR_BITNR),
> +
> +    /* expose 64bit MMIO BARs when available */
> +#define QEMU_PCI_CAP_MEM64_BITNR 5
> +    QEMU_PCI_CAP_MEM64 = (1 << QEMU_PCI_CAP_MEM64_BITNR),
>  };
>  
>  typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector,
>

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 57ec104..9afddb0 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -63,6 +63,8 @@  struct BusInfo pci_bus_info = {
                         QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
         DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
                         QEMU_PCI_CAP_SERR_BITNR, true),
+        DEFINE_PROP_BIT("mem64", PCIDevice, cap_present,
+                        QEMU_PCI_CAP_MEM64_BITNR, true),
         DEFINE_PROP_END_OF_LIST()
     }
 };
@@ -957,6 +959,22 @@  void pci_register_bar(PCIDevice *pci_dev, int region_num,
         exit(1);
     }
 
+    if (!(type & PCI_BASE_ADDRESS_SPACE_IO)) {
+        if (!(pci_dev->cap_present & QEMU_PCI_CAP_MEM64)) {
+            type &= ~PCI_BASE_ADDRESS_MEM_TYPE_64;
+        }
+
+        /* 32bit BARs are limited to 2GB */
+        if (size >= 0x80000000U && !(type & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
+            fprintf(stderr, "Device %04x:%02x:%02x.%x BAR %d is %ld "
+                    "GB, 64bit memory type required\n",
+                    pci_find_domain(pci_dev->bus), pci_bus_num(pci_dev->bus),
+                    PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn),
+                    region_num, (long)(size >> 30));
+            exit(1);
+        }
+    }
+
     r = &pci_dev->io_regions[region_num];
     r->addr = PCI_BAR_UNMAPPED;
     r->size = size;
diff --git a/hw/pci.h b/hw/pci.h
index 4220151..17fd996 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -126,6 +126,10 @@  enum {
     /* command register SERR bit enabled */
 #define QEMU_PCI_CAP_SERR_BITNR 4
     QEMU_PCI_CAP_SERR = (1 << QEMU_PCI_CAP_SERR_BITNR),
+
+    /* expose 64bit MMIO BARs when available */
+#define QEMU_PCI_CAP_MEM64_BITNR 5
+    QEMU_PCI_CAP_MEM64 = (1 << QEMU_PCI_CAP_MEM64_BITNR),
 };
 
 typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector,