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

[Michael S. Tsirkin]

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 <address@hidden>
> > > ---
> > >  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.

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