| Message ID | 20120201225203.26724.82424.stgit@bling.home |
|---|---|
| State | New |
| Headers | show |
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,
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,
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, >
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,
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(-)