[Qemu-devel] [PATCH 0/2 v2] vfio: blacklist loading of unstable roms

[Qemu-devel] [PATCH 0/2 v2] vfio: blacklist loading of unstable roms

[Bandan Das]

v2:
1. Break up into two patches separating the infrastructural changes
2. Change vendor_id and device_id type to uint16_t
3. Rename struct for blacklisted devid and vendorid to more meaningful names
4. Remove unnecessary rom_quirk variable and just call vfio_blacklist_opt_rom()
in vfio_pci_size_rom()

Bandan Das (2):
  pci: change default value of rom_bar to 2
  vfio: blacklist loading of unstable roms

 hw/misc/vfio.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci/pci.c   |  7 ++++++-
 2 files changed, 64 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2

[Bandan Das]

The following patch depends on the value of rom_bar to
determine rom blacklist behavior. Existing code shouldn't
be affected by changing the default value of rom_bar since
all relevant decisions only rely on whether rom_bar is zero
or non-zero.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/pci/pci.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 4e0701d..12c3e27 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -53,7 +53,12 @@ static void pci_bus_finalize(Object *obj);
 static Property pci_props[] = {
     DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
     DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
-    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
+    /*
+     * 0 = disable
+     * 1 = user requested on, force loading even if rom blacklisted
+     * 2 = enabled but disables loading of blacklisted roms (default)
+     */
+    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),
     DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
                     QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
     DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2 v2] vfio: blacklist loading of unstable roms

[Bandan Das]

Certain cards such as the Broadcom BCM57810 have rom quirks
that exhibit unstable system behavior duing device assignment. In
the particular case of 57810, rom execution hangs and if a FLR
follows, the device becomes inoperable until a power cycle. This
is a simple change to blacklist loading of rom for such cards
unless the user specifies a romfile or rombar=1 on the cmd line

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 hw/misc/vfio.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 8db182f..58f348e 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -209,6 +209,16 @@ typedef struct VFIOGroup {
     QLIST_ENTRY(VFIOGroup) container_next;
 } VFIOGroup;
 
+typedef struct VFIORomBlacklistEntry {
+    uint16_t vendor_id;
+    uint16_t device_id;
+} VFIORomBlacklistEntry;
+
+static const VFIORomBlacklistEntry romblacklist[] = {
+    /* Broadcom BCM 57810 */
+    { 0x14e4, 0x168e }
+};
+
 #define MSIX_CAP_LENGTH 12
 
 static QLIST_HEAD(, VFIOContainer)
@@ -1197,6 +1207,26 @@ static const MemoryRegionOps vfio_rom_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static bool vfio_blacklist_opt_rom(VFIODevice *vdev)
+{
+    PCIDevice *pdev = &vdev->pdev;
+    uint16_t vendor_id, device_id;
+    int count = 0;
+
+    vendor_id = pci_get_word(pdev->config + PCI_VENDOR_ID);
+    device_id = pci_get_word(pdev->config + PCI_DEVICE_ID);
+
+    while (count < ARRAY_SIZE(romblacklist)) {
+        if (romblacklist[count].vendor_id == vendor_id &&
+            romblacklist[count].device_id == device_id) {
+                return true;
+        }
+        count++;
+    }
+
+    return false;
+}
+
 static void vfio_pci_size_rom(VFIODevice *vdev)
 {
     uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
@@ -1204,9 +1234,37 @@ static void vfio_pci_size_rom(VFIODevice *vdev)
     char name[32];
 
     if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
+        /* Since pci handles romfile, just print a message and return */
+        if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
+            error_printf("Warning : Device at %04x:%02x:%02x.%x "
+                         "is known to cause system instability issues during "
+                         "option rom execution. "
+                         "Proceeding anyway since user specified romfile\n",
+                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
+                         vdev->host.function);
+        }
         return;
     }
 
+    if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.rom_bar) {
+        if (vdev->pdev.rom_bar == 1) {
+            error_printf("Warning : Device at %04x:%02x:%02x.%x "
+                         "is known to cause system instability issues during "
+                         "option rom execution. "
+                         "Proceeding anyway since user specified rombar=1\n",
+                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
+                         vdev->host.function);
+        } else {
+            error_printf("Warning : Rom loading for device at "
+                         "%04x:%02x:%02x.%x has been disabled due to "
+                         "system instability issues. "
+                         "Specify rombar=1 or romfile to force\n",
+                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
+                         vdev->host.function);
+            return;
+        }
+    }
+
     /*
      * Use the same size ROM BAR as the physical device.  The contents
      * will get filled in later when the guest tries to read it.
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2

[Alex Williamson]

On Wed, 2014-02-19 at 15:20 -0500, Bandan Das wrote:
> The following patch depends on the value of rom_bar to
> determine rom blacklist behavior. Existing code shouldn't
> be affected by changing the default value of rom_bar since
> all relevant decisions only rely on whether rom_bar is zero
> or non-zero.
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  hw/pci/pci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 4e0701d..12c3e27 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -53,7 +53,12 @@ static void pci_bus_finalize(Object *obj);
>  static Property pci_props[] = {
>      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> -    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
> +    /*
> +     * 0 = disable
> +     * 1 = user requested on, force loading even if rom blacklisted
> +     * 2 = enabled but disables loading of blacklisted roms (default)
> +     */
> +    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),
>      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
>                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
>      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,

