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 <gs...@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
