[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2 |
Date: |
Sun, 23 Feb 2014 19:56:54 -0700 |
On Sun, 2014-02-23 at 20:32 -0500, Bandan Das wrote:
> "Michael S. Tsirkin" <address@hidden> 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 <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.
> >>
> >> 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.
- Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2, (continued)
- Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2, Alex Williamson, 2014/02/19
- Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2, Michael S. Tsirkin, 2014/02/20
- Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2, Bandan Das, 2014/02/20
- Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2, Alex Williamson, 2014/02/22
- Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2, Michael S. Tsirkin, 2014/02/23
- Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2, Alex Williamson, 2014/02/23
- Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2, Michael S. Tsirkin, 2014/02/23
- Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2, Bandan Das, 2014/02/23
- Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2,
Alex Williamson <=
[Qemu-devel] [PATCH 2/2 v2] vfio: blacklist loading of unstable roms, Bandan Das, 2014/02/19