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@xxxxxxxxxx>
> > ---
> >
> > 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,
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
