[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 11/22] x86: add a boot option to enable and disable the direct map
On 11.01.2024 19:25, Elias El Yandouzi wrote: > > > 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. I'm afraid I don't follow: Very low addresses are always in the direct map range, which - on x86 - always starts at 0. > 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? fixmap may be an option to consider, but I also don't see why you apparently think using vmap() would be a possibility while at the same time making use of map_domain_page() is too much effort. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |