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

[Elias El Yandouzi <eliasely@xxxxxxxxxx>]

On 11/01/2024 14:09, Jan Beulich wrote:
> On 11.01.2024 13:25, Julien Grall wrote:
> > 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())?
> Well, in a way. If done this way, mfn_to_virt() (and __va()) should have no
> users by the end of the series, and it would be obvious that nothing was
> missed (and by then purging the old ones we could also ensure no new uses
> would appear).
What about maddr_to_virt()? For instance, in the function 
xen/arch/x86/dmi_scan.c:dmi_iterate(), we need to access a very low 
machine address which isn't in the directmap range.
How would you proceed? Calling vmap() seems to be a bit overkill for 
just a temporary mapping and I don't really want to rework this function 
to use map_domain_page().
In such case, how would you proceed? What do you suggest?

Cheers,

--
Elias