[RFC PATCH v1 0/4] Add support for partitioning registers

[nikh...@ti.com]

From: Nikhil Devshatwar <nikh...@ti.com>

This series adds support for partitioning registers across different cells
in the Jailhouse. Jailhouse supports partitioning memory regions; where it uses
MMU mapping for page aligned regions and subpage handler for non aligned regions.

However, most of the embedded platforms will have common set of registers which
need to be partitioned at the granularity of single register. One such example is
the pinmux registers avaialble in many platforms including K3 J721e.

This series implements a regmap unit which allows to describe the ownerhip of the
registers using a simple bitmap. This scales well when you have to partition
hundreds of control module or pinmux registers.

Nikhil Devshatwar (4):
configs: arm64: k3-j721e-linux: Add USB mem_regions
core: Introduce regmaps in cell config for partitioning registers
core: Implement regmap unit for partitioning registers
configs: k3-j721e: Add regmaps for PADCONFIG registers

configs/arm64/k3-j721e-evm-linux-demo.c | 41 +++-
configs/arm64/k3-j721e-evm.c | 15 ++
hypervisor/Makefile | 2 +-
hypervisor/include/jailhouse/cell.h | 2 +
hypervisor/include/jailhouse/regmap.h | 47 +++++
hypervisor/regmap.c | 258 ++++++++++++++++++++++++
include/jailhouse/cell-config.h | 22 +-
tools/jailhouse-cell-linux | 5 +-
tools/jailhouse-hardware-check | 2 +-
9 files changed, 387 insertions(+), 7 deletions(-)
create mode 100644 hypervisor/include/jailhouse/regmap.h
create mode 100644 hypervisor/regmap.c

--
2.17.1

[nikh...@ti.com]

From: Nikhil Devshatwar <nikh...@ti.com>

