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

[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.


> 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(),
>   };
>   
>