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

[Cornelia Huck <cohuck@redhat.com>]

On Wed, Oct 26 2022, Gavin Shan <gshan@redhat.com> wrote:

> Hi Eric,
>
> On 10/26/22 12:29 AM, Eric Auger wrote:
>> On 10/24/22 05:54, 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 likely to be disabled by
>>>      code 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, high_memmap}() isn't
>>> optimized because the high memory region's PA space is always reserved,
>>> regardless of whatever the actual state in the corresponding
>>> vms->highmem_{redists, ecam, mmio} flag. In the code, 'base' and
>>> 'vms->highest_gpa' are always increased for case (1), (2) and (3).
>>> It's unnecessary since the assigned PA space for the disabled high
>>> memory region won't be used afterwards.
>>>
>>> Improve 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). The memory layout may
>>> be changed after the improvement is applied, which leads to potential
>>> migration breakage. So 'vms->highmem_compact' is added to control if
>>> the improvement should be applied. For now, 'vms->highmem_compact' is
>>> set to false, meaning that we don't have memory layout change until it
>>> becomes configurable through property 'compact-highmem' in next patch.
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> the code has quite changed since Connie's R-b
>
> Right. Connie, could you please check if the changes make sense to you
> and I can regain your R-B? :)

My R-b still holds.

>
>>> Tested-by: Zhenyu Zhang <zhenyzha@redhat.com>
>>> ---
>>>   hw/arm/virt.c         | 15 ++++++++++-----
>>>   include/hw/arm/virt.h |  1 +
>>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index ee98a8a3b6..4896f600b4 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -1721,18 +1721,23 @@ static void virt_set_high_memmap(VirtMachineState *vms,
>>>           vms->memmap[i].size = region_size;
>>>   
>>>           /*
>>> -         * Check each device to see if they fit in the PA space,
>>> -         * moving highest_gpa as we go.
>>> +         * Check each device to see if it fits 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.
>>>            *
>>>            * 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;
>>> +        *region_enabled &= fits;
>>> +        if (vms->highmem_compact && !*region_enabled) {
>>> +            continue;
>>>           }
>>>   
>>> -        *region_enabled &= fits;
>>>           base = region_base + region_size;
>>> +        if (fits) {
>>> +            vms->highest_gpa = region_base + region_size - 1;
>> 
>> vms->highest_gpa = base - 1;
>> 
>
> It's personal taste actually. I was thinking of using 'base - 1', but
> 'region_base + region_size - 1' looks more like a direct way. I don't
> have strong sense though and lets use 'base - 1' in next respin.

I don't really have a preference for one or the other.