[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init()

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH v4 5/5] Add param Error ** for msi_init()
Date: Sun, 10 Apr 2016 11:20:07 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

On 04/09/2016 03:19 PM, Cao jin wrote:

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 
      /* 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:


   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[*].

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,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]