Re: [PATCH 10/10] arm/mpu: Create the skeleton for MPU compilation

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 14/03/2025 09:38, Luca Fancellu wrote:
> 
> 
> Hi Michal,
> 
>> On 14 Mar 2025, at 08:27, Orzel, Michal <michal.orzel@xxxxxxx> wrote:
>>
>>
>>
>> On 12/03/2025 14:52, Luca Fancellu wrote:
>>>
>>>
>>> This commit introduces the skeleton for the MPU memory management
>>> subsystem that allows the compilation.
>> You forgot to mention that you're talking about Arm64 only. For Arm32, Ayan 
>> has
>> a series containing a few patches to enable compilation.
> 
> Yes I’ll be more specific here that we enable arm64 MPU build
> 
>>
>> P.S.
>> Once your series are merged, I'll send a patch enabling CI compilation for 
>> both
>> Arm64 and Arm32 MPU.
> 
> thanks a lot for that
> 
> 
>>>
>>> +
>>> +#include <xen/mm.h>
>> I can't see anything mm.h related here. I'd expect types.h/stdbool.h and 
>> bug.h
> 
> I’ll add
> 
>>
>>> +
>>> +int prepare_secondary_mm(int cpu)
>>> +{
>>> +    BUG_ON("unimplemented");
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +void update_boot_mapping(bool enable)
>>> +{
>>> +    BUG_ON("unimplemented");
>>> +}
>>> +
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-file-style: "BSD"
>>> + * c-basic-offset: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>>> diff --git a/xen/arch/arm/include/asm/mpu/p2m.h 
>>> b/xen/arch/arm/include/asm/mpu/p2m.h
>>> new file mode 100644
>>> index 000000000000..e5c0e302167c
>>> --- /dev/null
>>> +++ b/xen/arch/arm/include/asm/mpu/p2m.h
>>> @@ -0,0 +1,20 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> Empty line here please
>>
>>> +#ifndef __ARM_MPU_P2M_H__
>>> +#define __ARM_MPU_P2M_H__
>>> +
>>> +/* Not used on MPU system */
>> I personally don't see a value in such comments. Initially when there were 
>> just
>> 1, 2 instances I was ok, but then I realized you keep adding them. It's 
>> pretty
>> clear that if a stub does not have BUG_ON, it means it's not used. I worry 
>> the
>> files will look awful with so many "Not used on MPU system" comments. If at 
>> all,
>> the only place they would make sense is if they were in MMU/MPU common code. 
>> Can
>> we get rid of them completely?
> 
> Sure, I’ll get rid of them
> 
>>
>>> +static inline void p2m_clear_root_pages(struct p2m_domain *p2m) {}
>> You should at least forward declare struct p2m_domain.
> 
> Ok I’ll fix, should I include asm/p2m.h for the visibility of this structure 
> or
> because we won’t use that I should just forward declare?
forward declare

> 
>>>
>>> static inline bool arch_acquire_resource_check(struct domain *d)
>>> diff --git a/xen/arch/arm/mpu/Makefile b/xen/arch/arm/mpu/Makefile
>>> index f1417cd1b9db..a963b35db88d 100644
>>> --- a/xen/arch/arm/mpu/Makefile
>>> +++ b/xen/arch/arm/mpu/Makefile
>>> @@ -1,3 +1,4 @@
>>> obj-y += mm.o
>>> obj-y += p2m.o
>>> obj-y += setup.init.o
>>> +obj-y += vmap-mpu.o
>> What's the point of adding -mpu suffix to the file located already under 
>> /mpu?
> 
> ok I’ll rename to vmap
> 
> 
>>> diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
>>> index 45214bfeb661..28119e008341 100644
>>> --- a/xen/arch/arm/mpu/setup.c
>>> +++ b/xen/arch/arm/mpu/setup.c
>>> @@ -2,12 +2,36 @@
>>>
>>> #include <xen/bug.h>
>>> #include <xen/init.h>
>>> +#include <xen/types.h>
>>> +#include <asm/setup.h>
>> Please sort alphabetically
> 
> Isn’t xen/* first followed by asm/* ?
Oops, I missed the fact that this is xen/ and asm/. Ignore then.

~Michal