On Wed, Jul 03, 2024 at 01:00:21PM +0200, BALATON Zoltan 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.
I mean, the code is open, why do you keep guessing?
No, these reuse the bool parsing logic:
static void prop_get_bit(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
{
Property *prop = opaque;
uint32_t *p = object_field_prop_ptr(obj, prop);
bool value = (*p & qdev_get_prop_mask(prop)) != 0;
visit_type_bool(v, name, &value, errp);
}
and that never accepted 0 or 1:
bool qapi_bool_parse(const char *name, const char *value, bool *obj, Error
**errp)
{
if (g_str_equal(value, "on") ||
g_str_equal(value, "yes") ||
g_str_equal(value, "true") ||
g_str_equal(value, "y")) {
*obj = true;
return true;
}
if (g_str_equal(value, "off") ||
g_str_equal(value, "no") ||
g_str_equal(value, "false") ||
g_str_equal(value, "n")) {
*obj = false;
return true;
}
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
"'on' or 'off'");
return false;
}
> Maybe similar aliases could be added to on/off/auto?
Could be, but even then switching to that would mean that user sets 1
but query returns "on". Might or might not surprise some users.
Adding true/false yes/no y/n aliases to on/off/auto might make sense
though, for consistency. Donnu if QAPI guys will agree, though,
and not directly related to this patchset.
One other idea is to add a generic way to detect that a property is set
by user. This requirement comes up, once in a while.
> 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
I agree here, but it's been here for a long time.
--
MST
