Re: [PATCH V3 (resend) 03/19] x86/pv: Rewrite how building PV dom0 handles domheap mappings

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.07.2024 18:12, Elias El Yandouzi wrote:
> Hi Jan,
> 
> On 14/05/2024 16:03, Jan Beulich wrote:
>> On 13.05.2024 15:40, Elias El Yandouzi wrote:
>>> --- a/xen/arch/x86/pv/dom0_build.c
>>> +++ b/xen/arch/x86/pv/dom0_build.c
>>> @@ -382,6 +382,10 @@ int __init dom0_construct_pv(struct domain *d,
>>>       l3_pgentry_t *l3tab = NULL, *l3start = NULL;
>>>       l2_pgentry_t *l2tab = NULL, *l2start = NULL;
>>>       l1_pgentry_t *l1tab = NULL, *l1start = NULL;
>>> +    mfn_t l4start_mfn = INVALID_MFN;
>>> +    mfn_t l3start_mfn = INVALID_MFN;
>>> +    mfn_t l2start_mfn = INVALID_MFN;
>>> +    mfn_t l1start_mfn = INVALID_MFN;
>>
>> Just to mention it here again: By limiting the scope of these I'm pretty
>> sure no initializer would be needed even "just in case" (really I don't
>> think they're needed even when the all have function scope, as producer
>> and consumer are always close together afaics; quite different from
>> l<N>start and l<N>tab).
> 
> I had a closer look at your suggestion and I don't think it is possible, 
> especially for l3start_mfn.
> 
> The variable, l3start_mfn, can get initialized in the else leg of the 
> first if statement along with l3start variable.
> 
> If you look a few lines below in the for loop, we call 
> l4e_from_mfn(l3start_mfn, L4_PROT) which assumes l3start_mfn is valid. 
> It could not be the case if we took the first leg of the aforementioned 
> if statement.
> 
> I don't think I can this easily limit their scope. It could work for the 
> others, but not l3start_mfn. So I can either leave things as they are or 
> limit the scope of every variables but l3start_mfn.

Please do. Limiting the scope of variables, especially in larger functions,
is often quite helpful. The one exception is certainly necessary here, I
agree.

Jan