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

[Bandan Das <bsd@redhat.com>]

"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
>> > > 
>> > > 
>> 
>>