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/.
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);"
/*
* 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);
+
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.
- 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?
+ __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/43fa7815-96ee-a91e-c8b3-9e23f146f502%40siemens.com.
For more options, visit https://groups.google.com/d/optout.
