On 30.08.2024 13:01, [email protected] wrote: > On Wed, 2024-08-28 at 12:44 +0200, Jan Beulich wrote: >> On 28.08.2024 11:53, [email protected] wrote: >>> On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote: >>>>> >>>>> + >>>>> +/* >>>>> + * Direct access to xen_fixmap[] should only happen when {set, >>>>> + * clear}_fixmap() is unusable (e.g. where we would end up to >>>>> + * recursively call the helpers). >>>>> + */ >>>>> +extern pte_t xen_fixmap[]; >>>> >>>> I'm afraid I keep being irritated by the comment: What recursive >>>> use >>>> of >>>> helpers is being talked about here? I can't see anything >>>> recursive in >>>> this >>>> patch. If this starts happening with a subsequent patch, then you >>>> have >>>> two options: Move the declaration + comment there, or clarify in >>>> the >>>> description (in enough detail) what this is about. > We can't move declaration of xen_fixmap[] to the patch where > set_fixmap() will be introduced ( which uses PMAP for domain map page > infrastructure ) as xen_fixmap[] is used in the current patch. > > And we can't properly provide the proper description with the function > which will be introduced one day in the future ( what can be not good > too ). I came up with the following description in the comment above > xen_fixmap[] declaration: > /* > * Direct access to xen_fixmap[] should only happen when {set, > * clear}_fixmap() is unusable (e.g. where we would end up to > * recursively call the helpers). > * > * One such case is pmap_map() where set_fixmap() can not be used. > * It happens because PMAP is used when the domain map page > infrastructure > * is not yet initialized, so map_pages_to_xen() called by > set_fixmap() needs > * to map pages on demand, which then calls pmap() again, resulting > in a loop. > * Modification of the PTEs directly instead in arch_pmap_map(). > * The same is true for pmap_unmap(). > */ > > Could it be an option just to drop the comment for now at all as at the > moment there is no such restriction with the usage of > {set,clear}_fixmap() and xen_fixmap[]?
The comment isn't the right place to explain things here, imo. It's the patch description where unexpected aspects need shedding light on. >>> This comment is added because of: >>> ``` >>> void *__init pmap_map(mfn_t mfn) >>> ... >>> /* >>> * We cannot use set_fixmap() here. We use PMAP when the >>> domain map >>> * page infrastructure is not yet initialized, so >>> map_pages_to_xen() called >>> * by set_fixmap() needs to map pages on demand, which then >>> calls >>> pmap() >>> * again, resulting in a loop. Modify the PTEs directly >>> instead. >>> The same >>> * is true for pmap_unmap(). >>> */ >>> arch_pmap_map(slot, mfn); >>> ... >>> ``` >>> And it happens because set_fixmap() could be defined using generic >>> PT >>> helpers >> >> As you say - could be. If I'm not mistaken no set_fixmap() >> implementation >> exists even by the end of the series. Fundamentally I'd expect >> set_fixmap() >> to (possibly) use xen_fixmap[] directly. That in turn ... >> >>> so what will lead to recursive behaviour when when there is no >>> direct map: >> >> ... would mean no recursion afaict. Hence why clarification is needed >> as >> to what's going on here _and_ what's planned. > Yes, it is true. No recursion will happen in this case but if to look > at the implementation of set_fixmap() for other Arm or x86 ( but I am > not sure that x86 uses PMAP inside map_pages_to_xen() ) they are using > map_pages_to_xen(). There's no PMAP so far on x86. Jan
