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

[Marcel Apfelbaum <marcel@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