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

[BALATON Zoltan <balaton@eik.bme.hu>]

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.

Regards,
BALATON Zoltan