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

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

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