Signed-off-by: Nikhil Devshatwar <nikh...@ti.com>
---
configs/arm64/k3-j721e-evm-linux-demo.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/configs/arm64/k3-j721e-evm-linux-demo.c b/configs/arm64/k3-j721e-evm-linux-demo.c
index 9a853e9a..47ad32ea 100644
--- a/configs/arm64/k3-j721e-evm-linux-demo.c
+++ b/configs/arm64/k3-j721e-evm-linux-demo.c
@@ -24,7 +24,7 @@
struct {
struct jailhouse_cell_desc cell;
__u64 cpus[1];
- struct jailhouse_memory mem_regions[25];
+ struct jailhouse_memory mem_regions[27];
struct jailhouse_irqchip irqchips[4];
struct jailhouse_pci_device pci_devices[1];
__u32 stream_ids[2];
@@ -86,6 +86,20 @@ struct {
.flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE |
JAILHOUSE_MEM_IO | JAILHOUSE_MEM_ROOTSHARED,
},
+ /* USBSS1 */ {
+ .phys_start = 0x04114000,
+ .virt_start = 0x04114000,
+ .size = 0x1000,
+ .flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE |
+ JAILHOUSE_MEM_IO,
+ },
+ /* USB1.USB3 */ {
+ .phys_start = 0x06400000,
+ .virt_start = 0x06400000,
+ .size = 0x30000,
+ .flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE |
+ JAILHOUSE_MEM_IO,
+ },
/* main_gpio2 */ {
.phys_start = 0x00610000,
.virt_start = 0x00610000,
--
2.17.1

[nikh...@ti.com]

From: Nikhil Devshatwar <nikh...@ti.com>

Typical SoCs tend to have hardware blocks where you have to
partition the resources at a register level.
e.g. pinmux, clock control, and other common registers need to be
partitioned at register level granularity.

Adding jailhouse_mem_region for each register does not make sense
because of the high number of such registers. A simple bitmap
would be useful to describe the individual register ownership.

Add support for jailhouse_regmap, where each bit indicates ownership
of a register in the region specified.

Update the HEADER_REVISION and struct parser formats to reflect new
struct jailhouse_cell_desc

Signed-off-by: Nikhil Devshatwar <nikh...@ti.com>
---

include/jailhouse/cell-config.h | 22 ++++++++++++++++++++--
tools/jailhouse-cell-linux | 5 +++--
tools/jailhouse-hardware-check | 2 +-
3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/include/jailhouse/cell-config.h b/include/jailhouse/cell-config.h
index 30ec5d06..18d63dde 100644
--- a/include/jailhouse/cell-config.h
+++ b/include/jailhouse/cell-config.h
@@ -50,7 +50,7 @@
* Incremented on any layout or semantic change of system or cell config.
* Also update HEADER_REVISION in tools.
*/
-#define JAILHOUSE_CONFIG_REVISION 12
+#define JAILHOUSE_CONFIG_REVISION 13

#define JAILHOUSE_CELL_NAME_MAXLEN 31

@@ -96,6 +96,7 @@ struct jailhouse_cell_desc {
__u32 num_pci_devices;
__u32 num_pci_caps;
__u32 num_stream_ids;
+ __u32 num_regmaps;

__u32 vpci_irq_base;

@@ -241,6 +242,14 @@ struct jailhouse_pci_capability {
#define JAILHOUSE_APIC_MODE_XAPIC 1
#define JAILHOUSE_APIC_MODE_X2APIC 2

+struct jailhouse_regmap {
+ __u64 reg_base;
+ __u32 reg_count;
+ __u8 reg_size;
+ __u32 flags;
+ __u32 reg_bitmap[8];
+} __attribute__((packed));
+
#define JAILHOUSE_MAX_IOMMU_UNITS 8

#define JAILHOUSE_IOMMU_AMD 1
@@ -344,7 +353,8 @@ jailhouse_cell_config_size(struct jailhouse_cell_desc *cell)
cell->num_pio_regions * sizeof(struct jailhouse_pio) +
cell->num_pci_devices * sizeof(struct jailhouse_pci_device) +
cell->num_pci_caps * sizeof(struct jailhouse_pci_capability) +
- cell->num_stream_ids * sizeof(__u32);
+ cell->num_stream_ids * sizeof(__u32) +
+ cell->num_regmaps * sizeof(struct jailhouse_regmap);
}

static inline __u32
@@ -415,4 +425,12 @@ jailhouse_cell_stream_ids(const struct jailhouse_cell_desc *cell)
cell->num_pci_caps * sizeof(struct jailhouse_pci_capability));
}

+static inline const struct jailhouse_regmap *
+jailhouse_cell_regmaps(const struct jailhouse_cell_desc *cell)
+{
+ return (const struct jailhouse_regmap *)
+ ((void *)jailhouse_cell_stream_ids(cell) +
+ cell->num_stream_ids * sizeof(__u32));
+}
+
#endif /* !_JAILHOUSE_CELL_CONFIG_H */
diff --git a/tools/jailhouse-cell-linux b/tools/jailhouse-cell-linux
index 007a5c46..91bc4a6d 100755
--- a/tools/jailhouse-cell-linux
+++ b/tools/jailhouse-cell-linux
@@ -573,8 +573,8 @@ class PIORegion:


class Config:
- _HEADER_FORMAT = '=6sH32s4xIIIIIIIIIIQ8x32x'
- _HEADER_REVISION = 12
+ _HEADER_FORMAT = '=6sH32s4xIIIIIIIIIIIQ8x32x'
+ _HEADER_REVISION = 13

def __init__(self, config_file):
self.data = config_file.read()
@@ -591,6 +591,7 @@ class Config:
self.num_pci_devices,
self.num_pci_caps,
self.num_stream_ids,
+ self.num_regmaps,
self.vpci_irq_base,
self.cpu_reset_address) = \
struct.unpack_from(Config._HEADER_FORMAT, self.data)
diff --git a/tools/jailhouse-hardware-check b/tools/jailhouse-hardware-check
index 375816e2..2db103c0 100755
--- a/tools/jailhouse-hardware-check
+++ b/tools/jailhouse-hardware-check
@@ -136,7 +136,7 @@ class Sysconfig:
X86_MAX_IOMMU_UNITS = 8
X86_IOMMU_SIZE = 20

- HEADER_REVISION = 12
+ HEADER_REVISION = 13
HEADER_FORMAT = '6sH'

def __init__(self, path):
--
2.17.1

[nikh...@ti.com]

From: Nikhil Devshatwar <nikh...@ti.com>

Implement regmap as a unit, Use reg_map_data as book keeping
data structure per cell.

Register a MMIO handler for each regmap region and handle the
mmio access based on the regmap described in the config.

Implement the regmap_modify_root to map and unmap the regmap
access from the root cell while creating inmate cells.

Signed-off-by: Nikhil Devshatwar <nikh...@ti.com>
---

hypervisor/Makefile | 2 +-
hypervisor/include/jailhouse/cell.h | 2 +
hypervisor/include/jailhouse/regmap.h | 47 +++++

hypervisor/regmap.c | 258 ++++++++++++++++++++++++++
4 files changed, 308 insertions(+), 1 deletion(-)

create mode 100644 hypervisor/include/jailhouse/regmap.h
create mode 100644 hypervisor/regmap.c

