Re: [PATCH v3 3/5] hw/arm/virt: Introduce variable region_base in virt_set_high_memmap()

[Gavin Shan <gshan@redhat.com>]

Hi Eric,

On 9/28/22 10:10 PM, Eric Auger wrote:
> On 9/22/22 01:13, Gavin Shan wrote:
>> This introduces variable 'region_base' for the base address of the
>> specific high memory region. It's the preparatory work to optimize
>> high memory region address assignment.
> Why is it a preparatory work (same comment for previous patch, ie [2/5]
> ). Are those changes really needed? why?
> 

In PATCH[4/5], @base argument is added to virt_set_high_memmap(), to
represent current global base address. With the optimization applied
in PATCH[4/5], @base isn't unconditionally updated to the top of the
iterated high memory region. So we need @region_base here (PATCH[3/5])
to track the aligned base address for the iterated high memory region,
which may or may be not updated to @base.

Since we have @region_base in PATCH[3/5], it'd better to have @region_size
in PATCH[2/5].

Actually, PATCH[1-3/5] are all preparatory patches for PATCH[4/5]. My
intention was to organize the patches in a way to keep the logical
change part simple enough, for easier review.

Thanks,
Gavin

>>
>> No functional change intended.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/arm/virt.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 187b3ee0e2..b0b679d1f4 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1692,15 +1692,15 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
>>   static void virt_set_high_memmap(VirtMachineState *vms,
>>                                    hwaddr base, int pa_bits)
>>   {
>> -    hwaddr region_size;
>> +    hwaddr region_base, region_size;
>>       bool fits;
>>       int i;
>>   
>>       for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
>> +        region_base = ROUND_UP(base, extended_memmap[i].size);
>>           region_size = extended_memmap[i].size;
>>   
>> -        base = ROUND_UP(base, region_size);
>> -        vms->memmap[i].base = base;
>> +        vms->memmap[i].base = region_base;
>>           vms->memmap[i].size = region_size;
>>   
>>           /*
>> @@ -1709,9 +1709,9 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>>            *
>>            * For each device that doesn't fit, disable it.
>>            */
>> -        fits = (base + region_size) <= BIT_ULL(pa_bits);
>> +        fits = (region_base + region_size) <= BIT_ULL(pa_bits);
>>           if (fits) {
>> -            vms->highest_gpa = base + region_size - 1;
>> +            vms->highest_gpa = region_base + region_size - 1;
>>           }
>>   
>>           switch (i) {
>> @@ -1726,7 +1726,7 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>>               break;
>>           }
>>   
>> -        base += region_size;
>> +        base = region_base + region_size;
>>       }
>>   }
>>   
>