A slightly more satisfying option might be to define rom_bar as int32_t
with default of -1.  I don't know if that would break libvirt though.
I'll let MST weigh in.  Thanks,

Alex

Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2

[Bandan Das]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Wed, 2014-02-19 at 15:20 -0500, Bandan Das wrote:
>> The following patch depends on the value of rom_bar to
>> determine rom blacklist behavior. Existing code shouldn't
>> be affected by changing the default value of rom_bar since
>> all relevant decisions only rely on whether rom_bar is zero
>> or non-zero.
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  hw/pci/pci.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 4e0701d..12c3e27 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -53,7 +53,12 @@ static void pci_bus_finalize(Object *obj);
>>  static Property pci_props[] = {
>>      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>>      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
>> -    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
>> +    /*
>> +     * 0 = disable
>> +     * 1 = user requested on, force loading even if rom blacklisted
>> +     * 2 = enabled but disables loading of blacklisted roms (default)
>> +     */
>> +    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),
>>      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
>>                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
>>      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
>
> A slightly more satisfying option might be to define rom_bar as int32_t
> with default of -1.  I don't know if that would break libvirt though.
> I'll let MST weigh in.  Thanks,

IMO, since the default is "enabled", having a value of -1 for an enabled
option could be a bit confusing to some.

Bandan

> Alex

Re: [Qemu-devel] [PATCH 2/2 v2] vfio: blacklist loading of unstable roms

[Alex Williamson]

On Wed, 2014-02-19 at 15:20 -0500, Bandan Das wrote:
> Certain cards such as the Broadcom BCM57810 have rom quirks
> that exhibit unstable system behavior duing device assignment. In
> the particular case of 57810, rom execution hangs and if a FLR
> follows, the device becomes inoperable until a power cycle. This
> is a simple change to blacklist loading of rom for such cards
> unless the user specifies a romfile or rombar=1 on the cmd line
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  hw/misc/vfio.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 8db182f..58f348e 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -209,6 +209,16 @@ typedef struct VFIOGroup {
>      QLIST_ENTRY(VFIOGroup) container_next;
>  } VFIOGroup;
>  
> +typedef struct VFIORomBlacklistEntry {
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +} VFIORomBlacklistEntry;
> +
> +static const VFIORomBlacklistEntry romblacklist[] = {
> +    /* Broadcom BCM 57810 */
> +    { 0x14e4, 0x168e }
> +};
> +

Ideally we'd want to be able to reference a bugzilla here so we have
some reference in the code to track developments.  Also, can we capture
the ROM version known to cause this problem so if somebody later says
that it works we can have something to compare?  The PCI firmware spec
defines the data structure.  Effectively you can dump the ROM from the
device, run xxd on it and look for the "PCIR" string that defines the
start of the PCI data structure.  The 2 bytes at offset 12h (where 0h is
the 'P' in "PCIR") have a revision level field.

>  #define MSIX_CAP_LENGTH 12
>  
>  static QLIST_HEAD(, VFIOContainer)
> @@ -1197,6 +1207,26 @@ static const MemoryRegionOps vfio_rom_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +static bool vfio_blacklist_opt_rom(VFIODevice *vdev)
> +{
> +    PCIDevice *pdev = &vdev->pdev;
> +    uint16_t vendor_id, device_id;
> +    int count = 0;
> +
> +    vendor_id = pci_get_word(pdev->config + PCI_VENDOR_ID);
> +    device_id = pci_get_word(pdev->config + PCI_DEVICE_ID);
> +
> +    while (count < ARRAY_SIZE(romblacklist)) {
> +        if (romblacklist[count].vendor_id == vendor_id &&
> +            romblacklist[count].device_id == device_id) {
> +                return true;
> +        }
> +        count++;
> +    }
> +
> +    return false;
> +}
> +
>  static void vfio_pci_size_rom(VFIODevice *vdev)
>  {
>      uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
> @@ -1204,9 +1234,37 @@ static void vfio_pci_size_rom(VFIODevice *vdev)
>      char name[32];
>  
>      if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
> +        /* Since pci handles romfile, just print a message and return */
> +        if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
> +            error_printf("Warning : Device at %04x:%02x:%02x.%x "
> +                         "is known to cause system instability issues during "
> +                         "option rom execution. "
> +                         "Proceeding anyway since user specified romfile\n",
> +                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
> +                         vdev->host.function);
> +        }
>          return;
>      }
>  
> +    if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.rom_bar) {
> +        if (vdev->pdev.rom_bar == 1) {
> +            error_printf("Warning : Device at %04x:%02x:%02x.%x "
> +                         "is known to cause system instability issues during "
> +                         "option rom execution. "
> +                         "Proceeding anyway since user specified rombar=1\n",
> +                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
> +                         vdev->host.function);
> +        } else {
> +            error_printf("Warning : Rom loading for device at "
> +                         "%04x:%02x:%02x.%x has been disabled due to "
> +                         "system instability issues. "
> +                         "Specify rombar=1 or romfile to force\n",
> +                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
> +                         vdev->host.function);
> +            return;
> +        }
> +    }
> +

