Re: [Qemu-devel] [PATCH v5 11/11] pci: Convert msi_init() to Error and fix callers to check it

[Marcel Apfelbaum <marcel@redhat.com>]

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;
>>>           }
>>>       }
>
>