Re: [PATCH v3 4/5] hw/arm/virt: Improve high memory region address assignment

[Gavin Shan <gshan@redhat.com>]

Hi Eric,

On 9/28/22 10:51 PM, Eric Auger wrote:
> On 9/22/22 01:13, Gavin Shan wrote:
>> There are three high memory regions, which are VIRT_HIGH_REDIST2,
>> VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses
>> are floating on highest RAM address. However, they can be disabled
>> in several cases.
>>
>> (1) One specific high memory region is disabled by developer by
>>      toggling vms->highmem_{redists, ecam, mmio}.
>>
>> (2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is
>>      'virt-2.12' or ealier than it.
>>
>> (3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded
>>      on 32-bits system.
>>
>> (4) One specific high memory region is disabled when it breaks the
>>      PA space limit.
>>
>> The current implementation of virt_set_memmap() isn't comprehensive
>> because the space for one specific high memory region is always
>> reserved from the PA space for case (1), (2) and (3). In the code,
>> 'base' and 'vms->highest_gpa' are always increased for those three
>> cases. It's unnecessary since the assigned space of the disabled
>> high memory region won't be used afterwards.
>>
>> This improves the address assignment for those three high memory
>> region by skipping the address assignment for one specific high
>> memory region if it has been disabled in case (1), (2) and (3).
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/arm/virt.c | 44 ++++++++++++++++++++++++++------------------
>>   1 file changed, 26 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index b0b679d1f4..b702f8f2b5 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1693,15 +1693,31 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>>                                    hwaddr base, int pa_bits)
>>   {
>>       hwaddr region_base, region_size;
>> -    bool fits;
>> +    bool *region_enabled, fits;
> IDo you really need a pointer? If the region is unknown this is a bug in
> virt code.

The pointer is needed so that we can disable the region by setting 'false'
to it at later point. Yeah, I think you're correct that 'unknown region'
is a bug and we need to do assert(region_enabled), or something like below.

>>       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;
>>   
>> -        vms->memmap[i].base = region_base;
>> -        vms->memmap[i].size = region_size;
>> +        switch (i) {
>> +        case VIRT_HIGH_GIC_REDIST2:
>> +            region_enabled = &vms->highmem_redists;
>> +            break;
>> +        case VIRT_HIGH_PCIE_ECAM:
>> +            region_enabled = &vms->highmem_ecam;
>> +            break;
>> +        case VIRT_HIGH_PCIE_MMIO:
>> +            region_enabled = &vms->highmem_mmio;
>> +            break;
> While we are at it I would change the vms fields dealing with those
> highmem regions and turn those fields into an array of bool indexed
> using i - VIRT_LOWMEMMAP_LAST (using a macro or something alike). We
> would not be obliged to have this switch, now duplicated.

It makes sense to me. How about to have something like below in v4?

static inline bool *virt_get_high_memmap_enabled(VirtMachineState *vms, int index)
{
     bool *enabled_array[] = {
           &vms->highmem_redists,
           &vms->highmem_ecam,
           &vms->highmem_mmio,
     };

     assert(index - VIRT_LOWMEMMAP_LAST < ARRAY_SIZE(enabled_array));

     return enabled_array[index - VIRT_LOWMEMMAP_LAST];
}

>> +        default:
>> +            region_enabled = NULL;
>> +        }
>> +
>> +        /* Skip unknown region */
>> +        if (!region_enabled) {
>> +            continue;
>> +        }
>>   
>>           /*
>>            * Check each device to see if they fit in the PA space,
>> @@ -1710,23 +1726,15 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>>            * For each device that doesn't fit, disable it.
>>            */
>>           fits = (region_base + region_size) <= BIT_ULL(pa_bits);
>> -        if (fits) {
>> -            vms->highest_gpa = region_base + region_size - 1;
>> -        }
>> +        if (*region_enabled && fits) {
>> +            vms->memmap[i].base = region_base;
>> +            vms->memmap[i].size = region_size;
>>   
>> -        switch (i) {
>> -        case VIRT_HIGH_GIC_REDIST2:
>> -            vms->highmem_redists &= fits;
>> -            break;
>> -        case VIRT_HIGH_PCIE_ECAM:
>> -            vms->highmem_ecam &= fits;
>> -            break;
>> -        case VIRT_HIGH_PCIE_MMIO:
>> -            vms->highmem_mmio &= fits;
>> -            break;
>> +            vms->highest_gpa = region_base + region_size - 1;
>> +            base = region_base + region_size;
>> +        } else {
>> +            *region_enabled = false;
>>           }
>> -
>> -        base = region_base + region_size;
>>       }
>>   }
>>   

Thanks,
Gavin