What happens if this card, or some later user of this blacklisting, has
SKUs that don't have a ROM?  Aren't we going to print this last warning
regardless of whether we found anything to load?  Just shifting this
whole block down below the next couple tests is probably sufficient.
Thanks,

Alex

>      /*
>       * Use the same size ROM BAR as the physical device.  The contents
>       * will get filled in later when the guest tries to read it.

Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2

[Michael S. Tsirkin]

On Wed, Feb 19, 2014 at 03:20:54PM -0500, Bandan Das wrote:
> The following patch depends on the value of rom_bar to
> determine rom blacklist behavior. Existing code shouldn't
> be affected by changing the default value of rom_bar since
> all relevant decisions only rely on whether rom_bar is zero
> or non-zero.
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  hw/pci/pci.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 4e0701d..12c3e27 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -53,7 +53,12 @@ static void pci_bus_finalize(Object *obj);
>  static Property pci_props[] = {
>      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> -    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
> +    /*
> +     * 0 = disable
> +     * 1 = user requested on, force loading even if rom blacklisted
> +     * 2 = enabled but disables loading of blacklisted roms (default)
> +     */
> +    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),

How do users figure out this interface?
Read code?
Could we add a bit property rombarforce=on/off instead?
Seems better.

Maybe we should teach bool type visitors
about 0 and 1 being legal values
(call out to int visitor, then check value 0 or 1),
then rombar can be changed to bit property too.

Also, this will need QMP support right?
IIUC rombar is not exposed in QMP ATM.

>      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
>                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
>      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2

[Michael S. Tsirkin]

On Wed, Feb 19, 2014 at 01:36:45PM -0700, Alex Williamson wrote:
> On Wed, 2014-02-19 at 15:20 -0500, Bandan Das wrote:
> > The following patch depends on the value of rom_bar to
> > determine rom blacklist behavior. Existing code shouldn't
> > be affected by changing the default value of rom_bar since
> > all relevant decisions only rely on whether rom_bar is zero
> > or non-zero.
> > 
> > Signed-off-by: Bandan Das <bsd@redhat.com>
> > ---
> >  hw/pci/pci.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 4e0701d..12c3e27 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -53,7 +53,12 @@ static void pci_bus_finalize(Object *obj);
> >  static Property pci_props[] = {
> >      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> >      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> > -    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
> > +    /*
> > +     * 0 = disable
> > +     * 1 = user requested on, force loading even if rom blacklisted
> > +     * 2 = enabled but disables loading of blacklisted roms (default)
> > +     */
> > +    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),
> >      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
> >                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
> >      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
> 
> A slightly more satisfying option might be to define rom_bar as int32_t
> with default of -1.  I don't know if that would break libvirt though.
> I'll let MST weigh in.  Thanks,
> 
> Alex

I don't see rombar in json schema at all.
I think it was designed as an internal flag
for compatibility with legacy machine types.
As such it's likely not a good interface
for users.

-- 
MST

Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2

[Bandan Das]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Feb 19, 2014 at 03:20:54PM -0500, Bandan Das wrote:
>> The following patch depends on the value of rom_bar to
>> determine rom blacklist behavior. Existing code shouldn't
>> be affected by changing the default value of rom_bar since
>> all relevant decisions only rely on whether rom_bar is zero
>> or non-zero.
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  hw/pci/pci.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 4e0701d..12c3e27 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -53,7 +53,12 @@ static void pci_bus_finalize(Object *obj);
>>  static Property pci_props[] = {
>>      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>>      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
>> -    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
>> +    /*
>> +     * 0 = disable
>> +     * 1 = user requested on, force loading even if rom blacklisted
>> +     * 2 = enabled but disables loading of blacklisted roms (default)
>> +     */
>> +    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),
>
> How do users figure out this interface?
> Read code?
> Could we add a bit property rombarforce=on/off instead?
> Seems better.

Fine with me, but aren't rombarforce and rombar a little 
bit similar in thier functions ? 

1. rombarforce=on automatically implies rombar=1 (force)
2. rombarforce=off(default), rombar=1 (on but don't force loading
if blacklisted)
3. rombarforce=on, rombar=0 (do nothing)
4. rombarforce=off, rombar=0 (again nothing)

So why not just a tristate rombar=on/off/force

> Maybe we should teach bool type visitors
> about 0 and 1 being legal values
> (call out to int visitor, then check value 0 or 1),
> then rombar can be changed to bit property too.
>
> Also, this will need QMP support right?
> IIUC rombar is not exposed in QMP ATM.

