Re: [PATCH v10 11/12] hw/pci: Convert rom_bar into OnOffAuto

[Alex Williamson]

On Wed, 3 Jul 2024 13:00:21 +0200 (CEST)
BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Wed, 3 Jul 2024, Michael S. Tsirkin wrote:
> > On Wed, Jul 03, 2024 at 04:15:23AM +0200, BALATON Zoltan wrote:  
> >> On Tue, 2 Jul 2024, Michael S. Tsirkin wrote:  
> >>> On Thu, Jun 27, 2024 at 03:08:00PM +0900, Akihiko Odaki wrote:  
> >>>> rom_bar is tristate but was defined as uint32_t so convert it into
> >>>> OnOffAuto.
> >>>>
> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>  
> >>>
> >>> Commit log should explain why this is an improvement,
> >>> not just what's done.
> >>>
> >>>  
> >>>> diff --git a/docs/igd-assign.txt b/docs/igd-assign.txt
> >>>> index e17bb50789ad..35c6c8e28493 100644
> >>>> --- a/docs/igd-assign.txt
> >>>> +++ b/docs/igd-assign.txt
> >>>> @@ -35,7 +35,7 @@ IGD has two different modes for assignment using 
> >>>> vfio-pci:
> >>>>        ISA/LPC bridge device (vfio-pci-igd-lpc-bridge) on the root bus at
> >>>>        PCI address 1f.0.
> >>>>      * The IGD device must have a VGA ROM, either provided via the 
> >>>> romfile
> >>>> -      option or loaded automatically through vfio (standard).  rombar=0
> >>>> +      option or loaded automatically through vfio (standard).  
> >>>> rombar=off
> >>>>        will disable legacy mode support.
> >>>>      * Hotplug of the IGD device is not supported.
> >>>>      * The IGD device must be a SandyBridge or newer model device.  
> >>>
> >>> ...
> >>>  
> >>>> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> >>>> index 39dae72497e0..0e920ed0691a 100644
> >>>> --- a/hw/vfio/pci-quirks.c
> >>>> +++ b/hw/vfio/pci-quirks.c
> >>>> @@ -33,7 +33,7 @@
> >>>>   * execution as noticed with the BCM 57810 card for lack of a
> >>>>   * more better way to handle such issues.
> >>>>   * The  user can still override by specifying a romfile or
> >>>> - * rombar=1.
> >>>> + * rombar=on.
> >>>>   * Please see https://bugs.launchpad.net/qemu/+bug/1284874
> >>>>   * for an analysis of the 57810 card hang. When adding
> >>>>   * a new vendor id/device id combination below, please also add  
> >>>
> >>>
> >>> So we are apparently breaking a bunch of users who followed
> >>> documentation to the dot. Why is this a good idea?  
> >>
> >> On/off is clearer than 1/0. But isn't 1/0 a synonym for on/off so previous
> >> command lines would still work?
> >>
> >> Regards,
> >> BALATON Zoltan  
> >
> > I see nothing in code that would make it so:
> >
> >
> > const QEnumLookup OnOffAuto_lookup = {
> >    .array = (const char *const[]) {
> >        [ON_OFF_AUTO_AUTO] = "auto",
> >        [ON_OFF_AUTO_ON] = "on",
> >        [ON_OFF_AUTO_OFF] = "off",
> >    },
> >    .size = ON_OFF_AUTO__MAX
> > };
> >
> > I also tried with an existing property:
> >
> > $ ./qemu-system-x86_64 -device intel-hda,msi=0
> > qemu-system-x86_64: -device intel-hda,msi=0: Parameter 'msi' does not 
> > accept value '0'  
> 
> Then it was probably bit properties that also accept 0/1, on/off, 
> true/false. Maybe similar aliases could be added to on/off/auto?
> 
> In any case when I first saw rombar I thought it would set the BAR of the 
> ROM so wondered why it's 1 and not 5 or 6 or an offset. So on/off is 
> clearer in this case.

There's only one PCI spec defined offset for the ROM BAR.  Yes, the
option could be more clear but relocating the ROM to a different
regular BAR offset is invalid.  Thanks,

Alex