Re: [PATCH V3 (resend) 14/19] Rename mfn_to_virt() calls

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, May 14, 2024 at 06:22:59PM +0200, Jan Beulich wrote:
> On 14.05.2024 17:45, Roger Pau Monné wrote:
> > On Mon, May 13, 2024 at 01:40:41PM +0000, Elias El Yandouzi wrote:
> >> Until directmap gets completely removed, we'd still need to
> >> keep some calls to mfn_to_virt() for xenheap pages or when the
> >> directmap is enabled.
> >>
> >> Rename the macro to mfn_to_directmap_virt() to flag them and
> >> prevent further use of mfn_to_virt().
> > 
> > Both here and in the following patch, I'm afraid I'm unsure of it's
> > usefulness.  I'm leaning towards this being code churn for very little
> > benefit.
> 
> I expect this patch is a response to an earlier comment of mine. I'm
> rather worried that at the time this series actually goes in, un-audited
> mfn_to_virt() uses remain (perhaps because of introduction between patch
> submission and its committing). Such uses would all very likely end in
> crashes or worse, but they may not be found by testing.

I see, would be good to note the intention on the commit message then.

> > Also, I'm not sure I see how the patch prevents further usage of
> > mfn_to_virt(), as (for Arm) the existing macro is not removed.  If
> > anything I would prefer a comment clearly stating that the macro
> > operates on directmap space, and avoid the name change.
> 
> But Arm isn't switched to this sparse direct map mode, I think? At which
> point uses in Arm-specific code continue to be okay.

Right, it's just that Arm will have both mfn_to_virt() and
mfn_to_directmap_virt() which seems a bit confusing when they are
actually the same implementation.

Thanks, Roger.