diff --git a/hypervisor/Makefile b/hypervisor/Makefile
index 893ead42..62c86a4b 100644
--- a/hypervisor/Makefile
+++ b/hypervisor/Makefile
@@ -36,7 +36,7 @@ ifneq ($(wildcard $(INC_CONFIG_H)),)
KBUILD_CFLAGS += -include $(INC_CONFIG_H)
endif

-CORE_OBJECTS = setup.o printk.o paging.o control.o lib.o mmio.o pci.o ivshmem.o
+CORE_OBJECTS = setup.o printk.o paging.o control.o lib.o mmio.o pci.o ivshmem.o regmap.o
CORE_OBJECTS += uart.o uart-8250.o

ifdef CONFIG_JAILHOUSE_GCOV
diff --git a/hypervisor/include/jailhouse/cell.h b/hypervisor/include/jailhouse/cell.h
index c804a5df..90575bb9 100644
--- a/hypervisor/include/jailhouse/cell.h
+++ b/hypervisor/include/jailhouse/cell.h
@@ -69,6 +69,8 @@ struct cell {
unsigned int num_mmio_regions;
/** Maximum number of MMIO regions. */
unsigned int max_mmio_regions;
+ /** List of register maps assigned to this cell. */
+ struct reg_map_data *regmap;
};

