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

[Marcel Apfelbaum <marcel@redhat.com>]

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