On 03.07.19 13:03, Pratyush Yadav wrote:
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?
Logically, yes. I've only done that in past when QEMU was still lacking AMD IOMMU emulation. But the hardware (CPU vs. chipset) has hard dependencies that prevent that in practice.
+ 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 AMDreg_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.
Then we are almost back where we are today (only AMD needs extra fields). Maybe leave the union refactoring out for now so that you only add the type (which, I suppose, we will need on ARM when we want to add NXP's SMMUv2 implementation). I'm not against the union per see, but let's do that when needed (e.g. when an SMMU requires private fields).
Jan
+ __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
-- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux -- 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/0e71665a-efb7-1a85-d78a-600285407b82%40siemens.com. For more options, visit https://groups.google.com/d/optout.
