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

Do you agree? If yes, just call pci_del_capability and you don't need
this patch.


Thanks,
Marcel

> +
>   static const TypeInfo pci_bridge_type_info = {
>       .name = TYPE_PCI_BRIDGE,
>       .parent = TYPE_PCI_DEVICE,
> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> index 6186a32..b1e25ad 100644
> --- a/include/hw/pci/pci_bridge.h
> +++ b/include/hw/pci/pci_bridge.h
> @@ -147,4 +147,5 @@ typedef struct PCIResReserve {
>   int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
>                                  PCIResReserve res_reserve, Error **errp);
>   
> +void pci_bridge_qemu_reserve_cap_uninit(PCIDevice *dev);
>   #endif /* QEMU_PCI_BRIDGE_H */