From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45613) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b4mkz-0004zQ-26 for qemu-devel@nongnu.org; Mon, 23 May 2016 06:06:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b4mku-0006hN-Oi for qemu-devel@nongnu.org; Mon, 23 May 2016 06:06:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39566) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b4mku-0006h8-GL for qemu-devel@nongnu.org; Mon, 23 May 2016 06:06:36 -0400 References: <1462508442-9407-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1462508442-9407-12-git-send-email-caoj.fnst@cn.fujitsu.com> <57387C96.2090103@redhat.com> <573AEDA9.5080507@cn.fujitsu.com> From: Marcel Apfelbaum Message-ID: <5742D61F.2090602@redhat.com> Date: Mon, 23 May 2016 13:06:23 +0300 MIME-Version: 1.0 In-Reply-To: <573AEDA9.5080507@cn.fujitsu.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 11/11] pci: Convert msi_init() to Error and fix callers to check it List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin , qemu-devel@nongnu.org Cc: Gerd Hoffmann , John Snow , Dmitry Fleytman , Jason Wang , "Michael S. Tsirkin" , Hannes Reinecke , Paolo Bonzini , Alex Williamson , Markus Armbruster On 05/17/2016 01:08 PM, Cao jin wrote: > > > On 05/15/2016 09:41 PM, Marcel Apfelbaum wrote: > >> >>> >>> diff --git a/hw/pci-bridge/pci_bridge_dev.c >>> b/hw/pci-bridge/pci_bridge_dev.c >>> index 9e31f0e..af71c98 100644 >>> --- a/hw/pci-bridge/pci_bridge_dev.c >>> +++ b/hw/pci-bridge/pci_bridge_dev.c >>> @@ -53,6 +53,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) >>> PCIBridge *br = PCI_BRIDGE(dev); >>> PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); >>> int err; >>> + Error *local_err = NULL; >>> >>> pci_bridge_initfn(dev, TYPE_PCI_BUS); >>> >>> @@ -74,11 +75,11 @@ static int pci_bridge_dev_initfn(PCIDevice *dev) >>> goto slotid_error; >>> } >>> >>> - if ((bridge_dev->msi == ON_OFF_AUTO_AUTO || >>> - bridge_dev->msi == ON_OFF_AUTO_ON) && >>> + if (bridge_dev->msi != ON_OFF_AUTO_OFF && >> >> So we made the msi property OnOffAuto, but we don't make a difference >> between ON and Auto? >> > > That is why I am not quite sure about this device, msi has a relation with shpc. > > From its previous behaviour, it can be seen that it don`t treat 'on' as 'auto'.(I am not sure why it is different with others) > > Its previous behaviour treat msi_init`s failure as fatal, and lead to device creation fail. According to Markus`s comments(if I understand him right), this device has no non-msi variants. Actually it has. The bridge needs msi for the SHPC controller, as you said. If you look into shpc_interrupt_update function (hw/pci/shpc.c) you can see it can fall back to legacy interrupts. The only complication here is that msi is needed only if shpc is present. So maybe having msi=on/off/auto is OK. msi=auto => if shpc not present or msi broken results in msi = off, else msi = on msi=on => fails if shpc present and msi broken msi=off => use legacy interrupts if shpc is present Basically the msi flag has no meaning if shpc not present. Thanks, Marcel I think my > patch follows the previous behaviour. > > And also according to function comments: > /* MSI is not applicable without SHPC */ > which means device either has both, or neither(if I understand it right), so that is why I don`t make a difference > >>> msi_nonbroken) { >>> - err = msi_init(dev, 0, 1, true, true); >>> + err = msi_init(dev, 0, 1, true, true, &local_err); >>> if (err < 0) { >>> + error_report_err(local_err); >>> goto msi_error; >>> } >>> } > >