Re: [PATCH 4/6] arm/mpu: Destroy an existing entry in Xen MPU memory mapping table

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 01/07/2025 15:56, Hari Limaye wrote:
> Hi Ayan,
Hi Hari,
> Thank you for the review. I have just a couple of clarifications before I
>
> re-spin the series to address all the comments:
>
> > > -    if ( flags & _PAGE_PRESENT )
>
> > > +    if ( (flags & _PAGE_PRESENT) && (MPUMAP_REGION_NOTFOUND == rc) )
>
> >
>
> > Same question in this patch , why do we need to check for _PAGE_PRESENT.
>
> > Can't we just rely on MPUMAP_REGION_XXX ?
>
> The _PAGE_PRESENTflag indicates intent - whether the caller intends to 
> create
> or remove a region.
If so, then I misunderstood the code. However, looking at 
xen_pt_check_entry(), it seems _PAGE_PRESENTindicates if the page table 
entry exists. If so, using _PAGE_PRESENTis not making sense  to me 
atleast. May be others can chime in.
> The MPUMAP_REGION_XXX values only describe the current state
>
> of the MPU map - whether the region already exists - not the caller's 
> purpose.
> For example, if the function is called to remove a region and it turns 
> out to be
> MPUMAP_REGION_NOTFOUND, we don't want to accidentally create it.
>
> The flag is passed via the `flags` argument in calls to 
> `map_pages_to_xen()`.
> In this patch:
>
> https://patchwork.kernel.org/project/xen-devel/patch/deccb1566ced5fa64f6de5c988ab968b76dc945a.1750411205.git.hari.limaye@xxxxxxx/
>
> `flags` is set to PAGE_HYPERVISOR_RO, and as defined in
>
> xen/arch/arm/include/asm/page.h, this includes _PAGE_PRESENT.
>
> It is also used in a similar way in `xen_pt_update_entry()` in
>
> xen/arch/arm/mmu/pt.c.
>
> > > +static void disable_mpu_region_from_index(uint8_t index)
>
> > > +{
>
> > > + ASSERT(spin_is_locked(&xen_mpumap_lock));
>
> > > +    ASSERT(index != INVALID_REGION_IDX);
>
> > > +
>
> > > +    if ( !region_is_valid(&xen_mpumap[index]) )
>
> > > +    {
>
> > > +        printk(XENLOG_WARNING
>
> > > +               "mpu: MPU memory region[%u] is already disabled\n", 
> index);
> > > +        return;
>
> > > +    }
>
> > > +
>
> > > +    /* Zeroing the region will also zero the region enable */
>
> > > +    memset(&xen_mpumap[index], 0, sizeof(pr_t));
>
> > > +    clear_bit(index, xen_mpumap_mask);
>
> >
>
> > NIT.
>
> >
>
> > These 2 lines we can move before the if { ..}. So that the region is
>
> > zeroed even if the region is disabled. This will add a small overhead,
>
> > but we will be sure that the region is zeroed whenever it is disabled.
>
> Thank you for the suggestion - just to clarify, unless I am missing 
> something
> I think that moving those lines above the if block would actually 
> break the
> logic.
Ah my mistake. I meant these two lines should be moved *inside* the if 
{...} condition.
This is a minor suggestion so feel free to ignore.

> The memset() call zeroes the region in the xen_mpumap data structure, 
> and this
> is what region_is_valid() inspects. So if we zero the region before 
> calling
> region_is_valid(), that check will always fail, and we would exit 
> early without
> updating the hardware.
>
> It’s the subsequent lines that actually write the region to the MPU 
> register. So
> if we exit early, the hardware MPU state remains unchanged.
All good.

- Ayan