extern struct cell root_cell;
diff --git a/hypervisor/include/jailhouse/regmap.h b/hypervisor/include/jailhouse/regmap.h
new file mode 100644
index 00000000..98faf2c8
--- /dev/null
+++ b/hypervisor/include/jailhouse/regmap.h
@@ -0,0 +1,47 @@
+/*
+ * Jailhouse, a Linux-based partitioning hypervisor
+ *
+ * Copyright (c) 2019 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Authors:
+ * Nikhil Devshatwar <nikh...@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_REGMAP_H
+#define _JAILHOUSE_REGMAP_H
+
+#include <jailhouse/types.h>
+#include <asm/mmio.h>
+#include <jailhouse/cell-config.h>
+
+struct cell;
+
+/**
+ * @defgroup REGMAP Regmap subsystem
+ *
+ * This subsystem provides interpretation and handling of intercepted
+ * register accesses performed by cells.
+ *
+ * @{
+ */
+
+#define JAILHOUSE_REGMAP_WORDS 8
+#define JAILHOUSE_REGMAP_BITS (JAILHOUSE_REGMAP_WORDS * 32)
+
+/** Register map description */
+struct reg_map_data {
+ /** Reference to regmap defined in config */
+ const struct jailhouse_regmap *info;
+ /** Owning cell */
+ struct cell *cell;
+ /** virt address where this regmap is mapped */
+ void *map_base;
+ /** Ownership details for each register */
+ u32 reg_bitmap[JAILHOUSE_REGMAP_WORDS];
+};
+
+/** @} REGMAP */
+#endif /* !_JAILHOUSE_REGMAP_H */
diff --git a/hypervisor/regmap.c b/hypervisor/regmap.c
new file mode 100644
index 00000000..9f3d32dc
--- /dev/null
+++ b/hypervisor/regmap.c
@@ -0,0 +1,258 @@
+/*
+ * Jailhouse, a Linux-based partitioning hypervisor
+ *
+ * Copyright (c) 2019 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Authors:
+ * Nikhil Devshatwar <nikh...@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/cell.h>
+#include <jailhouse/control.h>
+#include <jailhouse/paging.h>
+#include <jailhouse/printk.h>
+#include <jailhouse/unit.h>
+#include <jailhouse/percpu.h>
+#include <jailhouse/regmap.h>
+
+static inline bool regmap_is_enabled(struct reg_map_data *regmap, int reg)
+{
+ u32 idx, mask;
+
+ idx = reg / 32;
+ mask = 1 << (reg % 32);
+
+ return regmap->reg_bitmap[idx] & mask ? 1 : 0;
+}
+
+static inline void regmap_enable(struct reg_map_data *regmap, int reg)
+{
+ u32 idx, mask;
+
+ idx = reg / 32;
+ mask = 1 << (reg % 32);
+
+ regmap->reg_bitmap[idx] |= mask;
+}
+
+static inline void regmap_disable(struct reg_map_data *regmap, int reg)
+{
+ u32 idx, mask;
+
+ idx = reg / 32;
+ mask = 1 << (reg % 32);
+
+ regmap->reg_bitmap[idx] &= ~mask;
+}
+
+/**
+ * Find the regmap which degines the ownership bitmap for
+ * the register address provided.
+ *
+ * @param cell Cell in which to search.
+ * @param addr Register address to match
+ * @param idx Pointer to index, populated with index of register in
+ * the matching regmap
+ *
+ * @return Valid reg_map_data or NULL when not found.
+ */
+static struct reg_map_data *cell_get_regmap(struct cell *cell,
+ unsigned long addr, unsigned int *idx)
+{
+ const struct jailhouse_regmap *info;
+ struct reg_map_data *regmap;
+ unsigned long start, end;
+ u32 i;
+
+ for (i = 0; i < cell->config->num_regmaps; i++) {
+ regmap = &cell->regmap[i];
+ info = regmap->info;
+ start = (unsigned long)info->reg_base;
+ end = (unsigned long)start + info->reg_size * info->reg_count;
+
+ if (addr < start || addr >= end)
+ continue;
+
+ *idx = (addr - info->reg_base) / info->reg_size;
+ return regmap;
+ }
+ return NULL;
+}
+
+/**
+ * Handle emulation of regmap access as per permission bitmap
+ * Check regmap access permissions and ownership
+ * Based on that, allow or forbid the MMIOs access to register
+ *
+ * @param arg Private argument, reg_map_data.
+ * @param mmio describes the mmio access which caused the fault
+ *
+ * @return MMIO_HANDLED if the access is as per regmap description,
+ * MMIO_ERROR if it violates some of the permissions,
+ */
+static enum mmio_result regmap_handler(void *arg, struct mmio_access *mmio)
+{
+ struct reg_map_data *regmap = (struct reg_map_data *)arg;
+ const struct jailhouse_regmap *info;
+ unsigned int idx;
+
+ info = regmap->info;
+ idx = mmio->address / info->reg_size;
+
+ if (mmio->is_write) {
+ if ((info->flags & JAILHOUSE_MEM_WRITE) == 0)
+ return MMIO_ERROR;
+ } else {
+ if ((info->flags & JAILHOUSE_MEM_READ) == 0)
+ return MMIO_ERROR;
+ }
+
+ if (regmap_is_enabled(regmap, idx)) {
+ mmio_perform_access(regmap->map_base, mmio);
+ return MMIO_HANDLED;
+ } else {
+ printk("MMIO access disabled\n");
+ return MMIO_ERROR;
+ }
+}
+
+/**
+ * Modify root_cell's bitmap to (un)mask the registers defined in inmate cell.
+ * Ignore if the root cell does not describe the regmap used by inmate
+ * Handles the case where root cell describes the registers using
+ * different address range
+ *
+ * @param cell inmate cell handle.
+ * @param regmap register (un)map to be removed from root_cell.
+ * @param map true to map the regmap, false to unmap.
+ *
+ * @return 0 on successfully (un)mapping the regmap.
+ */
+static int regmap_modify_root(struct cell *cell, struct reg_map_data *regmap,
+ bool map)
+{
+ const struct jailhouse_regmap *info = regmap->info;
+ struct reg_map_data *root_regmap = NULL;
+ unsigned long long addr;
+ u32 reg, idx;
+
+ if (cell == &root_cell)
+ return 0;
+ if (info->flags & JAILHOUSE_MEM_ROOTSHARED)
+ return 0;
+
+ for (reg = 0; reg < info->reg_count; reg++) {
+
+ addr = info->reg_base + reg * info->reg_size;
+ if (!root_regmap) {
+ root_regmap = cell_get_regmap(&root_cell, addr, &idx);
+ if (!root_regmap)
+ continue;
+ }
+
+ if (regmap_is_enabled(regmap, reg)) {
+ if (map) {
+ regmap_enable(root_regmap, idx);
+
+ /* For unmapping, ensure that its mapped in root cell regmap */
+ } else if (regmap_is_enabled(root_regmap, idx)) {
+
+ regmap_disable(root_regmap, idx);
+ } else {
+ printk("ERROR: Root cell does not own bitmap for reg %llx\n",
+ addr);
+ return -EINVAL;
+ }
+ }
+
+ /* reuse the same root_regmap for next register if idx is within limit */
+ idx++;
+ if (idx >= root_regmap->info->reg_count)
+ root_regmap = NULL;
+ }
+ return 0;
+}
+
+static int regmap_cell_init(struct cell *cell)
+{
+ const struct jailhouse_regmap *info;
+ struct reg_map_data *regmap;
+ u32 i, num_pages, size;
+ int ret;
+
+ if (cell->config->num_regmaps == 0)
+ return 0;
+
+ num_pages = PAGES(cell->config->num_regmaps * sizeof(struct reg_map_data));
+ cell->regmap = page_alloc(&mem_pool, num_pages);
+ if (!cell->regmap)
+ return -ENOMEM;
+
+ info = jailhouse_cell_regmaps(cell->config);
+ for (i = 0; i < cell->config->num_regmaps; i++, info++) {
+
+ regmap = &cell->regmap[i];
+ regmap->info = info;
+ regmap->cell = cell;
+ size = info->reg_size * info->reg_count;
+
+ if (info->reg_count > JAILHOUSE_REGMAP_BITS ||
+ (info->flags & (JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE)) == 0)
+ goto invalid;
+
+ regmap->map_base = paging_map_device(info->reg_base, size);
+ if (!regmap->map_base)
+ return -ENOMEM;
+
+ memcpy(regmap->reg_bitmap, info->reg_bitmap,
+ sizeof(regmap->reg_bitmap));
+
+ mmio_region_register(cell, info->reg_base, size,
+ regmap_handler, regmap);
+
+ /* Unmap the memory so that handler can be triggered */
+ ret = paging_destroy(&cell->arch.mm, info->reg_base, size,
+ PAGING_COHERENT);
+ if (ret)
+ goto invalid;
+
+ ret = regmap_modify_root(cell, regmap, false);
+ if (ret)
+ goto invalid;
+ }
+
+ return 0;
+invalid:
+ page_free(&mem_pool, cell->regmap, 1);
+ return -EINVAL;
+}
+
+static void regmap_cell_exit(struct cell *cell)
+{
+ struct reg_map_data *regmap;
+ u32 i, num_pages;
+
+ for (i = 0; i < cell->config->num_regmaps; i++) {
+ regmap = &cell->regmap[i];
+ regmap_modify_root(cell, regmap, true);
+ }
+
+ num_pages = PAGES(cell->config->num_regmaps);
+ page_free(&mem_pool, cell->regmap, num_pages);
+}
+
+static int regmap_init(void)
+{
+ return regmap_cell_init(&root_cell);
+}
+
+static unsigned int regmap_mmio_count_regions(struct cell *cell)
+{
+ return cell->config->num_regmaps;
+}
+
+DEFINE_UNIT_SHUTDOWN_STUB(regmap);
+DEFINE_UNIT(regmap, "regmap");
--
2.17.1