Will qmp hotplug care about option rom ? If not, atleast the reboot
will, so I think this is needed.
 
>>      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
>>                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
>>      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
>> -- 
>> 1.8.3.1

Re: [Qemu-devel] [PATCH 2/2 v2] vfio: blacklist loading of unstable roms

[Bandan Das]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Wed, 2014-02-19 at 15:20 -0500, Bandan Das wrote:
>> Certain cards such as the Broadcom BCM57810 have rom quirks
>> that exhibit unstable system behavior duing device assignment. In
>> the particular case of 57810, rom execution hangs and if a FLR
>> follows, the device becomes inoperable until a power cycle. This
>> is a simple change to blacklist loading of rom for such cards
>> unless the user specifies a romfile or rombar=1 on the cmd line
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  hw/misc/vfio.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 58 insertions(+)
>> 
>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>> index 8db182f..58f348e 100644
>> --- a/hw/misc/vfio.c
>> +++ b/hw/misc/vfio.c
>> @@ -209,6 +209,16 @@ typedef struct VFIOGroup {
>>      QLIST_ENTRY(VFIOGroup) container_next;
>>  } VFIOGroup;
>>  
>> +typedef struct VFIORomBlacklistEntry {
>> +    uint16_t vendor_id;
>> +    uint16_t device_id;
>> +} VFIORomBlacklistEntry;
>> +
>> +static const VFIORomBlacklistEntry romblacklist[] = {
>> +    /* Broadcom BCM 57810 */
>> +    { 0x14e4, 0x168e }
>> +};
>> +
>
> Ideally we'd want to be able to reference a bugzilla here so we have
> some reference in the code to track developments.  Also, can we capture
> the ROM version known to cause this problem so if somebody later says
> that it works we can have something to compare?  The PCI firmware spec
> defines the data structure.  Effectively you can dump the ROM from the
> device, run xxd on it and look for the "PCIR" string that defines the
> start of the PCI data structure.  The 2 bytes at offset 12h (where 0h is
> the 'P' in "PCIR") have a revision level field.
>
>>  #define MSIX_CAP_LENGTH 12
>>  
>>  static QLIST_HEAD(, VFIOContainer)
>> @@ -1197,6 +1207,26 @@ static const MemoryRegionOps vfio_rom_ops = {
>>      .endianness = DEVICE_LITTLE_ENDIAN,
>>  };
>>  
>> +static bool vfio_blacklist_opt_rom(VFIODevice *vdev)
>> +{
>> +    PCIDevice *pdev = &vdev->pdev;
>> +    uint16_t vendor_id, device_id;
>> +    int count = 0;
>> +
>> +    vendor_id = pci_get_word(pdev->config + PCI_VENDOR_ID);
>> +    device_id = pci_get_word(pdev->config + PCI_DEVICE_ID);
>> +
>> +    while (count < ARRAY_SIZE(romblacklist)) {
>> +        if (romblacklist[count].vendor_id == vendor_id &&
>> +            romblacklist[count].device_id == device_id) {
>> +                return true;
>> +        }
>> +        count++;
>> +    }
>> +
>> +    return false;
>> +}
>> +
>>  static void vfio_pci_size_rom(VFIODevice *vdev)
>>  {
>>      uint32_t orig, size = cpu_to_le32((uint32_t)PCI_ROM_ADDRESS_MASK);
>> @@ -1204,9 +1234,37 @@ static void vfio_pci_size_rom(VFIODevice *vdev)
>>      char name[32];
>>  
>>      if (vdev->pdev.romfile || !vdev->pdev.rom_bar) {
>> +        /* Since pci handles romfile, just print a message and return */
>> +        if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.romfile) {
>> +            error_printf("Warning : Device at %04x:%02x:%02x.%x "
>> +                         "is known to cause system instability issues during "
>> +                         "option rom execution. "
>> +                         "Proceeding anyway since user specified romfile\n",
>> +                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> +                         vdev->host.function);
>> +        }
>>          return;
>>      }
>>  
>> +    if (vfio_blacklist_opt_rom(vdev) && vdev->pdev.rom_bar) {
>> +        if (vdev->pdev.rom_bar == 1) {
>> +            error_printf("Warning : Device at %04x:%02x:%02x.%x "
>> +                         "is known to cause system instability issues during "
>> +                         "option rom execution. "
>> +                         "Proceeding anyway since user specified rombar=1\n",
>> +                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> +                         vdev->host.function);
>> +        } else {
>> +            error_printf("Warning : Rom loading for device at "
>> +                         "%04x:%02x:%02x.%x has been disabled due to "
>> +                         "system instability issues. "
>> +                         "Specify rombar=1 or romfile to force\n",
>> +                         vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> +                         vdev->host.function);
>> +            return;
>> +        }
>> +    }
>> +
>
> What happens if this card, or some later user of this blacklisting, has
> SKUs that don't have a ROM?  Aren't we going to print this last warning
> regardless of whether we found anything to load?  Just shifting this
> whole block down below the next couple tests is probably sufficient.
> Thanks,

