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

[Marcel Apfelbaum]

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