| Message ID | 1505942471-11260-1-git-send-email-zuban32s@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/pci-bridge/pcie_pci_bridge: properly handle MSI unavailability case | expand |
Hi Aleksandr, On 21/09/2017 0:21, Aleksandr Bezzubikov wrote: > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> > --- > 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. > 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? 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(), > }; > >
2017-09-21 13:16 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: > Hi Aleksandr, > > On 21/09/2017 0:21, Aleksandr Bezzubikov wrote: >> >> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> >> --- >> 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? > > >> 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. > > 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(), >> }; >> > >
On 21/09/2017 21:12, Aleksandr Bezzubikov wrote: > 2017-09-21 13:16 GMT+03:00 Marcel Apfelbaum <marcel@redhat.com>: >> Hi Aleksandr, >> >> On 21/09/2017 0:21, Aleksandr Bezzubikov wrote: >>> >>> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> >>> --- >>> 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 <marcel@redhat.com> 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(), >>> }; >>> >> >> > > >
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); + } 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(), };
Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com> --- hw/pci-bridge/pcie_pci_bridge.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)