Thanks for catching this! I will shift this down for the next version..

> Alex
>
>>      /*
>>       * Use the same size ROM BAR as the physical device.  The contents
>>       * will get filled in later when the guest tries to read it.

Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2

[Alex Williamson]

On Thu, 2014-02-20 at 10:12 +0200, Michael S. Tsirkin wrote:
> On Wed, Feb 19, 2014 at 03:20:54PM -0500, Bandan Das wrote:
> > The following patch depends on the value of rom_bar to
> > determine rom blacklist behavior. Existing code shouldn't
> > be affected by changing the default value of rom_bar since
> > all relevant decisions only rely on whether rom_bar is zero
> > or non-zero.
> > 
> > Signed-off-by: Bandan Das <bsd@redhat.com>
> > ---
> >  hw/pci/pci.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 4e0701d..12c3e27 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -53,7 +53,12 @@ static void pci_bus_finalize(Object *obj);
> >  static Property pci_props[] = {
> >      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> >      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> > -    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
> > +    /*
> > +     * 0 = disable
> > +     * 1 = user requested on, force loading even if rom blacklisted
> > +     * 2 = enabled but disables loading of blacklisted roms (default)
> > +     */
> > +    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),
> 
> How do users figure out this interface?
> Read code?
> Could we add a bit property rombarforce=on/off instead?
> Seems better.
> 
> Maybe we should teach bool type visitors
> about 0 and 1 being legal values
> (call out to int visitor, then check value 0 or 1),
> then rombar can be changed to bit property too.
> 
> Also, this will need QMP support right?
> IIUC rombar is not exposed in QMP ATM.

rombarforce seems very redundant for a user interface; rombar=1 "expose
the ROM BAR of the device", rombarforce=1 "yes, really expose the ROM
BAR of the device".  Even if force implies rombar, I don't think that's
very easy to code in libvirt.  I think we really just want to detect
unspecified versus specified, which probably means setting the default
value to something the user can't, or at least wouldn't, specify.
Thanks,

Alex

> 
> >      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
> >                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
> >      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2

[Michael S. Tsirkin]

On Sat, Feb 22, 2014 at 04:28:26PM -0700, Alex Williamson wrote:
> On Thu, 2014-02-20 at 10:12 +0200, Michael S. Tsirkin wrote:
> > On Wed, Feb 19, 2014 at 03:20:54PM -0500, Bandan Das wrote:
> > > The following patch depends on the value of rom_bar to
> > > determine rom blacklist behavior. Existing code shouldn't
> > > be affected by changing the default value of rom_bar since
> > > all relevant decisions only rely on whether rom_bar is zero
> > > or non-zero.
> > > 
> > > Signed-off-by: Bandan Das <bsd@redhat.com>
> > > ---
> > >  hw/pci/pci.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 4e0701d..12c3e27 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -53,7 +53,12 @@ static void pci_bus_finalize(Object *obj);
> > >  static Property pci_props[] = {
> > >      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> > >      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> > > -    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
> > > +    /*
> > > +     * 0 = disable
> > > +     * 1 = user requested on, force loading even if rom blacklisted
> > > +     * 2 = enabled but disables loading of blacklisted roms (default)
> > > +     */
> > > +    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),
> > 
> > How do users figure out this interface?
> > Read code?
> > Could we add a bit property rombarforce=on/off instead?
> > Seems better.
> > 
> > Maybe we should teach bool type visitors
> > about 0 and 1 being legal values
> > (call out to int visitor, then check value 0 or 1),
> > then rombar can be changed to bit property too.
> > 
> > Also, this will need QMP support right?
> > IIUC rombar is not exposed in QMP ATM.
> 
> rombarforce seems very redundant for a user interface; rombar=1 "expose
> the ROM BAR of the device", rombarforce=1 "yes, really expose the ROM
> BAR of the device".

Not really.
In this design, rombarforce=yes means "expose ROM BAR of the device",
rombar should not be exposed to users - it's a compatibility property
used for cross-version migration.

> Even if force implies rombar,
> I don't think that's
> very easy to code in libvirt.

Libvirt doesn't touch rombar AFAIK.

>  I think we really just want to detect
> unspecified versus specified, which probably means setting the default
> value to something the user can't, or at least wouldn't, specify.
> Thanks,
> 
> Alex

OK but I should be able to query value of each variable and figure
out what it means.

We can build a tri-state property type if desired:
force on/force off/auto.
Just let's not code up random magic values.
0 and 1 for on/off is ugly enough.


> > 
> > >      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
> > >                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
> > >      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
> > > -- 
> > > 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2

[Alex Williamson]

