From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37401) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WHkPb-0003RM-0F for qemu-devel@nongnu.org; Sun, 23 Feb 2014 20:32:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WHkPW-0000pQ-04 for qemu-devel@nongnu.org; Sun, 23 Feb 2014 20:32:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:25849) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WHkPV-0000ol-OA for qemu-devel@nongnu.org; Sun, 23 Feb 2014 20:32:45 -0500 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s1O1WgOT006890 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 23 Feb 2014 20:32:43 -0500 From: Bandan Das References: <1392841255-22741-1-git-send-email-bsd@redhat.com> <1392841255-22741-2-git-send-email-bsd@redhat.com> <20140220081235.GA28812@redhat.com> <1393111706.9111.97.camel@ul30vt.home> <20140223063251.GA10908@redhat.com> <1393165087.9111.98.camel@ul30vt.home> <20140224003114.GA7747@redhat.com> Date: Sun, 23 Feb 2014 20:32:34 -0500 In-Reply-To: <20140224003114.GA7747@redhat.com> (Michael S. Tsirkin's message of "Mon, 24 Feb 2014 02:31:14 +0200") Message-ID: MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 1/2 v2] pci: change default value of rom_bar to 2 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Alex Williamson , qemu-devel@nongnu.org "Michael S. Tsirkin" 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 >> > > > > --- >> > > > > 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 >> >> > > > 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 >> > > >> > > >> >>