On 3/22/2019 5:15 PM, Jan Kiszka wrote:
> On 22.03.19 10:44, 'Lokesh' via Jailhouse wrote:
>> From: Nikhil Devshatwar <[email protected]>
>>
>> Right now, jailhouse only suppports 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 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
>>
>> Also get rid of the iommu_count_units and implement
>> simple logic to process iommus of the desired type.
>>
>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>> Signed-off-by: Lokesh Vutla <[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 -----
>> include/jailhouse/cell-config.h | 13 ++++++-
>> 6 files changed, 50 insertions(+), 45 deletions(-)
>>
>> diff --git a/configs/x86/f2a88xm-hd3.c b/configs/x86/f2a88xm-hd3.c
>> index d5320d7b..98f56665 100644
>> --- a/configs/x86/f2a88xm-hd3.c
>> +++ b/configs/x86/f2a88xm-hd3.c
>> @@ -52,12 +52,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 68b8f18d..3ec1fbab 100644
>> --- a/configs/x86/qemu-x86.c
>> +++ b/configs/x86/qemu-x86.c
>> @@ -52,8 +52,9 @@ struct {
>> .vtd_interrupt_limit = 256,
>> .iommu_units = {
>> {
>> - .base = 0xfed90000,
>> - .size = 0x1000,
>> + .type = JAILHOUSE_IOMMU_AMD,
>
> This one is actually Intel.
okay, I missed this. Will fix this and rest of the comments in next version.
Thanks and regards,
Lokesh
>
>> + .amd.base = 0xfed90000,
>> + .amd.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);
>> /*
>> * 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;
>> 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;
>> +
>> 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);
>> - 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;
>> -}
>
> This removal will break the build: you forgot the use case of
> hypervisor/arch/x86/vtd.c.
>
>> -
>> struct public_per_cpu *iommu_select_fault_reporting_cpu(void)
>> {
>> struct public_per_cpu *target_data;
>> diff --git a/include/jailhouse/cell-config.h
>> b/include/jailhouse/cell-config.h
>> index 66e13c3d..2d043bf9 100644
>> --- a/include/jailhouse/cell-config.h
>> +++ b/include/jailhouse/cell-config.h
>> @@ -191,13 +191,24 @@ struct jailhouse_pci_capability {
>> #define JAILHOUSE_MAX_IOMMU_UNITS 8
>> -struct jailhouse_iommu {
>> +enum jailhouse_iommu_type {
>> + JAILHOUSE_IOMMU_AMD,
>
> We have one more type: INTEL/VTD.
>
>> +};
>> +
>> +struct jailhouse_iommu_amd {
>> __u64 base;
>> __u32 size;
>
> Base and size are shared at least with Intel.
>
>> __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;
>> + };
>> } __attribute__((packed));
>> #define JAILHOUSE_SYSTEM_SIGNATURE "JHSYST"
>>
>
> Note that you will also have to update the config generator and its
> template.
>
> Jan
>
--
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].
For more options, visit https://groups.google.com/d/optout.
