Re: [PATCH v4 5/6] hw/arm/virt: Improve high memory region address

[Gavin Shan <gshan@redhat.com>]

Hi Connie,

On 10/4/22 6:53 PM, Cornelia Huck wrote:
> On Tue, Oct 04 2022, Gavin Shan <gshan@redhat.com> 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).
>> 'vms->high_compact' is false for now, meaning that we don't have
>> any behavior changes until it becomes configurable through property
>> 'compact-highmem' in next patch.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/arm/virt.c         | 19 ++++++++++++-------
>>   include/hw/arm/virt.h |  1 +
>>   2 files changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 59de7b78b5..4164da49e9 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1715,9 +1715,6 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>>           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;
>> -
>>           /*
>>            * Check each device to see if they fit in the PA space,
>>            * moving highest_gpa as we go.
> 
> Maybe tweak this comment?
> 
> "Check each enabled device to see if they fit in the PA space,
> moving highest_gpa as we go. For compatibility, move highest_gpa
> for disabled fitting devices as well, if the compact layout has
> been disabled."
> 
> (Or would that be overkill?)
> 

It looks overkill to me since the code is simple and clear. However,
comments won't be harmful. I will integrate the proposed comment
in next respin.

Thanks,
Gavin