Hi Ayan,
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_PRESENT flag indicates intent - whether the caller intends to create
or remove a region. 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.
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.
That said, I believe the current logic is sound - the early return path should
only be hit if the region is already known to be disabled both in software and
hardware. This function assumes it is the sole point of disabling regions, so
the check should be reliable.
Many Thanks,
Hari