[nikh...@ti.com]

From: Nikhil Devshatwar <nikh...@ti.com>

Add regmaps to cover PADCONFIG registers in CTRL_MMR in J721e.
Describe the ownership for PAD registers in bitmap.
Enable serdes and UART control registers.

Signed-off-by: Nikhil Devshatwar <nikh...@ti.com>
---

configs/arm64/k3-j721e-evm-linux-demo.c | 25 +++++++++++++++++++++++++
configs/arm64/k3-j721e-evm.c | 15 +++++++++++++++
2 files changed, 40 insertions(+)

diff --git a/configs/arm64/k3-j721e-evm-linux-demo.c b/configs/arm64/k3-j721e-evm-linux-demo.c
index 47ad32ea..40b7aff8 100644
--- a/configs/arm64/k3-j721e-evm-linux-demo.c
+++ b/configs/arm64/k3-j721e-evm-linux-demo.c
@@ -28,6 +28,8 @@ struct {

struct jailhouse_irqchip irqchips[4];
struct jailhouse_pci_device pci_devices[1];
__u32 stream_ids[2];

+ struct jailhouse_regmap regmaps[1];
+
} __attribute__((packed)) config = {
.cell = {
.signature = JAILHOUSE_CELL_DESC_SIGNATURE,
@@ -40,6 +42,7 @@ struct {
.num_irqchips = ARRAY_SIZE(config.irqchips),
.num_pci_devices = ARRAY_SIZE(config.pci_devices),
.num_stream_ids = ARRAY_SIZE(config.stream_ids),
+ .num_regmaps = ARRAY_SIZE(config.regmaps),
.cpu_reset_address = 0x0,
.vpci_irq_base = 195 - 32,
.console = {
@@ -288,4 +291,26 @@ struct {
/* Non PCIe peripherals */
0x0003, 0xf003,
},
+
+ .regmaps = {
+ /*
+ * offset = (CTRL_MMR address - reg_base) / 4 / 32
+ * bit = (CTRL_MMR address - reg_base) / 4 % 32
+ */
+
+ /* Partition7 */ {
+ .reg_base = 0x11c000,
+ .reg_size = 4,
+ .reg_count = 256,
+ .flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE,
+ /*
+ * sw10_button_pins_default => 0:[0]
+ * main_usbss1_pins_default => 4:[6]
+ */
+ .reg_bitmap = {
+ 0x00000001, 0x00000000, 0x00000000, 0x00000000,
+ 0x00000020, 0x00000000, 0x00000000, 0x00000000,
+ },
+ },
+ },
};
diff --git a/configs/arm64/k3-j721e-evm.c b/configs/arm64/k3-j721e-evm.c
index 4f9755a8..ac422143 100644
--- a/configs/arm64/k3-j721e-evm.c
+++ b/configs/arm64/k3-j721e-evm.c
@@ -23,6 +23,7 @@ struct {
struct jailhouse_irqchip irqchips[6];
struct jailhouse_pci_device pci_devices[1];
__u32 stream_ids[30];
+ struct jailhouse_regmap regmaps[1];
} __attribute__((packed)) config = {
.header = {
.signature = JAILHOUSE_SYSTEM_SIGNATURE,
@@ -89,6 +90,7 @@ struct {
.num_irqchips = ARRAY_SIZE(config.irqchips),
.num_pci_devices = ARRAY_SIZE(config.pci_devices),
.num_stream_ids = ARRAY_SIZE(config.stream_ids),
+ .num_regmaps = ARRAY_SIZE(config.regmaps),
.vpci_irq_base = 191 - 32,
},
},
@@ -398,4 +400,17 @@ struct {
/* PCI3 */
0x8100, 0x8101, 0x8102, 0x8103, 0x8104, 0x8105,
},
+
+ .regmaps = {
+ /* Partition7 */ {
+ .reg_base = 0x11c000,
+ .reg_size = 4,
+ .reg_count = 256,
+ .flags = JAILHOUSE_MEM_READ | JAILHOUSE_MEM_WRITE,
+ .reg_bitmap = {
+ 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff,
+ 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff,
+ },
+ },
+ },
};
--
2.17.1

[Nikhil Devshatwar]

On 27/01/20 7:26 pm, nikh...@ti.com wrote:

From: Nikhil Devshatwar <nikh...@ti.com>

Signed-off-by: Nikhil Devshatwar <nikh...@ti.com>

Ignore this patch, sent by mistake in the regmap series.
I will send this separately with proper commit message

Nikhil D

[Jan Kiszka]

Worthwhile to discuss, indeed. The key question for me is how well it
could map on other SoCs. Ralf, do you think it could be that simple,
based on your experiments? Or could we also face scenarios where we
would have to dispatch bits of a registers? We already do in some GIC
regs, clock gates may be another case.

Jan

[Ralf Ramsauer]

The question is what you try to achieve:

Jan, when you introduced subpaging, the goal was to allow to assign
devices to different domains, if multiple devices are located on the
same page. We can observe that pattern on many platforms, and subpaging
provides a "generic" solution.

So what do you try to achieve with subpaging on a byte-wise/bit-wise
granularity? Make a non-partitionable device partitionable? That will
only work for some rare cases.

However, those cases exist, and I think it can really be helpful.

> would have to dispatch bits of a registers? We already do in some GIC

Yes, unfortunately I can imagine such scenarios, and even bitwise
dispatching is not enough. Two examples:

1) Reset controllers. E.g., the Jetson TK1/TX1 have bitwise granularity
(cf. reset.png). Here we need bitwise granularity. But that's still
easy.

2) Clock controllers. The semantic of a bit in a register may depend on
the configuration of a higher-level clock. On the other hand,
the reconfiguration of a higher might affect lower-level clocks
across multiple cells.

Let me just share the thoughts we had back then...

So for 1), bitwise granularity would really be helpful. But keep in mind
that upstream drivers usually don't like that. What would you do, if
your inmate violates the permissions? Probably panic the cell. But by
default and in case of Linux, many drivers aren't aware that they only
see a subset of the device's functionality.

