Re: [PATCH v2 15/15] hw/misc: EDU: add ATS/PRI capability

[Daniel Henrique Barboza]

Hi Frank!

On 5/7/24 12:32, Frank Chang wrote:
> Hi Daniel,
>
> Daniel Henrique Barboza <dbarboza@ventanamicro.com> 於 2024年3月8日 週五 上午12:05寫道：
> > From: Tomasz Jeznach <tjeznach@rivosinc.com>
> >
> > Mimic ATS interface with IOMMU translate request with IOMMU_NONE.  If
> > mapping exists, translation service will return current permission
> > flags, otherwise will report no permissions.
> >
> > Implement and register the IOMMU memory region listener to be notified
> > whenever an ATS invalidation request is sent from the IOMMU.
> >
> > Implement and register the IOMMU memory region listener to be notified
> > whenever an ATS page request group response is triggered from the IOMMU.
> >
> > Introduces a retry mechanism to the timer design so that any page that's
> > not available should be only accessed after the PRGR notification has
> > been received.
> >
> > Signed-off-by: Tomasz Jeznach <tjeznach@rivosinc.com>
> > Signed-off-by: Sebastien Boeuf <seb@rivosinc.com>
> > ---
> >   hw/misc/edu.c | 258 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 251 insertions(+), 7 deletions(-)

(...)


> > +
> >   static void pci_edu_realize(PCIDevice *pdev, Error **errp)
> >   {
> >       EduState *edu = EDU(pdev);
> > +    AddressSpace *dma_as = NULL;
> >       uint8_t *pci_conf = pdev->config;
> >       int pos;
> >
> > @@ -390,9 +603,28 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
> >       pos = PCI_CONFIG_SPACE_SIZE;
> >       if (edu->enable_pasid) {
> >           /* PCIe Spec 7.8.9 PASID Extended Capability Structure */
> > -        pcie_add_capability(pdev, 0x1b, 1, pos, 8);
> > +        pcie_add_capability(pdev, PCI_EXT_CAP_ID_PASID, 1, pos, 8);
>
> This should be included in the 14th commit.
>
> >           pci_set_long(pdev->config + pos + 4, 0x00001400);
> >           pci_set_long(pdev->wmask + pos + 4,  0xfff0ffff);
> > +        pos += 8;
> > +
> > +        /* ATS Capability */
> > +        pcie_ats_init(pdev, pos, true);
> > +        pos += PCI_EXT_CAP_ATS_SIZEOF;
> > +
> > +        /* PRI Capability */
> > +        pcie_add_capability(pdev, PCI_EXT_CAP_ID_PRI, 1, pos, 16);
> > +        /* PRI STOPPED */
> > +        pci_set_long(pdev->config + pos +  4, 0x01000000);
> > +        /* PRI ENABLE bit writable */
> > +        pci_set_long(pdev->wmask  + pos +  4, 0x00000001);
> > +        /* PRI Capacity Supported */
> > +        pci_set_long(pdev->config + pos +  8, 0x00000080);
> > +        /* PRI Allocations Allowed, 32 */
> > +        pci_set_long(pdev->config + pos + 12, 0x00000040);
> > +        pci_set_long(pdev->wmask  + pos + 12, 0x0000007f);
>
> We should use the defines declared in
> include/standard-headers/linux/pci_regs.h for readability,
> though some of the bitfields are not defined in the header file.
>
> Regards,
> Frank Chang
>
> > +
> > +        pos += 8;
> >       }

I'll reply here for both patches 14 and 15.

I changed it to use the defines we have in pci_regs.h if we have the definition
in the header. When we don't have the definition I ended up adding a manual
comment in the line like it's being done up above.

I'll also add doc changes for each new feature added.

All this said, I'm inclined to remove these 2 patches from the series. It's a
way of experiment with the riscv-iommu impl but it's not a crucial part of it.
The changes I made so far, based on your review, were uploaded here:


https://gitlab.com/danielhb/qemu/-/commits/edu_pasid_v3


Thanks,

Daniel


> >       if (msi_init(pdev, 0, 1, true, false, errp)) {
> > @@ -409,12 +641,24 @@ static void pci_edu_realize(PCIDevice *pdev, Error **errp)
> >       memory_region_init_io(&edu->mmio, OBJECT(edu), &edu_mmio_ops, edu,
> >                       "edu-mmio", 1 * MiB);
> >       pci_register_bar(pdev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &edu->mmio);
> > +
> > +    /* Register IOMMU listener */
> > +    edu->iommu_listener = (MemoryListener) {
> > +        .name = "edu-iommu",
> > +        .region_add = edu_iommu_region_add,
> > +        .region_del = edu_iommu_region_del,
> > +    };
> > +
> > +    dma_as = pci_device_iommu_address_space(pdev);
> > +    memory_listener_register(&edu->iommu_listener, dma_as);
> >   }
> >
> >   static void pci_edu_uninit(PCIDevice *pdev)
> >   {
> >       EduState *edu = EDU(pdev);
> >
> > +    memory_listener_unregister(&edu->iommu_listener);
> > +
> >       qemu_mutex_lock(&edu->thr_mutex);
> >       edu->stopping = true;
> >       qemu_mutex_unlock(&edu->thr_mutex);
> > --
> > 2.43.2