On Sun, 2014-02-23 at 08:32 +0200, Michael S. Tsirkin wrote:
> On Sat, Feb 22, 2014 at 04:28:26PM -0700, Alex Williamson wrote:
> > On Thu, 2014-02-20 at 10:12 +0200, Michael S. Tsirkin wrote:
> > > On Wed, Feb 19, 2014 at 03:20:54PM -0500, Bandan Das wrote:
> > > > The following patch depends on the value of rom_bar to
> > > > determine rom blacklist behavior. Existing code shouldn't
> > > > be affected by changing the default value of rom_bar since
> > > > all relevant decisions only rely on whether rom_bar is zero
> > > > or non-zero.
> > > > 
> > > > Signed-off-by: Bandan Das <bsd@redhat.com>
> > > > ---
> > > >  hw/pci/pci.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index 4e0701d..12c3e27 100644
> > > > --- a/hw/pci/pci.c
> > > > +++ b/hw/pci/pci.c
> > > > @@ -53,7 +53,12 @@ static void pci_bus_finalize(Object *obj);
> > > >  static Property pci_props[] = {
> > > >      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> > > >      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> > > > -    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
> > > > +    /*
> > > > +     * 0 = disable
> > > > +     * 1 = user requested on, force loading even if rom blacklisted
> > > > +     * 2 = enabled but disables loading of blacklisted roms (default)
> > > > +     */
> > > > +    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),
> > > 
> > > How do users figure out this interface?
> > > Read code?
> > > Could we add a bit property rombarforce=on/off instead?
> > > Seems better.
> > > 
> > > Maybe we should teach bool type visitors
> > > about 0 and 1 being legal values
> > > (call out to int visitor, then check value 0 or 1),
> > > then rombar can be changed to bit property too.
> > > 
> > > Also, this will need QMP support right?
> > > IIUC rombar is not exposed in QMP ATM.
> > 
> > rombarforce seems very redundant for a user interface; rombar=1 "expose
> > the ROM BAR of the device", rombarforce=1 "yes, really expose the ROM
> > BAR of the device".
> 
> Not really.
> In this design, rombarforce=yes means "expose ROM BAR of the device",
> rombar should not be exposed to users - it's a compatibility property
> used for cross-version migration.
> 
> > Even if force implies rombar,
> > I don't think that's
> > very easy to code in libvirt.
> 
> Libvirt doesn't touch rombar AFAIK.

It does

http://libvirt.org/formatdomain.html#elementsNICSROM

<rom bar='off'>

> >  I think we really just want to detect
> > unspecified versus specified, which probably means setting the default
> > value to something the user can't, or at least wouldn't, specify.
> > Thanks,
> > 
> > Alex
> 
> OK but I should be able to query value of each variable and figure
> out what it means.
> 
> We can build a tri-state property type if desired:
> force on/force off/auto.
> Just let's not code up random magic values.
> 0 and 1 for on/off is ugly enough.
> 
> 
> > > 
> > > >      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
> > > >                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
> > > >      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
> > > > -- 
> > > > 1.8.3.1
> > 
> > 

Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2

[Michael S. Tsirkin]

On Sun, Feb 23, 2014 at 07:18:07AM -0700, Alex Williamson wrote:
> On Sun, 2014-02-23 at 08:32 +0200, Michael S. Tsirkin wrote:
> > On Sat, Feb 22, 2014 at 04:28:26PM -0700, Alex Williamson wrote:
> > > On Thu, 2014-02-20 at 10:12 +0200, Michael S. Tsirkin wrote:
> > > > On Wed, Feb 19, 2014 at 03:20:54PM -0500, Bandan Das wrote:
> > > > > The following patch depends on the value of rom_bar to
> > > > > determine rom blacklist behavior. Existing code shouldn't
> > > > > be affected by changing the default value of rom_bar since
> > > > > all relevant decisions only rely on whether rom_bar is zero
> > > > > or non-zero.
> > > > > 
> > > > > Signed-off-by: Bandan Das <bsd@redhat.com>
> > > > > ---
> > > > >  hw/pci/pci.c | 7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > > index 4e0701d..12c3e27 100644
> > > > > --- a/hw/pci/pci.c
> > > > > +++ b/hw/pci/pci.c
> > > > > @@ -53,7 +53,12 @@ static void pci_bus_finalize(Object *obj);
> > > > >  static Property pci_props[] = {
> > > > >      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> > > > >      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> > > > > -    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
> > > > > +    /*
> > > > > +     * 0 = disable
> > > > > +     * 1 = user requested on, force loading even if rom blacklisted
> > > > > +     * 2 = enabled but disables loading of blacklisted roms (default)
> > > > > +     */
> > > > > +    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),
> > > > 
> > > > How do users figure out this interface?
> > > > Read code?
> > > > Could we add a bit property rombarforce=on/off instead?
> > > > Seems better.
> > > > 
> > > > Maybe we should teach bool type visitors
> > > > about 0 and 1 being legal values
> > > > (call out to int visitor, then check value 0 or 1),
> > > > then rombar can be changed to bit property too.
> > > > 
> > > > Also, this will need QMP support right?
> > > > IIUC rombar is not exposed in QMP ATM.
> > > 
> > > rombarforce seems very redundant for a user interface; rombar=1 "expose
> > > the ROM BAR of the device", rombarforce=1 "yes, really expose the ROM
> > > BAR of the device".
> > 
> > Not really.
> > In this design, rombarforce=yes means "expose ROM BAR of the device",
> > rombar should not be exposed to users - it's a compatibility property
> > used for cross-version migration.
> > 
> > > Even if force implies rombar,
> > > I don't think that's
> > > very easy to code in libvirt.
> > 
> > Libvirt doesn't touch rombar AFAIK.
> 
> It does
> 
> http://libvirt.org/formatdomain.html#elementsNICSROM
> 
> <rom bar='off'>


