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

[Eric Auger]

Hi Gavin,

On 9/29/22 01:37, Gavin Shan wrote:
> 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.
Yeah I don't think using a pointer here is useful.
>
>>>       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];
> } 
I was rather thinking as directly using a vms->highmem_flags[] but your
proposal may work as well.
>
>>> +        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;
what's the purpose to update the region_enabled here? Is it used anywhere?

The fact you do not update vms->highmem_* flags may introduce
regressions I think as the resulting flag may be used in some places
such as:
virt_gicv3_redist_region_count().

>>> -
>>> -        base = region_base + region_size;
>>>       }
>>>   }
>>>   
>
> Thanks,
> Gavin
>
Thanks

Eric