Re: [Qemu-devel] [PATCH] pci: Add generic PCI device option to disable 64bit MMIO BARs

["Michael S. Tsirkin" <mst@redhat.com>]

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,