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

[Marcel Apfelbaum]

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