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

[Eric Auger]

On 10/4/22 00:17, Gavin Shan wrote:
> Hi Eric,
>
> On 10/3/22 4:44 PM, Eric Auger wrote:
>> On 9/29/22 01:37, Gavin Shan wrote:
>>> 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.
>
> When the high memory region can't fit into the PA space, it is disabled
> by toggling the corresponding flag (vms->highmem_{redists, ecam, mmio})
> to false. It's part of the original implementation, as below. We either
> need a 'switch ... case' or a pointer. A pointer is more convenient since
> we need check and possibly update to the value.
>
>        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;
>         }
>
>>>
>>>>>        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.
>
> Ok. I will use my proposed change in next revision.
>
>>>
>>>>> +        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().
>>
>
> 'region_enabled' points to 'vms->highmem_{redist2, ecam, mmio}'. They
> are same thing.
Oh OK. Sorry for the noise.

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