On 07.05.2024 17:21, Elias El Yandouzi wrote:
> > On 20/02/2024 10:28, Jan Beulich wrote:
>>> On 16.01.2024 20:25, 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;
>>
>> The reason initializers are needed here is, aiui, the overly large scope
>> of these variables. For example ...
>
> Correct, is it just an observation or do you want me to do anything?
Where possible reducing the scope of variables would be preferred. Hence
why I ...
>>> @@ -708,22 +712,32 @@ int __init dom0_construct_pv(struct domain *d,
>>> v->arch.pv.event_callback_cs = FLAT_COMPAT_KERNEL_CS;
>>> }
>>>
>>> +#define UNMAP_MAP_AND_ADVANCE(mfn_var, virt_var, maddr) \
>>> +do { \
>>> + unmap_domain_page(virt_var); \
>>> + mfn_var = maddr_to_mfn(maddr); \
>>> + maddr += PAGE_SIZE; \
>>> + virt_var = map_domain_page(mfn_var); \
>>> +} while ( false )
>>> +
>>> if ( !compat )
>>> {
>>> maddr_to_page(mpt_alloc)->u.inuse.type_info = PGT_l4_page_table;
>>> - l4start = l4tab = __va(mpt_alloc); mpt_alloc += PAGE_SIZE;
>>> + UNMAP_MAP_AND_ADVANCE(l4start_mfn, l4start, mpt_alloc);
>>> + l4tab = l4start;
>>> clear_page(l4tab);
>>> - init_xen_l4_slots(l4tab, _mfn(virt_to_mfn(l4start)),
>>> - d, INVALID_MFN, true);
>>> - v->arch.guest_table = pagetable_from_paddr(__pa(l4start));
>>> + init_xen_l4_slots(l4tab, l4start_mfn, d, INVALID_MFN, true);
>>> + v->arch.guest_table = pagetable_from_mfn(l4start_mfn);
>>
>> ... looks to be required only here, while ...
>>
>>> }
>>> else
>>> {
>>> /* Monitor table already created by switch_compat(). */
>>> - l4start = l4tab = __va(pagetable_get_paddr(v->arch.guest_table));
>>> + l4start_mfn = pagetable_get_mfn(v->arch.guest_table);
>>> + l4start = l4tab = map_domain_page(l4start_mfn);
>>
>> ... in principle the use of the variable could be avoided here. Below
>> from here there's no further use of it.
... went into some detail towards that possibility.
>>> @@ -781,30 +797,34 @@ int __init dom0_construct_pv(struct domain *d,
>>>
>>> if ( compat )
>>> {
>>> - l2_pgentry_t *l2t;
>>> -
>>> /* Ensure the first four L3 entries are all populated. */
>>> for ( i = 0, l3tab = l3start; i < 4; ++i, ++l3tab )
>>> {
>>> if ( !l3e_get_intpte(*l3tab) )
>>> {
>>> maddr_to_page(mpt_alloc)->u.inuse.type_info =
>>> PGT_l2_page_table;
>>> - l2tab = __va(mpt_alloc); mpt_alloc += PAGE_SIZE;
>>> - clear_page(l2tab);
>>> - *l3tab = l3e_from_paddr(__pa(l2tab), L3_PROT);
>>> + UNMAP_MAP_AND_ADVANCE(l2start_mfn, l2start, mpt_alloc);
>>> + clear_page(l2start);
>>> + *l3tab = l3e_from_mfn(l2start_mfn, L3_PROT);
>>> }
>>
>> The updating of l2start is only conditional here, yet ...
>>
>>> if ( i == 3 )
>>> l3e_get_page(*l3tab)->u.inuse.type_info |= PGT_pae_xen_l2;
>>> }
>>>
>>> - l2t = map_l2t_from_l3e(l3start[3]);
>>> - init_xen_pae_l2_slots(l2t, d);
>>> - unmap_domain_page(l2t);
>>> + init_xen_pae_l2_slots(l2start, d);
>>
>> ... here you assume it points at the page referenced by the 3rd L3 entry.
>
> Hmm, I missed it when sending the revision and indeed it doesn't look
> correct.
>
>> Question is why the original code is being replaced here in the first
>> place: It was already suitably mapping the page in question.
>
> The code was already suitably mapping the pages in question. This patch
> doesn't aim to make any functional change, just to rework how the
> domheap pages are used. The goal of the series is to remove the mappings
> from the directmap, which means those pages needs to be mapped and
> unmapped when required.
But that doesn't address my question: If there's nothing wrong with the
earlier code, why does it need changing (right here)?
Jan