Got it, thanks. So if you think the right thing
to do for users it to interpret rom=on as
meaning "force" then just do that.
Use some new hidden field for machine compatibility.


> > >  I think we really just want to detect
> > > unspecified versus specified, which probably means setting the default
> > > value to something the user can't, or at least wouldn't, specify.
> > > Thanks,
> > > 
> > > Alex
> > 
> > OK but I should be able to query value of each variable and figure
> > out what it means.
> > 
> > We can build a tri-state property type if desired:
> > force on/force off/auto.
> > Just let's not code up random magic values.
> > 0 and 1 for on/off is ugly enough.
> > 
> > 
> > > > 
> > > > >      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
> > > > >                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
> > > > >      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
> > > > > -- 
> > > > > 1.8.3.1
> > > 
> > > 
> 
> 

Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2

[Bandan Das]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Sun, Feb 23, 2014 at 07:18:07AM -0700, Alex Williamson wrote:
>> On Sun, 2014-02-23 at 08:32 +0200, Michael S. Tsirkin wrote:
>> > On Sat, Feb 22, 2014 at 04:28:26PM -0700, Alex Williamson wrote:
>> > > On Thu, 2014-02-20 at 10:12 +0200, Michael S. Tsirkin wrote:
>> > > > On Wed, Feb 19, 2014 at 03:20:54PM -0500, Bandan Das wrote:
>> > > > > The following patch depends on the value of rom_bar to
>> > > > > determine rom blacklist behavior. Existing code shouldn't
>> > > > > be affected by changing the default value of rom_bar since
>> > > > > all relevant decisions only rely on whether rom_bar is zero
>> > > > > or non-zero.
>> > > > > 
>> > > > > Signed-off-by: Bandan Das <bsd@redhat.com>
>> > > > > ---
>> > > > >  hw/pci/pci.c | 7 ++++++-
>> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
>> > > > > 
>> > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> > > > > index 4e0701d..12c3e27 100644
>> > > > > --- a/hw/pci/pci.c
>> > > > > +++ b/hw/pci/pci.c
>> > > > > @@ -53,7 +53,12 @@ static void pci_bus_finalize(Object *obj);
>> > > > >  static Property pci_props[] = {
>> > > > >      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
>> > > > >      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
>> > > > > -    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
>> > > > > +    /*
>> > > > > +     * 0 = disable
>> > > > > +     * 1 = user requested on, force loading even if rom blacklisted
>> > > > > +     * 2 = enabled but disables loading of blacklisted roms (default)
>> > > > > +     */
>> > > > > +    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),
>> > > > 
>> > > > How do users figure out this interface?
>> > > > Read code?
>> > > > Could we add a bit property rombarforce=on/off instead?
>> > > > Seems better.
>> > > > 
>> > > > Maybe we should teach bool type visitors
>> > > > about 0 and 1 being legal values
>> > > > (call out to int visitor, then check value 0 or 1),
>> > > > then rombar can be changed to bit property too.
>> > > > 
>> > > > Also, this will need QMP support right?
>> > > > IIUC rombar is not exposed in QMP ATM.
>> > > 
>> > > rombarforce seems very redundant for a user interface; rombar=1 "expose
>> > > the ROM BAR of the device", rombarforce=1 "yes, really expose the ROM
>> > > BAR of the device".
>> > 
>> > Not really.
>> > In this design, rombarforce=yes means "expose ROM BAR of the device",
>> > rombar should not be exposed to users - it's a compatibility property
>> > used for cross-version migration.
>> > 
>> > > Even if force implies rombar,
>> > > I don't think that's
>> > > very easy to code in libvirt.
>> > 
>> > Libvirt doesn't touch rombar AFAIK.
>> 
>> It does
>> 
>> http://libvirt.org/formatdomain.html#elementsNICSROM
>> 
>> <rom bar='off'>
>
>
> Got it, thanks. So if you think the right thing
> to do for users it to interpret rom=on as
> meaning "force" then just do that.
> Use some new hidden field for machine compatibility.

Even if we use another variable for machine compatibility, 
we can't assume rom=on means force.

"force" is that special case where even if the rom is blacklisted,
loading is attempted. (Please see 2/2 v2] vfio: blacklist loading of unstable roms)
For now, the usecase is to get around when there is a new rom to test.

