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