~Two years ago, I evaluated a similar approach. I ended up in rewriting
kernel drivers (with no benefit for upstream)...

And I see no easy way to solve 2) despite a constant, static (freeze)
configuration. That comes with a lot of drawbacks, as inmates aren't
able to dynamically reconfigure the device. In case of a clock device,
they aren't able to, e.g., change the speed of I2C, SPI, you name it.

We thought of paravirtualisation. In case of Jailhouse this is surely no
solution: It requires to provide an abstract device model together with
an implementation for thousands of different devices - no chance.

The third approach would be to implement the device's functionality in
the firmware in a partitionable manner, and use jailhouse only to
moderate access. Only feasible from the Jailhouse PoV.

HTH,
Ralf

[Nikhil Devshatwar]

The main intention here was not to partition peripheral devices, but just the registers
which are not really related to any device.

Most SoCs will have something like this where pinmux registers,
efuse registers, internal muxes, MAC addresses, and other config options are provided.

This series was intended to solve these kind of register partitioning.

However, those cases exist, and I think it can really be helpful.

would have to dispatch bits of a registers? We already do in some GIC

Yes, unfortunately I can imagine such scenarios, and even bitwise
dispatching is not enough. Two examples:

1) Reset controllers. E.g., the Jetson TK1/TX1 have bitwise granularity
   (cf. reset.png). Here we need bitwise granularity. But that's still
   easy.

