On 02/07/19 8:41 PM, Jan Kiszka wrote:
> On 02.07.19 16:36, Pratyush Yadav wrote:
>> From: Nikhil Devshatwar <[email protected]>
>>
>> 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 and add a type specific struct
>>
>> Move the AMD specific details in the jailhouse_iommu_amd and Intel
>> specific details in jailhouse_iommu_intel and update the code
>> accordingly.
>> Update the x86 config files which to reflect updated data
>> structure and define the new type field.
>> No code changes, just replace iommu->xyz with iommu->amd.xyz or
>> iommu->intel.xyz
>>
>> Also get rid of the iommu_count_units and implement
>> simple logic to process iommus of the desired type.
>
> This will require more resources, though it may safe some LoC. In any case,
> it should be a separate topic with some more detailed reasoning.
>
>>
>> [[email protected]: Add Intel IOMMU and fix compiler errors for AMD and
>> VT-D]
>>
>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>> Signed-off-by: Lokesh Vutla <[email protected]>
>> Signed-off-by: Pratyush Yadav <[email protected]>
>> ---
>> configs/x86/f2a88xm-hd3.c | 13 ++++---
>> configs/x86/qemu-x86.c | 5 ++-
>> hypervisor/arch/x86/amd_iommu.c | 52 +++++++++++++------------
>> hypervisor/arch/x86/include/asm/iommu.h | 2 -
>> hypervisor/arch/x86/iommu.c | 10 -----
>> hypervisor/arch/x86/vtd.c | 17 ++++----
>> include/jailhouse/cell-config.h | 28 ++++++++++---
>> 7 files changed, 70 insertions(+), 57 deletions(-)
>
> Looks like this does not update the config generator and the related config
> template, see tools/root-cell-config.c.tmpl and pyjailhouse/.
Ok, will do.
>>
>> diff --git a/configs/x86/f2a88xm-hd3.c b/configs/x86/f2a88xm-hd3.c
>> index 315d0e29..026f974a 100644
>> --- a/configs/x86/f2a88xm-hd3.c
>> +++ b/configs/x86/f2a88xm-hd3.c
>> @@ -50,12 +50,13 @@ struct {
>> .pm_timer_address = 0x808,
>> .iommu_units = {
>> {
>> - .base = 0xfeb80000,
>> - .size = 0x80000,
>> - .amd_bdf = 0x02,
>> - .amd_base_cap = 0x40,
>> - .amd_msi_cap = 0x54,
>> - .amd_features = 0x80048824,
>> + .type = JAILHOUSE_IOMMU_AMD,
>> + .amd.base = 0xfeb80000,
>> + .amd.size = 0x80000,
>> + .amd.bdf = 0x02,
>> + .amd.base_cap = 0x40,
>> + .amd.msi_cap = 0x54,
>> + .amd.features = 0x80048824,
>> },
>> },
>> },
>> diff --git a/configs/x86/qemu-x86.c b/configs/x86/qemu-x86.c
>> index fdfa8915..549deed9 100644
>> --- a/configs/x86/qemu-x86.c
>> +++ b/configs/x86/qemu-x86.c
>> @@ -50,8 +50,9 @@ struct {
>> .vtd_interrupt_limit = 256,
>> .iommu_units = {
>> {
>> - .base = 0xfed90000,
>> - .size = 0x1000,
>> + .type = JAILHOUSE_IOMMU_INTEL,
>> + .intel.base = 0xfed90000,
>> + .intel.size = 0x1000,
>> },
>> },
>> },
>> diff --git a/hypervisor/arch/x86/amd_iommu.c
>> b/hypervisor/arch/x86/amd_iommu.c
>> index 02712571..999590cd 100644
>> --- a/hypervisor/arch/x86/amd_iommu.c
>> +++ b/hypervisor/arch/x86/amd_iommu.c
>> @@ -448,14 +448,14 @@ 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);
>
> A chance to get rid of the stray blank "...cap + 2, 0, 2);"
Done.
>> /*
>> * Write new MSI capability block, re-enabling interrupts with
>> * the last word.
>> */
>> for (n = 3; n >= 0; n--)
>> - pci_write_config(cfg->amd_bdf, cfg->amd_msi_cap + 4 * n,
>> + pci_write_config(cfg->amd.bdf, cfg->amd.msi_cap + 4 * n,
>> msi_reg.raw[n], 4);
>> }
>> @@ -633,37 +633,37 @@ static int amd_iommu_init_pci(struct amd_iommu
>> *entry,
>> u64 caps_header, hi, lo;
>> /* Check alignment */
>> - if (iommu->size & (iommu->size - 1))
>> + if (iommu->amd.size & (iommu->amd.size - 1))
>> return trace_error(-EINVAL);
>> /* Check that EFR is supported */
>> - caps_header = pci_read_config(iommu->amd_bdf, iommu->amd_base_cap, 4);
>> + caps_header = pci_read_config(iommu->amd.bdf, iommu->amd.base_cap, 4);
>> if (!(caps_header & CAPS_IOMMU_EFR_SUP))
>> return trace_error(-EIO);
>> - lo = pci_read_config(iommu->amd_bdf,
>> - iommu->amd_base_cap + CAPS_IOMMU_BASE_LOW_REG, 4);
>> - hi = pci_read_config(iommu->amd_bdf,
>> - iommu->amd_base_cap + CAPS_IOMMU_BASE_HI_REG, 4);
>> + lo = pci_read_config(iommu->amd.bdf,
>> + iommu->amd.base_cap + CAPS_IOMMU_BASE_LOW_REG, 4);
>> + hi = pci_read_config(iommu->amd.bdf,
>> + iommu->amd.base_cap + CAPS_IOMMU_BASE_HI_REG, 4);
>> if (lo & CAPS_IOMMU_ENABLE &&
>> - ((hi << 32) | lo) != (iommu->base | CAPS_IOMMU_ENABLE)) {
>> + ((hi << 32) | lo) != (iommu->amd.base | CAPS_IOMMU_ENABLE)) {
>> printk("FATAL: IOMMU %d config is locked in invalid state.\n",
>> entry->idx);
>> return trace_error(-EPERM);
>> }
>> /* Should be configured by BIOS, but we want to be sure */
>> - pci_write_config(iommu->amd_bdf,
>> - iommu->amd_base_cap + CAPS_IOMMU_BASE_HI_REG,
>> - (u32)(iommu->base >> 32), 4);
>> - pci_write_config(iommu->amd_bdf,
>> - iommu->amd_base_cap + CAPS_IOMMU_BASE_LOW_REG,
>> - (u32)(iommu->base & 0xffffffff) | CAPS_IOMMU_ENABLE,
>> + pci_write_config(iommu->amd.bdf,
>> + iommu->amd.base_cap + CAPS_IOMMU_BASE_HI_REG,
>> + (u32)(iommu->amd.base >> 32), 4);
>> + pci_write_config(iommu->amd.bdf,
>> + iommu->amd.base_cap + CAPS_IOMMU_BASE_LOW_REG,
>> + (u32)(iommu->amd.base & 0xffffffff) | CAPS_IOMMU_ENABLE,
>> 4);
>> /* Allocate and map MMIO space */
>> - entry->mmio_base = paging_map_device(iommu->base, iommu->size);
>> + entry->mmio_base = paging_map_device(iommu->amd.base, iommu->amd.size);
>> if (!entry->mmio_base)
>> return -ENOMEM;
>> @@ -687,9 +687,9 @@ static int amd_iommu_init_features(struct amd_iommu
>> *entry,
>> return trace_error(-EIO);
>> /* Figure out if hardware events are supported. */
>> - if (iommu->amd_features)
>> + if (iommu->amd.features)
>> entry->he_supported =
>> - iommu->amd_features & ACPI_REPORTING_HE_SUP;
>> + iommu->amd.features & ACPI_REPORTING_HE_SUP;
>> else
>> entry->he_supported = efr & AMD_EXT_FEAT_HE_SUP;
>> @@ -777,20 +777,24 @@ static int amd_iommu_init(void)
>> {
>> struct jailhouse_iommu *iommu;
>> struct amd_iommu *entry;
>> - unsigned int n;
>> + unsigned int i;
>
> Why? "i" like "integer" i.e. signed. "n" because it's unsigned.
>
>> int err;
>> - iommu = &system_config->platform_info.x86.iommu_units[0];
>> - for (n = 0; iommu->base && n < iommu_count_units(); iommu++, n++) {
>> + for (i = 0; i < JAILHOUSE_MAX_IOMMU_UNITS; i++) {
>> +
>> + iommu = &system_config->platform_info.x86.iommu_units[i];
>> + if (iommu->type != JAILHOUSE_IOMMU_AMD)
>> + continue;
>
> return trace_error(-EINVAL);
Can you not mix two types of IOMMUs on x86? What if a system has both Intel and
AMD IOMMUs?
>> +
>> entry = &iommu_units[iommu_units_count];
>> - entry->idx = n;
>> + entry->idx = i;
>> /* Protect against accidental VT-d configs. */
>> - if (!iommu->amd_bdf)
>> + if (!iommu->amd.bdf)
>> return trace_error(-EINVAL);
>
> Can be removed when you have the type check above.
Will do.
>> - printk("AMD IOMMU @0x%llx/0x%x\n", iommu->base, iommu->size);
>> + printk("AMD IOMMU @0x%llx/0x%x\n", iommu->amd.base,
>> iommu->amd.size);
>> /* Initialize PCI registers and MMIO space */
>> err = amd_iommu_init_pci(entry, iommu);
>> diff --git a/hypervisor/arch/x86/include/asm/iommu.h
>> b/hypervisor/arch/x86/include/asm/iommu.h
>> index 848feb77..92051673 100644
>> --- a/hypervisor/arch/x86/include/asm/iommu.h
>> +++ b/hypervisor/arch/x86/include/asm/iommu.h
>> @@ -23,8 +23,6 @@
>> extern unsigned int fault_reporting_cpu_id;
>> -unsigned int iommu_count_units(void);
>> -
>> int iommu_map_memory_region(struct cell *cell,
>> const struct jailhouse_memory *mem);
>> int iommu_unmap_memory_region(struct cell *cell,
>> diff --git a/hypervisor/arch/x86/iommu.c b/hypervisor/arch/x86/iommu.c
>> index 68ca323f..aeaf21e5 100644
>> --- a/hypervisor/arch/x86/iommu.c
>> +++ b/hypervisor/arch/x86/iommu.c
>> @@ -15,16 +15,6 @@
>> unsigned int fault_reporting_cpu_id;
>> -unsigned int iommu_count_units(void)
>> -{
>> - unsigned int units = 0;
>> -
>> - while (units < JAILHOUSE_MAX_IOMMU_UNITS &&
>> - system_config->platform_info.x86.iommu_units[units].base)
>> - units++;
>> - return units;
>> -}
>> -
>> struct public_per_cpu *iommu_select_fault_reporting_cpu(void)
>> {
>> struct public_per_cpu *target_data;
>> diff --git a/hypervisor/arch/x86/vtd.c b/hypervisor/arch/x86/vtd.c
>> index a43632f5..1e817b36 100644
>> --- a/hypervisor/arch/x86/vtd.c
>> +++ b/hypervisor/arch/x86/vtd.c
>> @@ -207,7 +207,7 @@ static bool dmar_units_initialized;
>> static unsigned int vtd_mmio_count_regions(struct cell *cell)
>> {
>> - return cell == &root_cell ? iommu_count_units() : 0;
>> + return cell == &root_cell ? JAILHOUSE_MAX_IOMMU_UNITS : 0;
>
> Probably fine, at least as long as JAILHOUSE_MAX_IOMMU_UNITS is just 8.
>
>> }
>> static unsigned int inv_queue_write(void *inv_queue, unsigned int index,
>> @@ -959,7 +959,7 @@ static int vtd_init_ir_emulation(unsigned int unit_no,
>> void *reg_base)
>> root_cell.arch.vtd.ir_emulation = true;
>> - base = system_config->platform_info.x86.iommu_units[unit_no].base;
>> + base = system_config->platform_info.x86.iommu_units[unit_no].intel.base;
>> mmio_region_register(&root_cell, base, PAGE_SIZE,
>> vtd_unit_access_handler, unit);
>> @@ -1008,9 +1008,7 @@ static int vtd_init(void)
>> int_remap_table_size_log2 = n;
>> - units = iommu_count_units();
>> - if (units == 0)
>> - return trace_error(-EINVAL);
>
> Where does this check go? Well, it will probably be better placed in an
> offline "jailhouse config check" script... OK, fine with me.
>
>> + units = JAILHOUSE_MAX_IOMMU_UNITS;
>
> This is wrong now: This is not just an upper limit, we also set dmar_units to
> this. You will have to update the code accordingly. And I don't think we need
> "units" anymore then.
>
>> dmar_reg_base = page_alloc(&remap_pool, units *
>> PAGES(DMAR_MMIO_SIZE));
>> if (!dmar_reg_base)
>> @@ -1022,11 +1020,13 @@ 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)
>> + continue;
>
> That would actually be a bug, and we should fail, analogously to AMD
>
>> reg_base = dmar_reg_base + n * DMAR_MMIO_SIZE;
>> - err = paging_create(&hv_paging_structs, unit->base, unit->size,
>> - (unsigned long)reg_base,
>> + err = paging_create(&hv_paging_structs, unit->intel.base,
>> + unit->intel.size, (unsigned long)reg_base,
>> PAGE_DEFAULT_FLAGS | PAGE_FLAG_DEVICE,
>> PAGING_NON_COHERENT);
>> if (err)
>> @@ -1036,7 +1036,8 @@ static int vtd_init(void)
>> if (version < VTD_VER_MIN || version == 0xff)
>> return trace_error(-EIO);
>> - printk("DMAR unit @0x%llx/0x%x\n", unit->base, unit->size);
>> + printk("DMAR unit @0x%llx/0x%x\n", unit->intel.base,
>> + unit->intel.size);
>> caps = mmio_read64(reg_base + VTD_CAP_REG);
>> if (caps & VTD_CAP_SAGAW39)
>> diff --git a/include/jailhouse/cell-config.h
>> b/include/jailhouse/cell-config.h
>> index 5739f332..f492e409 100644
>> --- a/include/jailhouse/cell-config.h
>> +++ b/include/jailhouse/cell-config.h
>> @@ -196,13 +196,31 @@ struct jailhouse_pci_capability {
>> #define JAILHOUSE_MAX_IOMMU_UNITS 8
>> -struct jailhouse_iommu {
>> +enum jailhouse_iommu_type {
>> + JAILHOUSE_IOMMU_AMD,
>> + JAILHOUSE_IOMMU_INTEL,
>> +};
>> +
>> +struct jailhouse_iommu_amd {
>> + __u64 base;
>> + __u32 size;
>
> Look like base and size remain common fields, for all IOMMU types. Why making
> them specific then?
No specific reason. I'll make them common.
>> + __u16 bdf;
>> + __u8 base_cap;
>> + __u8 msi_cap;
>> + __u32 features;
>> +};
>> +
>> +struct jailhouse_iommu_intel {
>> __u64 base;
>> __u32 size;
>> - __u16 amd_bdf;
>> - __u8 amd_base_cap;
>> - __u8 amd_msi_cap;
>> - __u32 amd_features;
>> +};
>> +
>> +struct jailhouse_iommu {
>> + __u32 type;
>> + union {
>> + struct jailhouse_iommu_amd amd;
>> + struct jailhouse_iommu_intel intel;
>> + };
>> } __attribute__((packed));
>> #define JAILHOUSE_SYSTEM_SIGNATURE "JHSYST"
>>
>
> Jan
>
--
Regards,
Pratyush Yadav
--
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 [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/jailhouse-dev/c86f10aa-db0a-e133-ef0c-f5710f437bde%40ti.com.
For more options, visit https://groups.google.com/d/optout.
