On 22.07.19 11:51, Lokesh Vutla wrote:
>
>
> On 16/07/19 10:06 AM, Jan Kiszka wrote:
>> On 09.07.19 15:48, 'Pratyush Yadav' via Jailhouse wrote:
>>> From: Nikhil Devshatwar <[email protected]>
>>>
>>> Add the iommu hooks for ARM and ARM64 architectures with
>>> dummy implementations.
>>> Update the Kbuild to include new iommu files
>>>
>>> Introduce following hooks:
>>> iommu_map_memory - Setup iommu mapping for the memory region
>>> iommu_unmap_memory - Unmap the iommu mapping for the mem region
>>> iommu_config_commit - Commit all the changes to the mem mapping
>>>
>>> Call the map/unmap iommu functions in addition to CPU map/unmap and
>>> config_commit.
>>> Also add iommu_units in the platform data for ARM cells.
>>>
>>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>>> Signed-off-by: Lokesh Vutla <[email protected]>
>>> ---
>>> hypervisor/arch/arm-common/Kbuild | 2 +-
>>> hypervisor/arch/arm-common/control.c | 6 ++++
>>> .../arch/arm-common/include/asm/iommu.h | 26 +++++++++++++++
>>> hypervisor/arch/arm-common/iommu.c | 32 +++++++++++++++++++
>>> hypervisor/arch/arm-common/mmu_cell.c | 25 +++++++++++++++
>>> hypervisor/include/jailhouse/control.h | 1 +
>>> hypervisor/setup.c | 2 +-
>>> include/jailhouse/cell-config.h | 2 ++
>>> 8 files changed, 94 insertions(+), 2 deletions(-)
>>> create mode 100644 hypervisor/arch/arm-common/include/asm/iommu.h
>>> create mode 100644 hypervisor/arch/arm-common/iommu.c
>>>
>>> diff --git a/hypervisor/arch/arm-common/Kbuild
>>> b/hypervisor/arch/arm-common/Kbuild
>>> index 78b9e512..6f57ef02 100644
>>> --- a/hypervisor/arch/arm-common/Kbuild
>>> +++ b/hypervisor/arch/arm-common/Kbuild
>>> @@ -17,6 +17,6 @@ ccflags-$(CONFIG_JAILHOUSE_GCOV) += -fprofile-arcs
>>> -ftest-coverage
>>> objs-y += dbg-write.o lib.o psci.o control.o paging.o mmu_cell.o setup.o
>>> objs-y += irqchip.o pci.o ivshmem.o uart-pl011.o uart-xuartps.o
>>> uart-mvebu.o
>>> objs-y += uart-hscif.o uart-scifa.o uart-imx.o
>>> -objs-y += gic-v2.o gic-v3.o smccc.o
>>> +objs-y += gic-v2.o gic-v3.o smccc.o iommu.o
>>>
>>> common-objs-y = $(addprefix ../arm-common/,$(objs-y))
>>> diff --git a/hypervisor/arch/arm-common/control.c
>>> b/hypervisor/arch/arm-common/control.c
>>> index b59c05d6..d712b049 100644
>>> --- a/hypervisor/arch/arm-common/control.c
>>> +++ b/hypervisor/arch/arm-common/control.c
>>> @@ -16,6 +16,7 @@
>>> #include <jailhouse/printk.h>
>>> #include <asm/control.h>
>>> #include <asm/psci.h>
>>> +#include <asm/iommu.h>
>>>
>>> static void enter_cpu_off(struct public_per_cpu *cpu_public)
>>> {
>>> @@ -208,7 +209,12 @@ void arch_flush_cell_vcpu_caches(struct cell *cell)
>>>
>>> void arch_config_commit(struct cell *cell_added_removed)
>>> {
>>> + u8 err;
>>> +
>>> irqchip_config_commit(cell_added_removed);
>>> + err = iommu_config_commit(cell_added_removed);
>>
>> Passing an error here seems overkill. You are not generating any errors in
>> iommu_config_commit.
>
> Yeah, I can drop the check here. Do you also want to make the function to
> return
> void?
As long as it does not produce errors, yes. Once it does, we need to think about
that.
>
>>
>>> + if (err)
>>> + printk("WARNING: iommu_config_commit failed\n");
>>> }
>>>
>>> void __attribute__((noreturn)) arch_panic_stop(void)
>>> diff --git a/hypervisor/arch/arm-common/include/asm/iommu.h
>>> b/hypervisor/arch/arm-common/include/asm/iommu.h
>>> new file mode 100644
>>> index 00000000..d33ec17c
>>> --- /dev/null
>>> +++ b/hypervisor/arch/arm-common/include/asm/iommu.h
>>> @@ -0,0 +1,26 @@
>>> +/*
>>> + * Jailhouse, a Linux-based partitioning hypervisor
>>> + *
>>> + * Copyright (c) 2018 Texas Instruments Incorporated - http://www.ti.com
>>> + *
>>> + * Authors:
>>> + * Nikhil Devshatwar <[email protected]>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>>> + * the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#ifndef _JAILHOUSE_ASM_IOMMU_H
>>> +#define _JAILHOUSE_ASM_IOMMU_H
>>> +
>>> +#include <jailhouse/types.h>
>>> +#include <jailhouse/utils.h>
>>> +#include <jailhouse/config.h>
>>> +#include <jailhouse/cell-config.h>
>>> +
>>> +int iommu_map_memory_region(struct cell *cell,
>>> + const struct jailhouse_memory *mem);
>>> +int iommu_unmap_memory_region(struct cell *cell,
>>> + const struct jailhouse_memory *mem);
>>> +int iommu_config_commit(struct cell *cell);
>>> +#endif
>>> diff --git a/hypervisor/arch/arm-common/iommu.c
>>> b/hypervisor/arch/arm-common/iommu.c
>>> new file mode 100644
>>> index 00000000..5bc9e6a9
>>> --- /dev/null
>>> +++ b/hypervisor/arch/arm-common/iommu.c
>>> @@ -0,0 +1,32 @@
>>> +/*
>>> + * Jailhouse, a Linux-based partitioning hypervisor
>>> + *
>>> + * Copyright (c) 2018 Texas Instruments Incorporated - http://www.ti.com
>>> + *
>>> + * Authors:
>>> + * Nikhil Devshatwar <[email protected]>
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2. See
>>> + * the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#include <jailhouse/control.h>
>>> +#include <jailhouse/config.h>
>>> +#include <asm/iommu.h>
>>> +
>>> +int iommu_map_memory_region(struct cell *cell,
>>> + const struct jailhouse_memory *mem)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +int iommu_unmap_memory_region(struct cell *cell,
>>> + const struct jailhouse_memory *mem)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>> +int iommu_config_commit(struct cell *cell)
>>> +{
>>> + return 0;
>>> +}
>>> diff --git a/hypervisor/arch/arm-common/mmu_cell.c
>>> b/hypervisor/arch/arm-common/mmu_cell.c
>>> index 36a3016f..d51f92ca 100644
>>> --- a/hypervisor/arch/arm-common/mmu_cell.c
>>> +++ b/hypervisor/arch/arm-common/mmu_cell.c
>>> @@ -15,12 +15,14 @@
>>> #include <jailhouse/printk.h>
>>> #include <asm/sysregs.h>
>>> #include <asm/control.h>
>>> +#include <asm/iommu.h>
>>>
>>> int arch_map_memory_region(struct cell *cell,
>>> const struct jailhouse_memory *mem)
>>> {
>>> u64 phys_start = mem->phys_start;
>>> u32 flags = PTE_FLAG_VALID | PTE_ACCESS_FLAG;
>>> + int err = 0;
>>>
>>> if (mem->flags & JAILHOUSE_MEM_READ)
>>> flags |= S2_PTE_ACCESS_RO;
>>> @@ -37,6 +39,17 @@ int arch_map_memory_region(struct cell *cell,
>>> flags |= S2_PAGE_ACCESS_XN;
>>> */
>>>
>>> + /*
>>> + * Entire hypervisor memory is mapped to empty_page to avoid faults
>>> + * at the shutdown. We don't need this in the IOMMU mapping
>>> + * Skip mapping empty_page in the iommu mapping
>>> + */
>>
>> I agree this is not needed, but how much does it cost? Given the additional
>> effort here and on unmap and that we do not optimize this on x86, I would
>> really
>> like to understand the benefits.
>
> Isn't it wrong that hypervisor memory accessible by IOMMU? I guess it was
> added
> to just make things right. Do you want to drop it?
It is not wrong. At most, a device can read from that empty backing page. That's
why we did not care about this on x86. And if it would be a problem (more
regarding resources then), then it should be addressed generically.
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/180abb03-718e-880d-bdf8-b9c56258d58a%40siemens.com.
