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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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?

>> 
>> 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/* ?

Cheers,
Luca