On 9/21/2024 1:56 AM, Alejandro Jimenez wrote:
> Hi Santosh,
>
> On 9/16/24 10:31, Santosh Shukla wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>
>>
>> Introduce 'nodma' shared memory region to support PT mode
>> so that for each device, we only create an alias to shared memory
>> region when DMA-remapping is disabled.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>
>> Signed-off-by: Santosh Shukla <santosh.shu...@amd.com>
>> ---
>> hw/i386/amd_iommu.c | 49 ++++++++++++++++++++++++++++++++++++---------
>> hw/i386/amd_iommu.h | 2 ++
>> 2 files changed, 42 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
>> index abb64ea507be..c5f5103f4911 100644
>> --- a/hw/i386/amd_iommu.c
>> +++ b/hw/i386/amd_iommu.c
>> @@ -60,8 +60,9 @@ struct AMDVIAddressSpace {
>> uint8_t bus_num; /* bus number */
>> uint8_t devfn; /* device function */
>> AMDVIState *iommu_state; /* AMDVI - one per machine */
>> - MemoryRegion root; /* AMDVI Root memory map region */
>> + MemoryRegion root; /* AMDVI Root memory map region */
>> IOMMUMemoryRegion iommu; /* Device's address translation region */
>> + MemoryRegion iommu_nodma; /* Alias of shared nodma memory region */
>> MemoryRegion iommu_ir; /* Device's interrupt remapping region */
>> AddressSpace as; /* device's corresponding address space */
>> };
>> @@ -1412,6 +1413,7 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus,
>> void *opaque, int devfn)
>> AMDVIState *s = opaque;
>> AMDVIAddressSpace **iommu_as, *amdvi_dev_as;
>> int bus_num = pci_bus_num(bus);
>> + X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>> iommu_as = s->address_spaces[bus_num];
>> @@ -1436,13 +1438,13 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus
>> *bus, void *opaque, int devfn)
>> * Memory region relationships looks like (Address range shows
>> * only lower 32 bits to make it short in length...):
>> *
>> - * |-----------------+-------------------+----------|
>> - * | Name | Address range | Priority |
>> - * |-----------------+-------------------+----------+
>> - * | amdvi_root | 00000000-ffffffff | 0 |
>> - * | amdvi_iommu | 00000000-ffffffff | 1 |
>> - * | amdvi_iommu_ir | fee00000-feefffff | 64 |
>> - * |-----------------+-------------------+----------|
>> + * |--------------------+-------------------+----------|
>> + * | Name | Address range | Priority |
>> + * |--------------------+-------------------+----------+
>> + * | amdvi-root | 00000000-ffffffff | 0 |
>> + * | amdvi-iommu_nodma | 00000000-ffffffff | 0 |
>> + * | amdvi-iommu_ir | fee00000-feefffff | 64 |
>> + * |--------------------+-------------------+----------|
>
> Minor nit: I would keep the original indentation here to help reinforce the
> concept that iommu_nodma and iommu_ir are meant to be sub-regions under the
> root container. It would also be great if the table could show that they are
V3 - Thank you for pointing that out.
- Santosh
mutually exclusive based on whether passthrough is in use, but that is probably
too much to include in this format.
>
> Alejandro
>> +
>> + if (!x86_iommu->pt_supported) {
>> + memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, false);
>> + memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
>> + true);
>> + } else {
>> + memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
>> + false);
>> + memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, true);
>> + }
>> }
>> return &iommu_as[devfn]->as;
>> }
>> @@ -1602,6 +1622,17 @@ static void amdvi_sysbus_realize(DeviceState *dev,
>> Error **errp)
>> "amdvi-mmio", AMDVI_MMIO_SIZE);
>> memory_region_add_subregion(get_system_memory(), AMDVI_BASE_ADDR,
>> &s->mr_mmio);
>> +
>> + /* Create the share memory regions by all devices */
>> + memory_region_init(&s->mr_sys, OBJECT(s), "amdvi-sys", UINT64_MAX);
>> +
>> + /* set up the DMA disabled memory region */
>> + memory_region_init_alias(&s->mr_nodma, OBJECT(s),
>> + "amdvi-nodma", get_system_memory(), 0,
>> + memory_region_size(get_system_memory()));
>> + memory_region_add_subregion_overlap(&s->mr_sys, 0,
>> + &s->mr_nodma, 0);
>> +
>> pci_setup_iommu(bus, &amdvi_iommu_ops, s);
>> amdvi_init(s);
>> }
>> diff --git a/hw/i386/amd_iommu.h b/hw/i386/amd_iommu.h
>> index e5c2ae94f243..be417e51c4dc 100644
>> --- a/hw/i386/amd_iommu.h
>> +++ b/hw/i386/amd_iommu.h
>> @@ -354,6 +354,8 @@ struct AMDVIState {
>> uint32_t pprlog_tail; /* ppr log tail */
>> MemoryRegion mr_mmio; /* MMIO region */
>> + MemoryRegion mr_sys;
>> + MemoryRegion mr_nodma;
>> uint8_t mmior[AMDVI_MMIO_SIZE]; /* read/write MMIO */
>> uint8_t w1cmask[AMDVI_MMIO_SIZE]; /* read/write 1 clear mask */
>> uint8_t romask[AMDVI_MMIO_SIZE]; /* MMIO read/only mask */
