From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53804) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WHZsk-0003zC-H3 for qemu-devel@nongnu.org; Sun, 23 Feb 2014 09:18:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WHZsf-0001FI-KU for qemu-devel@nongnu.org; Sun, 23 Feb 2014 09:18:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:20039) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WHZsf-0001F7-Bx for qemu-devel@nongnu.org; Sun, 23 Feb 2014 09:18:09 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s1NEI8ig013511 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 23 Feb 2014 09:18:08 -0500 Message-ID: <1393165087.9111.98.camel@ul30vt.home> From: Alex Williamson Date: Sun, 23 Feb 2014 07:18:07 -0700 In-Reply-To: <20140223063251.GA10908@redhat.com> 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> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit 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: Bandan Das , qemu-devel@nongnu.org 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 > > 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 > > > >