Re: [Qemu-devel] [PATCH v2 2/3] hw/pci: add teardown function for PCI resource reserve capability

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

Hi Jing,

On 08/20/2018 05:58 AM, Liu, Jing2 wrote:
> Hi Marcel,
>
> On 8/18/2018 12:10 AM, Marcel Apfelbaum wrote:
>> Hi Jing,
>>
>> On 08/16/2018 12:28 PM, Jing Liu wrote:
>>> Clean up the PCI config space of resource reserve capability.
>>>
>>> Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>
>>> ---
>>>   hw/pci/pci_bridge.c         | 9 +++++++++
>>>   include/hw/pci/pci_bridge.h | 1 +
>>>   2 files changed, 10 insertions(+)
>>>
>>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>>> index 15b055e..dbcee90 100644
>>> --- a/hw/pci/pci_bridge.c
>>> +++ b/hw/pci/pci_bridge.c
>>> @@ -465,6 +465,15 @@ int pci_bridge_qemu_reserve_cap_init(PCIDevice 
>>> *dev, int cap_offset,
>>>       return 0;
>>>   }
>>> +void pci_bridge_qemu_reserve_cap_uninit(PCIDevice *dev)
>>> +{
>>> +    uint8_t pos = pci_find_capability(dev, PCI_CAP_ID_VNDR);
>>> +
>>> +    pci_del_capability(dev, PCI_CAP_ID_VNDR, 
>>> sizeof(PCIBridgeQemuCap));
>>
>> I think that you only need to call pci_del_capability,
>>
>>> +    memset(dev->config + pos + PCI_CAP_FLAGS, 0,
>>> +           sizeof(PCIBridgeQemuCap) - PCI_CAP_FLAGS);
>>> +}
>>
>> ... no need for the above line. The reason is pci_del_capability
>> will "unlink" the capability, and even if the data remains in
>> the configuration space array, it will not be used.
>>
> I think I got it: pci_del_capability "unlink" by set the tag
> pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
> so that pdev->config will not be used, right?

If is the latest capability in the list, yes.
Otherwise it will simply link 'prev' with 'next' using config array offsets.

Thanks,
Marcel

>
>> Do you agree? If yes, just call pci_del_capability and you don't need
>> this patch.
>>
> Yup, I agree with you. And let me remove this patch in next version.
>
> Thanks,
> Jing
>
>>
>> Thanks,
>> Marcel
>>
>
> [...]