| Message ID | 1392841255-22741-2-git-send-email-bsd@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 2014-02-19 at 15:20 -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), > DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present, > QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false), > DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present, A slightly more satisfying option might be to define rom_bar as int32_t with default of -1. I don't know if that would break libvirt though. I'll let MST weigh in. Thanks, Alex
Alex Williamson <alex.williamson@redhat.com> writes: > On Wed, 2014-02-19 at 15:20 -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), >> DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present, >> QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false), >> DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present, > > A slightly more satisfying option might be to define rom_bar as int32_t > with default of -1. I don't know if that would break libvirt though. > I'll let MST weigh in. Thanks, IMO, since the default is "enabled", having a value of -1 for an enabled option could be a bit confusing to some. Bandan > Alex
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. > 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
On Wed, Feb 19, 2014 at 01:36:45PM -0700, Alex Williamson wrote: > On Wed, 2014-02-19 at 15:20 -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), > > DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present, > > QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false), > > DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present, > > A slightly more satisfying option might be to define rom_bar as int32_t > with default of -1. I don't know if that would break libvirt though. > I'll let MST weigh in. Thanks, > > Alex I don't see rombar in json schema at all. I think it was designed as an internal flag for compatibility with legacy machine types. As such it's likely not a good interface for users.
"Michael S. Tsirkin" <mst@redhat.com> 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 <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. 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
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". Even if force implies rombar, I don't think that's very easy to code in libvirt. 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 > > > 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
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. > 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 > >
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'> > > 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 > > > >
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. > > > 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 > > > > > > > >
"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 >> > > >> > > >> >>
On Sun, 2014-02-23 at 20:32 -0500, Bandan Das wrote: > "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. I suspect there are ways to parse the opts for a given device to find whether rombar was specified so we don't need to create a magic "unset" value. We just need to dig through the obfuscation of the opts code.
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), DEFINE_PROP_BIT("multifunction", PCIDevice, cap_present, QEMU_PCI_CAP_MULTIFUNCTION_BITNR, false), DEFINE_PROP_BIT("command_serr_enable", PCIDevice, cap_present,
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(-)