From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52013) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b1w3a-0007hH-TZ for qemu-devel@nongnu.org; Sun, 15 May 2016 09:26:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b1w3X-0003nQ-Iu for qemu-devel@nongnu.org; Sun, 15 May 2016 09:26:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41136) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b1w3X-0003nB-BO for qemu-devel@nongnu.org; Sun, 15 May 2016 09:26:03 -0400 References: <1462508442-9407-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1462508442-9407-10-git-send-email-caoj.fnst@cn.fujitsu.com> From: Marcel Apfelbaum Message-ID: <573878E7.7070807@redhat.com> Date: Sun, 15 May 2016 16:25:59 +0300 MIME-Version: 1.0 In-Reply-To: <1462508442-9407-10-git-send-email-caoj.fnst@cn.fujitsu.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 09/11] pci bridge dev: change msi property type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin , qemu-devel@nongnu.org Cc: "Michael S. Tsirkin" , Markus Armbruster On 05/06/2016 07:20 AM, Cao jin wrote: > From bit to enum OnOffAuto. > > cc: Michael S. Tsirkin > cc: Markus Armbruster > cc: Marcel Apfelbaum > Signed-off-by: Cao jin > --- > > Actually, I am not quite sure this device need this change, RFC. > Well, it already has the 'msi' property, so we may want to make it standard 'OnOffAuto'. One problem I can see is the change of semantics. Until now msi=on means 'auto'. From now on it means 'force msi=on', fail otherwise. If I got this right, old machines having msi=on will failed to start on platforms with msibroken=true, right? Maybe we should preserve the semantics for old machines? (this patch does not actually affect the semantics, but patch 11/11 should, otherwise why change it to OnOffAuto, right? ) Thanks, Marcel > hw/pci-bridge/pci_bridge_dev.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c > index 32f4daa..9e31f0e 100644 > --- a/hw/pci-bridge/pci_bridge_dev.c > +++ b/hw/pci-bridge/pci_bridge_dev.c > @@ -41,9 +41,10 @@ struct PCIBridgeDev { > > MemoryRegion bar; > uint8_t chassis_nr; > -#define PCI_BRIDGE_DEV_F_MSI_REQ 0 > -#define PCI_BRIDGE_DEV_F_SHPC_REQ 1 > +#define PCI_BRIDGE_DEV_F_SHPC_REQ 0 > uint32_t flags; > + > + OnOffAuto msi; > }; > typedef struct PCIBridgeDev PCIBridgeDev; > > @@ -65,7 +66,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) > } > } else { > /* MSI is not applicable without SHPC */ > - bridge_dev->flags &= ~(1 << PCI_BRIDGE_DEV_F_MSI_REQ); > + bridge_dev->msi = ON_OFF_AUTO_OFF; > } > > err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0); > @@ -73,7 +74,8 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) > goto slotid_error; > } > > - if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) && > + if ((bridge_dev->msi == ON_OFF_AUTO_AUTO || > + bridge_dev->msi == ON_OFF_AUTO_ON) && > msi_nonbroken) { > err = msi_init(dev, 0, 1, true, true); > if (err < 0) { > @@ -146,8 +148,8 @@ static Property pci_bridge_dev_properties[] = { > /* Note: 0 is not a legal chassis number. */ > DEFINE_PROP_UINT8(PCI_BRIDGE_DEV_PROP_CHASSIS_NR, PCIBridgeDev, chassis_nr, > 0), > - DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, flags, > - PCI_BRIDGE_DEV_F_MSI_REQ, true), > + DEFINE_PROP_ON_OFF_AUTO(PCI_BRIDGE_DEV_PROP_MSI, PCIBridgeDev, msi, > + ON_OFF_AUTO_AUTO), > DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags, > PCI_BRIDGE_DEV_F_SHPC_REQ, true), > DEFINE_PROP_END_OF_LIST(), >