On 05.08.19 15:25, Ralf Ramsauer wrote:
> 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.
>
Oops, the enum is still around?! I requested to remove it but didn't check
again...
>> + 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.
Yes, see above.
>
> 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.
Worth to consider. Or we put Intel first, so that the common case falls back to
the right value.
>
>> +
>> 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.
We have different configurations for both, so this can be seen as a safety
measure. Moreover, we will see different SMMU version on ARM without a chance to
tell them apart by CPU vendor. So there is a use case for this type field
outside of x86.
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/29ff1833-f55b-d698-fbf5-49db43af711a%40siemens.com.