2) Clock controllers. The semantic of a bit in a register may depend on
   the configuration of a higher-level clock. On the other hand,
   the reconfiguration of a higher might affect lower-level clocks

clocking, power management, reset should be handled via a common
software entity, It is designed considering virtualization in mind.
Typical clocks are trees, where you need to change multiple parent clocks and muxes.

I believe this problem cannot be and should not be solved by Hypervisors.

On TI k3 devices, DMSC (Device mangement and security controlled) does this job
and Linux drivers use firmware APIs to get this done.

Regards,
Nikhil D

[Jan Kiszka]

Can you point out another SoC that we support which would benefit from
this description method?

>
> This series was intended to solve these kind of register partitioning.

So, subpaging does not scale when we have a scattered access map, right?
But can we use this description method to replace subpaging? The latter
registers an contiguous mmio dispatch region, your approach additionally
checks within that region a bitmap. A subpage entry can handle up to
PAGE_SIZE-1, a regmap currently only 256 bytes. I wonder if we can
combine both, maybe expand the memory region to optionally refer to a
bitmap for finer-grained access control.

Jan

[Nikhil Devshatwar]

e.g. We support jetson-tk1 which uses Nvidia tegra124. It can benefit from this.
There are lots of pinctrl nodes in the mainline Linux kernel device tree[0] and [1]

Currently, the pinmux configuration is described in root cell device tree [2] but not available in inmate device tree [3]

[0] - https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/tegra124.dtsi#L334
[1] - https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/tegra124-jetson-tk1.dts#L83
[2] - https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/tegra124-jetson-tk1.dts#L1035
[3] - https://github.com/siemens/jailhouse/blob/master/configs/arm/dts/inmate-jetson-tk1.dts

With this regmap framwork, it is possible to define the pinmux node such that
inmate Linux kernel can configure the pinmux when running in Jailhouse cell.

This series was intended to solve these kind of register partitioning.

So, subpaging does not scale when we have a scattered access map, right?

Yes

But can we use this description method to replace subpaging? The latter
registers an contiguous mmio dispatch region, your approach additionally
checks within that region a bitmap. A subpage entry can handle up to
PAGE_SIZE-1, a regmap currently only 256 bytes.

regmap implentation can be changed to support 4k pages as well.
That way, a bitmap can be generated to pass to regmap. But there is additional overhead of checking the offset.
But yes, regmap can replace subpage, however, this change should be transparant without having to change the jailhouse_memory descriptions.

I wonder if we can
combine both, maybe expand the memory region to optionally refer to a
bitmap for finer-grained access control.

Yes, that is also a good option, This will increas the size of cell config though.

Regards,
Nikhil D

[Jan Kiszka]

I don't think so, at least not noteworthy.

One benefit of combining both would be benefiting from the more precise
access size control features of the memory region. The regmap only
supports one register size, memory region all of them. Also, we may save
quite a bit of registration and dispatching code in the hypervisor.

How about something like this:

/* when set, access_bitmap_base is used for subpages */
#define JAILHOUSE_MEM_ACCESS_BITMAP 0x0200

struct jailhouse_memory {
__u64 phys_start;
__u64 virt_start;
__u64 size;
__u32 flags;
__u32 access_bitmap_base;
} __attribute__((packed));

Then we would append access bitmaps to the config as needed, and each
access_bitmap_base would point into that data. The size of each bitmap
would be (jailhouse_memory.size + 7) / 8. A bit set in the bitmap means
access allowed. In theory, we would get away with just adding the bitmap
test to mmio_handle_subpage.

Jan

[Nikhil Devshatwar]

I like this as well.
Right now I am calling paging_destroy to make sure that the MMIO handler gets triggered.
This can be avoided if the mem_region itself can be definesd it as bitmap.

Ralf, what are your thoughts?

- Nikhil D

Jan

[Ralf Ramsauer]

Does the offset inside the page need to be the same for phys and virt?

>>     __u64 size;
>>     __u32 flags;
>>     __u32 access_bitmap_base;

We don't need access_bitmap_base.

When we iterate over all memory regions, we know the pointer to the next
bitmap. Initially, the pointer points to the end of the config. That's
all information we need. Together with the size member of the current
region, we can simply online calculate the location of the next bitmap
and forward the pointer.

