On Wed, Oct 26 2022, Gavin Shan <gs...@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 <gs...@redhat.com>
>>> Reviewed-by: Cornelia Huck <coh...@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 <zheny...@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.
