Re: [Qemu-devel] [PATCH] hw/pci-bridge/pcie_pci_bridge: properly handle MSI unavailability case

[Marcel Apfelbaum]

On 21/09/2017 21:12, Aleksandr Bezzubikov wrote:
> 2017-09-21 13:16 GMT+03:00 Marcel Apfelbaum <address@hidden>:
> > Hi Aleksandr,
> >
> > On 21/09/2017 0:21, Aleksandr Bezzubikov wrote:
> > > Signed-off-by: Aleksandr Bezzubikov <address@hidden>
> > > ---
> > >    hw/pci-bridge/pcie_pci_bridge.c | 24 ++++++++++++++++++------
> > >    1 file changed, 18 insertions(+), 6 deletions(-)
> >
> > Please add Eduardo's Reported-by tag and the
> > actual failure in the commit message.
> > You might even explain in the commit message that you
> > moved msi prop to ON_OFF_AUTO_AUTO.
>
> Should I send a new one, or resend, or just reply to this post?

A new version would be preferred.

> > > diff --git a/hw/pci-bridge/pcie_pci_bridge.c
> > > b/hw/pci-bridge/pcie_pci_bridge.c
> > > index 9aa5cc3..da562fe 100644
> > > --- a/hw/pci-bridge/pcie_pci_bridge.c
> > > +++ b/hw/pci-bridge/pcie_pci_bridge.c
> > > @@ -65,10 +65,18 @@ static void pcie_pci_bridge_realize(PCIDevice *d,
> > > Error **errp)
> > >            goto aer_error;
> > >        }
> > >    +    Error *local_err = NULL;
> > >        if (pcie_br->msi != ON_OFF_AUTO_OFF) {
> > > -        rc = msi_init(d, 0, 1, true, true, errp);
> > > +        rc = msi_init(d, 0, 1, true, true, &local_err);
> > >            if (rc < 0) {
> > > -            goto msi_error;
> > > +            assert(rc == -ENOTSUP);
> > > +            if (pcie_br->msi != ON_OFF_AUTO_ON) {
> > > +                error_free(local_err);
> >
> >
> > In that case it will fallback to legacy INTx, right?
>
> Exactly. Maybe I should add this to the commit message.

With the above comment:

Reviewed-by: Marcel Apfelbaum <address@hidden>

Thanks,
Marcel

> > Thanks,
> > Marcel
> >
> >
> > > +            } else {
> > > +                /* failed to satisfy user's explicit request for MSI */
> > > +                error_propagate(errp, local_err);
> > > +                goto msi_error;
> > > +            }
> > >            }
> > >        }
> > >        pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
> > > @@ -81,7 +89,7 @@ aer_error:
> > >    pm_error:
> > >        pcie_cap_exit(d);
> > >    cap_error:
> > > -    shpc_free(d);
> > > +    shpc_cleanup(d, &pcie_br->shpc_bar);
> > >    error:
> > >        pci_bridge_exitfn(d);
> > >    }
> > > @@ -98,7 +106,9 @@ static void pcie_pci_bridge_reset(DeviceState *qdev)
> > >    {
> > >        PCIDevice *d = PCI_DEVICE(qdev);
> > >        pci_bridge_reset(qdev);
> > > -    msi_reset(d);
> > > +    if (msi_present(d)) {
> > > +        msi_reset(d);
> > > +    }
> > >        shpc_reset(d);
> > >    }
> > >    @@ -106,12 +116,14 @@ static void pcie_pci_bridge_write_config(PCIDevice
> > > *d,
> > >            uint32_t address, uint32_t val, int len)
> > >    {
> > >        pci_bridge_write_config(d, address, val, len);
> > > -    msi_write_config(d, address, val, len);
> > > +    if (msi_present(d)) {
> > > +        msi_write_config(d, address, val, len);
> > > +    }
> > >        shpc_cap_write_config(d, address, val, len);
> > >    }
> > >      static Property pcie_pci_bridge_dev_properties[] = {
> > > -        DEFINE_PROP_ON_OFF_AUTO("msi", PCIEPCIBridge, msi,
> > > ON_OFF_AUTO_ON),
> > > +        DEFINE_PROP_ON_OFF_AUTO("msi", PCIEPCIBridge, msi,
> > > ON_OFF_AUTO_AUTO),
> > >            DEFINE_PROP_END_OF_LIST(),
> > >    };