Re: [PATCH 2/2] xen: rename wrong named pfn related variables

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.08.2021 07:25, Juergen Gross wrote:
> On 03.08.21 12:42, Jan Beulich wrote:
>> On 30.07.2021 11:00, Juergen Gross wrote:
>>> On 16.06.21 12:43, Juergen Gross wrote:
>>>> On 16.06.21 11:56, Jan Beulich wrote:
>>>>> On 16.06.2021 09:30, Juergen Gross wrote:
>>>>>> --- a/arch/x86/xen/p2m.c
>>>>>> +++ b/arch/x86/xen/p2m.c
>>>>>> @@ -95,8 +95,8 @@ unsigned long *xen_p2m_addr __read_mostly;
>>>>>>    EXPORT_SYMBOL_GPL(xen_p2m_addr);
>>>>>>    unsigned long xen_p2m_size __read_mostly;
>>>>>>    EXPORT_SYMBOL_GPL(xen_p2m_size);
>>>>>> -unsigned long xen_max_p2m_pfn __read_mostly;
>>>>>> -EXPORT_SYMBOL_GPL(xen_max_p2m_pfn);
>>>>>> +unsigned long xen_p2m_max_size __read_mostly;
>>>>>> +EXPORT_SYMBOL_GPL(xen_p2m_max_size);
>>>>>
>>>>> Instead of renaming the exported variable (which will break consumers
>>>>> anyway), how about dropping the apparently unneeded export at this
>>>>> occasion?
>>>>
>>>> Why do you think it isn't needed? It is being referenced via the inline
>>>> function __pfn_to_mfn() in arch/x86/include/asm/xen/page.h. And
>>>> __pfn_to_mfn() is used via lots of other inline functions and macros.
>>>>
>>>>> Further it looks to me as if xen_p2m_size and this variable
>>>>> were actually always kept in sync, so I'd like to put up the question
>>>>> of dropping one of the two.
>>>>
>>>> Hmm, should be possible, yes.
>>>
>>> Looking into this it seems this is not possible.
>>>
>>> xen_p2m_size always holds the number of p2m entries in the p2m table,
>>> including invalid ones at the end. xen_p2m_pfn_limit however contains
>>> the (rounded up) index after the last valid p2m entry.
>>
>> I'm afraid I can't follow:
>>
>> xen_build_dynamic_phys_to_machine() sets xen_p2m_size and then syncs
>> its value to what so far has been xen_max_p2m_pfn.
>>
>> xen_vmalloc_p2m_tree() sets xen_max_p2m_pfn and then syncs its value
>> to xen_p2m_size.
>>
>> I therefore can't see how the two values would hold different values,
>> except for the brief periods between updating one and then the other.
> 
> The brief period in xen_vmalloc_p2m_tree() is the problematic one. The
> different values are especially important for the calls of
> __pfn_to_mfn() during xen_rebuild_p2m_list().

I'm still in trouble: Such __pfn_to_mfn() invocations would, afaics,
occur only in the context of page directory entry manipulation. Isn't
it the case that all valid RAM is below xen_p2m_size, and hence no
use of

        else if (unlikely(pfn < xen_max_p2m_pfn))
                return get_phys_to_machine(pfn);

would occur during that time window? (We're still way ahead of SMP
init, so what other CPUs might do does not look to be of concern
here.)

Jan