Re: [PATCH v2 2/5] amd_iommu: Add support for pass though mode

["Shukla, Santosh" <santosh.shukla@amd.com>]

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.suthikulpanit@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.suthikulpanit@amd.com>
>> Signed-off-by: Santosh Shukla <santosh.shukla@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          */