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

[Liu, Jing2]

Hi Marcel,

On 8/20/2018 9:38 PM, Marcel Apfelbaum wrote:
> 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 <address@hidden>
> > > > ---
> > > >   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.
I got it! Thanks very much for the details!

Jing

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