From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37502) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1apAbV-0006mX-CS for qemu-devel@nongnu.org; Sun, 10 Apr 2016 04:20:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1apAbS-0000md-5Y for qemu-devel@nongnu.org; Sun, 10 Apr 2016 04:20:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52467) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1apAbR-0000mZ-UH for qemu-devel@nongnu.org; Sun, 10 Apr 2016 04:20:18 -0400 References: <1459855602-16727-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1459855602-16727-6-git-send-email-caoj.fnst@cn.fujitsu.com> <87pou0sd4u.fsf@dusky.pond.sub.org> <5708F33B.2040000@cn.fujitsu.com> From: Marcel Apfelbaum Message-ID: <570A0CB7.1040509@redhat.com> Date: Sun, 10 Apr 2016 11:20:07 +0300 MIME-Version: 1.0 In-Reply-To: <5708F33B.2040000@cn.fujitsu.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin , Markus Armbruster Cc: qemu-devel@nongnu.org, mst@redhat.com, jasowang@redhat.com, alex.williamson@redhat.com, hare@suse.de, dmitry@daynix.com, pbonzini@redhat.com, jsnow@redhat.com, kraxel@redhat.com On 04/09/2016 03:19 PM, Cao jin wrote: > Hi > > On 04/08/2016 04:44 PM, Markus Armbruster wrote: > >>> diff --git a/hw/ide/ich.c b/hw/ide/ich.c >>> index 0a13334..db4fdb5 100644 >>> --- a/hw/ide/ich.c >>> +++ b/hw/ide/ich.c >>> @@ -146,7 +146,7 @@ static void pci_ich9_ahci_realize(PCIDevice *dev, Error **errp) >>> /* Although the AHCI 1.3 specification states that the first capability >>> * should be PMCAP, the Intel ICH9 data sheet specifies that the ICH9 >>> * AHCI device puts the MSI capability first, pointing to 0x80. */ >>> - msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false); >>> + msi_init(dev, ICH9_MSI_CAP_OFFSET, 1, true, false, errp); >> >> Sure there's nothing to undo on error? Instead of undoing, you may want >> to move msi_init() before the stuff that needs undoing. >> > > ich9-ahci is a on-board device of Q35, like cover-letter says: when it fail, qemu will exit. So, is it necessary to undo on error? > > maybe you saw, I did move msi_init() for some other devices. > >>> diff --git a/hw/pci/msi.c b/hw/pci/msi.c >> >> msi_init() has three failure modes: >> >> * -ENOTSUP >> >> Board's MSI emulation is not known to work: !msi_nonbroken. >> >> This is not necessarily an error. >> >> It is when the device model requires MSI. >> >> It isnt' when a non-MSI variant of the device model exists. Then >> caller should silently switch to the non-MSI variant[*]. >> > Hi, I'll let Markus to continue the review, it brings very valuable information, I will only try to answer the questions below. > Several questions on this topic: > 1. How to confirm whether a device model has non-MSI variant? AFAICT, it is these who have msi property. > MSI is required for PCI Express devices, optional for PCI devices. Even if a PCI device supports MSI, it is strongly advised to support legacy INTx for backward compatibility. Bottom line, as far as I know, almost all PCI devices support legacy interrupts. (an exception is the ivshmem device that requires MSI) > 2. For those have non-MSI variant devices(have msi property), as I see in the code, they all have it on by default, So we won`t know it is user order, or user don`t set it at all. > I didn't quite understand the sentence, but some devices have a "use_msi" property that can be set by the user. If no such property exists, we can assume the user "prefers" the msi version. > If user don`t know msi and don`t set it on, I think it is acceptable to create the non-msi variant for user silently. But if it is user order, like you said, it is an error. > I am not sure about this. At least a warning should be given IMHO. > So, how about: inform user to swich msi off and try again when encounter -ENOTSUP, no matter it is user order, or user doesn`t set it at all? > Not all devices have an "msi" switch. If the board has msi broken and the devices supports legacy interrupts, its OK to continue without MSI. > Actually in this v4, I do checked whether device has a msi property, like cover-letter said: > > 3. most devices have msi/msix(except vmxnet3 & pvscsi) property as a switch, if it has and is switched on, then msi_init() failure should results in return directly. So in this version, mptsas > is updated > I don't see a "msi" properties on PCIDevice class or VirtioPCIClass, are you sure we have an msi switch for most of the PCI devices? Thanks for looking into this, Marcel