Re: [PATCH v2 11/40] xen/mpu: build up start-of-day Xen MPU memory region map

From: Julien Grall <julien@xen.org>
To: Penny Zheng <Penny.Zheng@arm.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Wei Chen <Wei.Chen@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	"ayan.kumar.halder@xilinx.com" <ayan.kumar.halder@xilinx.com>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v2 11/40] xen/mpu: build up start-of-day Xen MPU memory region map
Date: Tue, 31 Jan 2023 09:27:36 +0000	[thread overview]
Message-ID: <509eafac-bbe7-ed18-ce11-3fede7ca691d@xen.org> (raw)
In-Reply-To: <AM0PR08MB45309F6DCB1B1E0975A741B7F7D09@AM0PR08MB4530.eurprd08.prod.outlook.com>

On 31/01/2023 04:11, Penny Zheng wrote:
> Hi Julien
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Monday, January 30, 2023 5:40 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> Subject: Re: [PATCH v2 11/40] xen/mpu: build up start-of-day Xen MPU
>> memory region map
>>
>> Hi Penny,
>>
>> On 30/01/2023 05:45, Penny Zheng wrote:
>>>    There are three types of MPU regions during boot-time:
>>> 1. Fixed MPU region
>>> Regions like Xen text section, Xen heap section, etc.
>>> 2. Boot-only MPU region
>>> Regions like Xen init sections, etc. It will be removed at the end of booting.
>>> 3.   Regions need switching in/out during vcpu context switch
>>> Region like system device memory map.
>>> For example, for FVP_BaseR_AEMv8R, we have [0x80000000, 0xfffff000) as
>>> the whole system device memory map for Xen(idle vcpu) in EL2,  when
>>> context switching to guest vcpu, it shall be replaced with
>>> guest-specific device mapping, like vgic, vpl011, passthrough device, etc.
>>>
>>> We don't have two mappings for different stage translations in MPU, like
>> we had in MMU.
>>> Xen stage 1 EL2 mapping and stage 2 mapping are both sharing one MPU
>>> memory mapping(xen_mpumap) So to save the trouble of hunting down
>> each
>>> switching regions in time-sensitive context switch, we must re-order
>> xen_mpumap to keep fixed regions in the front, and switching ones in the
>> heels of them.
>>
>>   From my understanding, hunting down each switching regions would be a
>> matter to loop over a bitmap. There will be a slight increase in the number
>> of instructions executed, but I don't believe it will be noticeable.
>>
>>>
>>> In Patch Serie v1, I was adding MPU regions in sequence,  and I
>>> introduced a set of bitmaps to record the location of same type
>>> regions. At the end of booting, I need to *disable* MPU to do the
>> reshuffling, as I can't move regions like xen heap while MPU on.
>>>
>>> And we discussed that it is too risky to disable MPU, and you
>>> suggested [1] "
>>>> You should not need any reorg if you map the boot-only section
>>>> towards in the higher slot index (or just after the fixed ones).
>>> "
>>
>> Right, looking at the new code. I realize that this was probably a bad idea
>> because we are adding complexity in the assembly code.
>>
>>>
>>> Maybe in assembly, we know exactly how many fixed regions are,
>>> boot-only regions are, but in C codes, we parse FDT to get static
>> configuration, like we don't know how many fixed regions for xen static
>> heap is enough.
>>> Approximation is not suggested in MPU system with limited MPU regions,
>>> some platform may only have 16 MPU regions, IMHO, it is not worthy
>> wasting in approximation.
>>
>> I haven't suggested to use approximation anywhere here. I will answer
>> about the limited number of entries in the other thread.
>>
>>>
>>> So I take the suggestion of putting regions in the higher slot index.
>>> Putting fixed regions in the front, and putting boot-only and
>>> switching ones at tail. Then, at the end of booting, when we reorg the
>> mpu mapping, we remove all boot-only regions, and for switching ones, we
>> disable-relocate(after fixed ones)-enable them. Specific codes in [2].
>>
>>   From this discussion, it feels to me that you are trying to make the code
>> more complicated just to keep the split and save a few cycles (see above).
>>
>> I would suggest to investigate the cost of "hunting down each section".
>> Depending on the result, we can discuss what the best approach.
>>
> 
> Correct me if I'm wrong, the complicated things in assembly you are worried about
> is that we couldn't define the index for initial sections, no hardcoded to keep simple.

Correct.

> And function write_pr, ik, is really a big chunk of codes, however the logic is simple there,
> just a bunch of "switch-cases".

I agree that write_pr() is a bunch of switch-cases. But there are a lot 
of duplication in it and the interface to use it is, IMHO, not intuitive.

> 
> If we are adding MPU regions in sequence as you suggested, while using bitmap at the
> same time to record used entry.
> TBH, this is how I designed at the very beginning internally. We found that if we don't
> do reorg late-boot to keep fixed in front and switching ones after, each time when we
> do vcpu context switch, not only we need to hunt down switching ones to disable,
> while we add new switch-in regions, using bitmap to find free entry is saying that the
> process is unpredictable. Uncertainty is what we want to avoid in Armv8-R architecture.

I don't understand why it would be unpredictable. For a given 
combination of platform/device-tree, the bitmap will always look the 
same. So the number of cycles/instructions will always be the same.

This is not very different from the case where you split the MPU in two 
because

> 
> Hmmm, TBH, I really really like your suggestion to put boot-only/switching regions into
> higher slot. It really saved a lot trouble in late-init reorg and also avoids disabling MPU
> at the same time. The split is a simple and easy-to-understand construction compared
> with bitmap too.

I would like to propose another split. I will reply to that in the 
thread where you provided the MPU layout.

Cheers,

-- 
Julien Grall