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

[Bandan Das]

"Michael S. Tsirkin" <address@hidden> writes:

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

Fine with me, but aren't rombarforce and rombar a little 
bit similar in thier functions ? 

1. rombarforce=on automatically implies rombar=1 (force)
2. rombarforce=off(default), rombar=1 (on but don't force loading
if blacklisted)
3. rombarforce=on, rombar=0 (do nothing)
4. rombarforce=off, rombar=0 (again nothing)

So why not just a tristate rombar=on/off/force

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

Will qmp hotplug care about option rom ? If not, atleast the reboot
will, so I think this is needed.
 
>>      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