A tristate property seems better, with an approach that addresses your concerns 
about random values that could confuse users.

Bandan

>
>> > >  I think we really just want to detect
>> > > unspecified versus specified, which probably means setting the default
>> > > value to something the user can't, or at least wouldn't, specify.
>> > > Thanks,
>> > > 
>> > > Alex
>> > 
>> > OK but I should be able to query value of each variable and figure
>> > out what it means.
>> > 
>> > We can build a tri-state property type if desired:
>> > force on/force off/auto.
>> > Just let's not code up random magic values.
>> > 0 and 1 for on/off is ugly enough.
>> > 
>> > 
>> > > > 
>> > > > >      DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present,
>> > > > >                      QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false),
>> > > > >      DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
>> > > > > -- 
>> > > > > 1.8.3.1
>> > > 
>> > > 
>> 
>> 

Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2

[Alex Williamson]

On Sun, 2014-02-23 at 20:32 -0500, Bandan Das wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Sun, Feb 23, 2014 at 07:18:07AM -0700, Alex Williamson wrote:
> >> On Sun, 2014-02-23 at 08:32 +0200, Michael S. Tsirkin wrote:
> >> > On Sat, Feb 22, 2014 at 04:28:26PM -0700, Alex Williamson wrote:
> >> > > On Thu, 2014-02-20 at 10:12 +0200, Michael S. Tsirkin wrote:
> >> > > > On Wed, Feb 19, 2014 at 03:20:54PM -0500, Bandan Das wrote:
> >> > > > > The following patch depends on the value of rom_bar to
> >> > > > > determine rom blacklist behavior. Existing code shouldn't
> >> > > > > be affected by changing the default value of rom_bar since
> >> > > > > all relevant decisions only rely on whether rom_bar is zero
> >> > > > > or non-zero.
> >> > > > > 
> >> > > > > Signed-off-by: Bandan Das <bsd@redhat.com>
> >> > > > > ---
> >> > > > >  hw/pci/pci.c | 7 ++++++-
> >> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> >> > > > > 
> >> > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> > > > > index 4e0701d..12c3e27 100644
> >> > > > > --- a/hw/pci/pci.c
> >> > > > > +++ b/hw/pci/pci.c
> >> > > > > @@ -53,7 +53,12 @@ static void pci_bus_finalize(Object *obj);
> >> > > > >  static Property pci_props[] = {
> >> > > > >      DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> >> > > > >      DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> >> > > > > -    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 1),
> >> > > > > +    /*
> >> > > > > +     * 0 = disable
> >> > > > > +     * 1 = user requested on, force loading even if rom blacklisted
> >> > > > > +     * 2 = enabled but disables loading of blacklisted roms (default)
> >> > > > > +     */
> >> > > > > +    DEFINE_PROP_UINT32("rombar",  PCIDevice, rom_bar, 2),
> >> > > > 
> >> > > > How do users figure out this interface?
> >> > > > Read code?
> >> > > > Could we add a bit property rombarforce=on/off instead?
> >> > > > Seems better.
> >> > > > 
> >> > > > Maybe we should teach bool type visitors
> >> > > > about 0 and 1 being legal values
> >> > > > (call out to int visitor, then check value 0 or 1),
> >> > > > then rombar can be changed to bit property too.
> >> > > > 
> >> > > > Also, this will need QMP support right?
> >> > > > IIUC rombar is not exposed in QMP ATM.
> >> > > 
> >> > > rombarforce seems very redundant for a user interface; rombar=1 "expose
> >> > > the ROM BAR of the device", rombarforce=1 "yes, really expose the ROM
> >> > > BAR of the device".
> >> > 
> >> > Not really.
> >> > In this design, rombarforce=yes means "expose ROM BAR of the device",
> >> > rombar should not be exposed to users - it's a compatibility property
> >> > used for cross-version migration.
> >> > 
> >> > > Even if force implies rombar,
> >> > > I don't think that's
> >> > > very easy to code in libvirt.
> >> > 
> >> > Libvirt doesn't touch rombar AFAIK.
> >> 
> >> It does
> >> 
> >> http://libvirt.org/formatdomain.html#elementsNICSROM
> >> 
> >> <rom bar='off'>
> >
> >
> > Got it, thanks. So if you think the right thing
> > to do for users it to interpret rom=on as
> > meaning "force" then just do that.
> > Use some new hidden field for machine compatibility.
> 
> Even if we use another variable for machine compatibility, 
> we can't assume rom=on means force.
> 
> "force" is that special case where even if the rom is blacklisted,
> loading is attempted. (Please see 2/2 v2] vfio: blacklist loading of unstable roms)
> For now, the usecase is to get around when there is a new rom to test.
> 
> A tristate property seems better, with an approach that addresses your concerns 
> about random values that could confuse users.

I suspect there are ways to parse the opts for a given device to find
whether rombar was specified so we don't need to create a magic "unset"
value.  We just need to dig through the obfuscation of the opts code.