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?
>> __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
--
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/7be98c82-d415-8215-d35c-11241d3727cd%40oth-regensburg.de.
