On Wed, 3 Jul 2024 13:00:21 +0200 (CEST) BALATON Zoltan <bala...@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.od...@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
