Re: [PATCH v1 3/5] physmem: xen: Conditionalize use of the mapcache

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 5 Mar 2026, Edgar E. Iglesias wrote:
> On Wed, Mar 04, 2026 at 04:07:23PM -0800, Stefano Stabellini wrote:
> > On Wed, 4 Mar 2026, Edgar E. Iglesias wrote:
> > > From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxx>
> > > 
> > > Conditionalize use of the mapcache. This is in preparation
> > > to optionally disable the mapcache at runtime.
> > > 
> > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxx>
> > > ---
> > >  system/physmem.c | 13 +++++++------
> > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/system/physmem.c b/system/physmem.c
> > > index e5ff26acec..64e6d50f8f 100644
> > > --- a/system/physmem.c
> > > +++ b/system/physmem.c
> > > @@ -578,7 +578,8 @@ MemoryRegion *flatview_translate(FlatView *fv, hwaddr 
> > > addr, hwaddr *xlat,
> > >                                      is_write, true, &as, attrs);
> > >      mr = section.mr;
> > >  
> > > -    if (xen_enabled() && memory_access_is_direct(mr, is_write, attrs)) {
> > > +    if (xen_map_cache_enabled() &&
> > > +        memory_access_is_direct(mr, is_write, attrs)) {
> > >          hwaddr page = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - 
> > > addr;
> > >          *plen = MIN(page, *plen);
> > >      }
> > 
> > All the other changes make sense. For this one, the code inside the if
> > check is not strictly related to the mapcache. Are you sure it should be
> > changed?
> 
> Hi, yes, when the mapcache is on, we limit translations to the current page
> because the next page might not be mapped/locked or could be in a different
> mapcache bucket with a different virtual address. When mapcache is off, guest
> RAM is fully mapped (same as non‑Xen), so we can skip capping plen and allow a
> larger mapping.
> 
> Perhaps we should add a comment to clarify?

Yes please! Add a one-line in-code comment, and with that:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>