Re: [PATCH v3 2/6] arm-common: Introduce iommu functionality

['Lokesh Vutla' via Jailhouse]

On 16/07/19 10:06 AM, Jan Kiszka wrote:
> On 09.07.19 15:48, 'Pratyush Yadav' via Jailhouse wrote:
>> From: Nikhil Devshatwar <nikhil...@ti.com>
>>
>> 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 <nikhil...@ti.com>
>> Signed-off-by: Lokesh Vutla <lokeshvu...@ti.com>
>> ---
>>  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?

> 
>> +    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 <nikhil...@ti.com>
>> + *
>> + * 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 <nikhil...@ti.com>
>> + *
>> + * 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?

Thanks and regards,
Lokesh

-- 
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 jailhouse-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/jailhouse-dev/147060c1-064c-cb85-0c71-2a5217573c43%40ti.com.