On 03/07/19 4:43 PM, Jan Kiszka wrote:
> 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 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.
>>
>
> 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).
If we go the route of runtime stage 1 enable/disable configuration, the SMMU
will need a private boolean for the configuration. But I'll remove the union
for now.
> 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
>>>
>>
>
--
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/cabe78cb-fc68-189e-0055-63e34ee74ce9%40ti.com.
For more options, visit https://groups.google.com/d/optout.
