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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.05.2024 18:49, Roger Pau Monné wrote:
> On Mon, May 13, 2024 at 01:40:30PM +0000, Elias El Yandouzi wrote:
>> @@ -710,22 +714,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);                \
> 
> FWIW, I would do the advance after the map, so that the order matches
> the name of the function.

Actually I was thinking kind of the same when looking at v3, even if I
may not have commented to that effect. Then again that goes somewhat
against the further suggestion below.

>> +} 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;
> 
> You could even make the macro return virt_var, and so use it like:
> 
> l4tab = l4start = UNMAP_MAP_AND_ADVANCE(l4start_mfn, mpt_alloc);
> 
> ?

Not quite, l4start also need to be an input to the macro:

    l4tab = l4start = UNMAP_MAP_AND_ADVANCE(l4start_mfn, l4start, mpt_alloc);

Else unmap_domain_page() has nothing to act upon. If anything that would
then (imo) likely better be

    l4tab = UNMAP_MAP_AND_ADVANCE(l4start_mfn, l4start, mpt_alloc);

with l4start still updated inside the macro.

Jan