On 30/01/2020 15:11, Jan Kiszka wrote:
> On 30.01.20 14:38, Ralf Ramsauer wrote:
>> On 28/01/2020 09:09, 'Nikhil Devshatwar' via Jailhouse wrote:
>>>
>>>
>>> On 28/01/20 2:41 am, Jan Kiszka wrote:
>>>> On 27.01.20 20:41, Nikhil Devshatwar wrote:
>>>>>
>>>>>
>>>>> On 27/01/20 10:03 pm, Jan Kiszka wrote:
>>>>>> On 27.01.20 17:13, Nikhil Devshatwar wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 27/01/20 9:30 pm, Ralf Ramsauer wrote:
>>>>>>>> On 27/01/2020 15:49, Jan Kiszka wrote:
>>>>>>>>> On 27.01.20 14:56, nikhil.nd via Jailhouse wrote:
>>>>>>>>>> From: Nikhil Devshatwar<[email protected]>
>>>>>>>>>>
>>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>> 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
>>>>>>>> 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.
>>>>>>> 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.
>>>>>>
>>>>>> Can you point out another SoC that we support which would benefit
>>>>>> from
>>>>>> this description method?
>>>>>>
>>>>>
>>>>> 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.
>>>>
>>>> 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;
>>
>> Does the offset inside the page need to be the same for phys and virt?
>
> 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.
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.
>
>>
>>>> } __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.
>>
>
> That would be possible, true. Would we have a use case for BITMAP already?
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
>
--
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/a4621641-750c-f539-2e02-5eb60af843a1%40oth-regensburg.de.