For the bitmap: Ideally, the first entry of the bitmap is non-zero,
otherwise phys_start would be off, right?

>> } __attribute__((packed));
>>
>> Then we would append access bitmaps to the config as needed, and each
>> access_bitmap_base would point into that data. The size of each bitmap
>> would be (jailhouse_memory.size + 7) / 8. A bit set in the bitmap means
>> access allowed. In theory, we would get away with just adding the bitmap
>> test to mmio_handle_subpage.

Ack. I like this idea.

Bitmap size could also be aligned with the bit width of the target
architecture. But that's just a minor detail...

> I like this as well.
> Right now I am calling paging_destroy to make sure that the MMIO handler
> gets triggered.
> This can be avoided if the mem_region itself can be definesd it as bitmap.
>
> Ralf, what are your thoughts?

We can easily extend this idea to allow for bit wise granularity:

#define JAILHOUSE_MEM_ACCESS_BYTEMAP 0x0200
#define JAILHOUSE_MEM_ACCESS_BITMAP 0x0400

struct jailhouse_memory {
__u64 phys_start;
__u64 virt_start;
__u64 size;
__u32 flags;

} __attribute__((packed));

The length of the bitmap is either (.size + 7) / 8 in case of
JAILHOUSE_MEM_ACCESS_BYTEMAP and .size in case of
JAILHOUSE_MEM_ACCESS_BITMAP.

Ralf

[Jan Kiszka]

It should not: virt is where Jailhouse intercepts the guest accesses,
phys is where it mapped the real device internally in order to execute
those accesses. Practically, we don't exploit that feature so far, though.

>
>>>     __u64 size;
>>>     __u32 flags;
>>>     __u32 access_bitmap_base;
>
> We don't need access_bitmap_base.
>
> When we iterate over all memory regions, we know the pointer to the next
> bitmap. Initially, the pointer points to the end of the config. That's
> all information we need. Together with the size member of the current
> region, we can simply online calculate the location of the next bitmap
> and forward the pointer.

Yes, we may be able to generate that pointer while parsing the config
and store it in the runtime state of Jailhouse. But we want a pointer
when we need to process an access.

>
> For the bitmap: Ideally, the first entry of the bitmap is non-zero,
> otherwise phys_start would be off, right?

I don't get this yet.

That would be possible, true. Would we have a use case for BITMAP already?

Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

[Ralf Ramsauer]

Yes, simply store it while registering all areas.

>
>>
>> For the bitmap: Ideally, the first entry of the bitmap is non-zero,
>> otherwise phys_start would be off, right?
>
> I don't get this yet.

What I wanted to say is that we now have multiple ways of
representations of the same memory region. Ideally, we "normalise"
entries by some convention.

For example:

.flags = MEM_BITMAP,
.virt_start = 0x1000,
.size = 0x0010,
.bitmap = {0x00, 0x0f},

is logically equal to:

.flags = MEM_BITMAP,
.virt_start = 0x1008,
.size = 0x0008,
.bitmap = {0x0f},

is logically equal to:

.flags = MEM_BITMAP,
.virt_start = 0x100c,
.size = 0x0004,
.bitmap = {0x0f},

One convention could be, e.g.:
- virtual addresses should be 8-byte aligned
- the first entry of the bitmap must be non-zero

Rationale: We need one byte in the bitmap to represent 8 byte. If the
byte is non-zero, then the bitmap is compact.

At the moment -- no. I had one a few years ago :-)

Nevertheless, even if bit granularity isn't implemented right now, we
should keep in mind that we might want to implement it one day.

Ralf

>
> Jan
>

[Nikhil Devshatwar]

On 27/01/20 7:26 pm, nikh...@ti.com wrote:

Jan/Chase,

I was doing some more testing / debug with this.
I root caused that the paging_destroy call does not take effect.

I have the fix (7cffb9b7d54d "core: fix hugepage splitting in paging_destroy") from the next branch
Do we have one more bug causing the paging_destroy to be ignored?

Regards,
Nikhil D

[Jan Kiszka]

>> + * Copyright (c) 2019 Texas Instruments Incorporated -http://www.ti.com

>> + * Copyright (c) 2019 Texas Instruments Incorporated -http://www.ti.com

Can you be more specific about the scenario (mapping before
paging_destroy, parameters to paging_destroy)? Can you instrument the
paging_destroy to visualize what is happening?

Besides that, the paging_destroy here is supposed to be removed in a
newer version of the patch because we will model regmap as memory
region. But even without that, I would have voted for requiring the user
to avoid duplicate mappings.