Re: [PATCH 11/22] x86: add a boot option to enable and disable the direct map

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 11/01/2024 11:53, Jan Beulich wrote:
> On 11.01.2024 11:47, Elias El Yandouzi wrote:
> > On 22/12/2022 13:24, Jan Beulich wrote:
> > > That said, I think this change comes too early in the series, or there is
> > > something missing.
> > At first, I had the same feeling but looking at the rest of the series,
> > I can see that the option is needed in follow-up patches.
> >
> > > As said in reply to patch 10, while there the mapcache
> > > is being initialized for the idle domain, I don't think it can be used
> > > just yet. Read through mapcache_current_vcpu() to understand why I think
> > > that way, paying particular attention to the ASSERT() near the end.
> > Would be able to elaborate a bit more why you think that? I haven't been
> > able to get your point.
> Why exactly I referred to the ASSERT() there I can't reconstruct. The
> function as a whole looks problematic though when suddenly the idle
> domain also gains a mapcache. I'm sorry, too much context was lost
> from over a year ago; all of this will need looking at from scratch
> again whenever a new version was posted.
>
> > > In preparation of this patch here I think the mfn_to_virt() uses have to all
> > > disappear from map_domain_page(). Perhaps yet more strongly all
> > > ..._to_virt() (except fix_to_virt() and friends) and __va() have to
> > > disappear up front from x86 and any code path which can be taken on x86
> > > (which may simply mean purging all respective x86 #define-s, without
> > > breaking the build in any way).
> > I agree with you on that one. I think it is what we're aiming for in the
> > long term. However, as mentioned by Julien in the cover letter, the
> > series's name is a misnomer and I am afraid we won't be able to remove
> > all of them with this series. These helpers would still be used for
> > xenheap pages or when the direct map is enabled.
> Leaving a hazard of certain uses not having been converted, or even
> overlooked in patches going in at around the same time as this series?
> I view this as pretty "adventurous".
Until we get rid of the directmap completely (which is not the goal of 
this series), we will need to keep mfn_to_virt().
In fact the one you ask to remove in map_domain_page() will need to be 
replaced with function doing the same thing. The same for the code that 
will initially prepare the directmap. This to avoid impacting 
performance when the user still wants to use the directmap.
So are you just asking to remove most of the use and rename *_to_virt() 
to something that is more directmap specific (e.g. mfn_to_directmap_virt())?
Cheers,

--
Julien Grall