Hi,
On 7/9/19 3:48 PM, 'Pratyush Yadav' via Jailhouse 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.
>
> Update the x86 config files to reflect updated data the new type field.
>
> [[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]>
> ---
> v2:
> - Undo removing iommu_count_units().
> - Remove the union from jailhouse_iommu.
> - Remove the stray blank in amd_iommu.h
> - Revert to using n instead of i in amd_iommu_init().
> - Fail in Intel and AMD when any other type of IOMMU is found.
> - Remove the accidental Intel configuration check.
> - Update cell config template and pyjailhouse
>
> Jan, please take a close look at the template and pyjailhouse update.
> I'm not sure if I missed something, or did something wrong.
>
> configs/x86/f2a88xm-hd3.c | 1 +
> configs/x86/qemu-x86.c | 1 +
> hypervisor/arch/x86/amd_iommu.c | 9 ++++-----
> hypervisor/arch/x86/vtd.c | 2 ++
> include/jailhouse/cell-config.h | 7 +++++++
> pyjailhouse/sysfs_parser.py | 2 ++
> tools/root-cell-config.c.tmpl | 1 +
> 7 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/configs/x86/f2a88xm-hd3.c b/configs/x86/f2a88xm-hd3.c
> index 315d0e29..849c5756 100644
> --- a/configs/x86/f2a88xm-hd3.c
> +++ b/configs/x86/f2a88xm-hd3.c
> @@ -50,6 +50,7 @@ struct {
> .pm_timer_address = 0x808,
> .iommu_units = {
> {
> + .type = JAILHOUSE_IOMMU_AMD,
> .base = 0xfeb80000,
> .size = 0x80000,
> .amd_bdf = 0x02,
> diff --git a/configs/x86/qemu-x86.c b/configs/x86/qemu-x86.c
> index fdfa8915..2df2807a 100644
> --- a/configs/x86/qemu-x86.c
> +++ b/configs/x86/qemu-x86.c
> @@ -50,6 +50,7 @@ struct {
> .vtd_interrupt_limit = 256,
> .iommu_units = {
> {
> + .type = JAILHOUSE_IOMMU_INTEL,
> .base = 0xfed90000,
> .size = 0x1000,
> },
> diff --git a/hypervisor/arch/x86/amd_iommu.c b/hypervisor/arch/x86/amd_iommu.c
> index 02712571..2fc6d033 100644
> --- a/hypervisor/arch/x86/amd_iommu.c
> +++ b/hypervisor/arch/x86/amd_iommu.c
> @@ -448,7 +448,7 @@ 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
> @@ -782,14 +782,13 @@ static int amd_iommu_init(void)
>
> iommu = &system_config->platform_info.x86.iommu_units[0];
> for (n = 0; iommu->base && n < iommu_count_units(); iommu++, n++) {
> + if (iommu->type != JAILHOUSE_IOMMU_AMD)
This is a comparison of an __u32 against an enum.
> + return trace_error(-EINVAL);
> +
> entry = &iommu_units[iommu_units_count];
>
> entry->idx = n;
>
> - /* Protect against accidental VT-d configs. */
> - if (!iommu->amd_bdf)
> - return trace_error(-EINVAL);
> -
> printk("AMD IOMMU @0x%llx/0x%x\n", iommu->base, iommu->size);
>
> /* Initialize PCI registers and MMIO space */
> diff --git a/hypervisor/arch/x86/vtd.c b/hypervisor/arch/x86/vtd.c
> index a43632f5..dc89928f 100644
> --- a/hypervisor/arch/x86/vtd.c
> +++ b/hypervisor/arch/x86/vtd.c
> @@ -1022,6 +1022,8 @@ 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)
Same here.
> + return trace_error(-EINVAL);
>
> reg_base = dmar_reg_base + n * DMAR_MMIO_SIZE;
>
> diff --git a/include/jailhouse/cell-config.h b/include/jailhouse/cell-config.h
> index 5739f332..781aac88 100644
> --- a/include/jailhouse/cell-config.h
> +++ b/include/jailhouse/cell-config.h
> @@ -196,9 +196,16 @@ struct jailhouse_pci_capability {
>
> #define JAILHOUSE_MAX_IOMMU_UNITS 8
>
> +enum jailhouse_iommu_type {
> + JAILHOUSE_IOMMU_AMD,
> + JAILHOUSE_IOMMU_INTEL,
> +};
Wouldn't it be better to use simple #defines here? As we use a __u32
type in the config, we don't benefit from the safer enum type.
BTW: I just rebased my local branch to next and Jailhouse failed
initialising due to the missing .type field of my local sysconfig. Well
- my bad. But if type is not set in the config, we now assume
JAILHOUSE_IOMMU_AMD per default.
Would it make sense to let those definitions start from 1? This would
ensure, that we have a empty type fields won't default to AMD.
> +
> struct jailhouse_iommu {
> + __u32 type;
This field changed the configuration format, and requires to increment
the config revision.
... But taking a step back I wonder why we actually need the type field:
We will never have an IOMMU_AMD on an Intel nor ARM system, et vice versa.
Ralf
> __u64 base;
> __u32 size;
> +
> __u16 amd_bdf;
> __u8 amd_base_cap;
> __u8 amd_msi_cap;
> diff --git a/pyjailhouse/sysfs_parser.py b/pyjailhouse/sysfs_parser.py
> index 3db61980..06f68c87 100644
> --- a/pyjailhouse/sysfs_parser.py
> +++ b/pyjailhouse/sysfs_parser.py
> @@ -243,6 +243,7 @@ def parse_dmar(pcidevices, ioapics, dmar_regions):
> if size > 0x3000:
> raise RuntimeError('Unexpectedly large DMAR region.')
> units.append(IOMMUConfig({
> + 'type': 'JAILHOUSE_IOMMU_INTEL',
> 'base_addr': base,
> 'mmio_size': size
> }))
> @@ -387,6 +388,7 @@ def parse_ivrs(pcidevices, ioapics):
> mmio_size = 16384
>
> units.append(IOMMUConfig({
> + 'type': 'JAILHOUSE_IOMMU_AMD',
> 'base_addr': base_addr,
> 'mmio_size': mmio_size,
> 'amd_bdf': iommu_bdf,
> diff --git a/tools/root-cell-config.c.tmpl b/tools/root-cell-config.c.tmpl
> index b6ac8637..9f904e9c 100644
> --- a/tools/root-cell-config.c.tmpl
> +++ b/tools/root-cell-config.c.tmpl
> @@ -90,6 +90,7 @@ struct {
> .iommu_units = {
> % for unit in iommu_units:
> {
> + .type = ${unit.type},
> .base = ${hex(unit.base_addr)},
> .size = ${hex(unit.mmio_size)},
> % if unit.is_amd_iommu:
>
--
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/14f0f8cf-b23f-63b9-22c6-7abadbcfb57d%40oth-regensburg.de.
