Re: [PATCH v3 1/6] iommu: x86: Generalize iommu definition

['Lokesh Vutla' via Jailhouse]

On 16/07/19 9:44 AM, Jan Kiszka wrote:
> On 09.07.19 15:48, 'Pratyush Yadav' via Jailhouse wrote:
>> From: Nikhil Devshatwar <nikhil...@ti.com>
>>
>> Right now, jailhouse only supports iommu for x86.
>> Generalize the data structures to support iommus of different types
>>
>> Assumption is that each jailhouse_system can define iommu
>> instances of different types. Extend the jailhouse_iommu
>> to add type info.
>>
>> Update the x86 config files to reflect updated data the new type field.
>>
>> [p-yad...@ti.com: Add Intel IOMMU and fix compiler errors for AMD and
>> VT-D]
>>
>> Signed-off-by: Nikhil Devshatwar <nikhil...@ti.com>
>> Signed-off-by: Lokesh Vutla <lokeshvu...@ti.com>
>> Signed-off-by: Pratyush Yadav <p-yad...@ti.com>
>> ---
>> v2:
>> - Undo removing iommu_count_units().
>> - Remove the union from jailhouse_iommu.
>> - Remove the stray blank in amd_iommu.h
>> - Revert to using n instead of i in amd_iommu_init().
>> - Fail in Intel and AMD when any other type of IOMMU is found.
>> - Remove the accidental Intel configuration check.
>> - Update cell config template and pyjailhouse
>>
>> Jan, please take a close look at the template and pyjailhouse update.
>> I'm not sure if I missed something, or did something wrong.
>>
>>  configs/x86/f2a88xm-hd3.c       | 1 +
>>  configs/x86/qemu-x86.c          | 1 +
>>  hypervisor/arch/x86/amd_iommu.c | 9 ++++-----
>>  hypervisor/arch/x86/vtd.c       | 2 ++
>>  include/jailhouse/cell-config.h | 7 +++++++
>>  pyjailhouse/sysfs_parser.py     | 2 ++
>>  tools/root-cell-config.c.tmpl   | 1 +
>>  7 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/configs/x86/f2a88xm-hd3.c b/configs/x86/f2a88xm-hd3.c
>> index 315d0e29..849c5756 100644
>> --- a/configs/x86/f2a88xm-hd3.c
>> +++ b/configs/x86/f2a88xm-hd3.c
>> @@ -50,6 +50,7 @@ struct {
>>                              .pm_timer_address = 0x808,
>>                              .iommu_units = {
>>                                      {
>> +                                            .type = JAILHOUSE_IOMMU_AMD,
>>                                              .base = 0xfeb80000,
>>                                              .size = 0x80000,
>>                                              .amd_bdf = 0x02,
>> diff --git a/configs/x86/qemu-x86.c b/configs/x86/qemu-x86.c
>> index fdfa8915..2df2807a 100644
>> --- a/configs/x86/qemu-x86.c
>> +++ b/configs/x86/qemu-x86.c
>> @@ -50,6 +50,7 @@ struct {
>>                              .vtd_interrupt_limit = 256,
>>                              .iommu_units = {
>>                                      {
>> +                                            .type = JAILHOUSE_IOMMU_INTEL,
>>                                              .base = 0xfed90000,
>>                                              .size = 0x1000,
>>                                      },
>> diff --git a/hypervisor/arch/x86/amd_iommu.c 
>> b/hypervisor/arch/x86/amd_iommu.c
>> index 02712571..2fc6d033 100644
>> --- a/hypervisor/arch/x86/amd_iommu.c
>> +++ b/hypervisor/arch/x86/amd_iommu.c
>> @@ -448,7 +448,7 @@ static void amd_iommu_init_fault_nmi(void)
>>                  &system_config->platform_info.x86.iommu_units[iommu->idx];
>>  
>>              /* Disable MSI during interrupt reprogramming. */
>> -            pci_write_config(cfg->amd_bdf, cfg->amd_msi_cap + 2 , 0, 2);
>> +            pci_write_config(cfg->amd_bdf, cfg->amd_msi_cap + 2, 0, 2);
>>  
>>              /*
>>               * Write new MSI capability block, re-enabling interrupts with
>> @@ -782,14 +782,13 @@ static int amd_iommu_init(void)
>>  
>>      iommu = &system_config->platform_info.x86.iommu_units[0];
>>      for (n = 0; iommu->base && n < iommu_count_units(); iommu++, n++) {
>> +            if (iommu->type != JAILHOUSE_IOMMU_AMD)
>> +                    return trace_error(-EINVAL);
>> +
>>              entry = &iommu_units[iommu_units_count];
>>  
>>              entry->idx = n;
>>  
>> -            /* Protect against accidental VT-d configs. */
>> -            if (!iommu->amd_bdf)
>> -                    return trace_error(-EINVAL);
>> -
>>              printk("AMD IOMMU @0x%llx/0x%x\n", iommu->base, iommu->size);
>>  
>>              /* Initialize PCI registers and MMIO space */
>> diff --git a/hypervisor/arch/x86/vtd.c b/hypervisor/arch/x86/vtd.c
>> index a43632f5..dc89928f 100644
>> --- a/hypervisor/arch/x86/vtd.c
>> +++ b/hypervisor/arch/x86/vtd.c
>> @@ -1022,6 +1022,8 @@ static int vtd_init(void)
>>  
>>      for (n = 0; n < units; n++) {
>>              unit = &system_config->platform_info.x86.iommu_units[n];
>> +            if (unit->type != JAILHOUSE_IOMMU_INTEL)
>> +                    return trace_error(-EINVAL);
>>  
>>              reg_base = dmar_reg_base + n * DMAR_MMIO_SIZE;
>>  
>> diff --git a/include/jailhouse/cell-config.h 
>> b/include/jailhouse/cell-config.h
>> index 5739f332..781aac88 100644
>> --- a/include/jailhouse/cell-config.h
>> +++ b/include/jailhouse/cell-config.h
>> @@ -196,9 +196,16 @@ struct jailhouse_pci_capability {
>>  
>>  #define JAILHOUSE_MAX_IOMMU_UNITS   8
>>  
>> +enum jailhouse_iommu_type {
>> +    JAILHOUSE_IOMMU_AMD,
>> +    JAILHOUSE_IOMMU_INTEL,
>> +};
> 
> I would prefer defines with explicit values for this, like for other types.
> 
>> +
>>  struct jailhouse_iommu {
>> +    __u32 type;
>>      __u64 base;
>>      __u32 size;
>> +
>>      __u16 amd_bdf;
>>      __u8 amd_base_cap;
>>      __u8 amd_msi_cap;
>> diff --git a/pyjailhouse/sysfs_parser.py b/pyjailhouse/sysfs_parser.py
>> index 3db61980..06f68c87 100644
>> --- a/pyjailhouse/sysfs_parser.py
>> +++ b/pyjailhouse/sysfs_parser.py
>> @@ -243,6 +243,7 @@ def parse_dmar(pcidevices, ioapics, dmar_regions):
>>              if size > 0x3000:
>>                  raise RuntimeError('Unexpectedly large DMAR region.')
>>              units.append(IOMMUConfig({
>> +                'type': 'JAILHOUSE_IOMMU_INTEL',
>>                  'base_addr': base,
>>                  'mmio_size': size
>>              }))
>> @@ -387,6 +388,7 @@ def parse_ivrs(pcidevices, ioapics):
>>                  mmio_size = 16384
>>  
>>              units.append(IOMMUConfig({
>> +                'type': 'JAILHOUSE_IOMMU_AMD',
>>                  'base_addr': base_addr,
>>                  'mmio_size': mmio_size,
>>                  'amd_bdf': iommu_bdf,
> 
> This lacks IOMMUConfig initialization:
> 
> diff --git a/pyjailhouse/sysfs_parser.py b/pyjailhouse/sysfs_parser.py
> index 07c7f00e..67dc85d0 100644
> --- a/pyjailhouse/sysfs_parser.py
> +++ b/pyjailhouse/sysfs_parser.py
> @@ -976,6 +976,7 @@ class IOMemRegionTree:
> 
>  class IOMMUConfig:
>      def __init__(self, props):
> +        self.type = props['type']
>          self.base_addr = props['base_addr']
>          self.mmio_size = props['mmio_size']
>          if 'amd_bdf' in props:
> 
>> diff --git a/tools/root-cell-config.c.tmpl b/tools/root-cell-config.c.tmpl
>> index b6ac8637..9f904e9c 100644
>> --- a/tools/root-cell-config.c.tmpl
>> +++ b/tools/root-cell-config.c.tmpl
>> @@ -90,6 +90,7 @@ struct {
>>                              .iommu_units = {
>>                                      % for unit in iommu_units:
>>                                      {
>> +                                            .type = ${unit.type},
>>                                              .base = ${hex(unit.base_addr)},
>>                                              .size = ${hex(unit.mmio_size)},
>>                                              % if unit.is_amd_iommu:
>>
> 
> Rest looks good.

Sure, will fix the above comments in next version.

Thanks and regards,
Lokesh

-- 
You received this message because you are subscribed to the Google Groups 
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to jailhouse-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/jailhouse-dev/6638fe02-e784-3731-592e-05e32bc69e32%40ti.com.