Re: [PATCH 1/2] hw/arm/virt: Improve address assignment for highmem IO regions

[Eric Auger]

Hi Gavin,

On 8/3/22 05:01, Gavin Shan wrote:
> Hi Eric,
>
> On 8/2/22 7:41 PM, Eric Auger wrote:
>> On 8/2/22 08:45, Gavin Shan wrote:
>>> There are 3 highmem IO regions as below. They can be disabled in
>>> two situations: (a) The specific region is disabled by user. (b)
>>> The specific region doesn't fit in the PA space. However, the base
>>> address and highest_gpa are still updated no matter if the region
>>> is enabled or disabled. It's incorrectly incurring waste in the PA
>>> space.
>> If I am not wrong highmem_redists and highmem_mmio are not user
>> selectable
>>
>> Only highmem ecam depends on machine type & ACPI setup. But I would say
>> that in server use case it is always set. So is that optimization really
>> needed?
>
> There are two other cases you missed.
>
> - highmem_ecam is enabled after virt-2.12, meaning it stays disabled
>   before that.
Yes that's what I meant above by 'depends on machine type'
>
> - The high memory region can be disabled if user is asking large
>   (normal) memory space through 'maxmem=' option. When the requested
>   memory by 'maxmem=' is large enough, the high memory regions are
>   disabled. It means the normal memory has higher priority than those
>   high memory regions. This is the case I provided in (b) of the
>   commit log.
yes but in such a case you don't "waste" IPA as you mention in the
commit log because you only ask for a VM dimensionned for the highest_gpa.
The only case where you would "waste" IPA is for high ecam which can
disabled by option combination but it is marginal.

>
> In the commit log, I was supposed to say something like below for
> (a):
>
> - The specific high memory region can be disabled through changing
>   the code by user or developer. For example, 'vms->highmem_mmio'
>   is changed from true to false in virt_instance_init().
>
>>>
>>> Improve address assignment for highmem IO regions to avoid the waste
>>> in the PA space by putting the logic into virt_memmap_fits().
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>>   hw/arm/virt.c | 54
>>> +++++++++++++++++++++++++++++----------------------
>>>   1 file changed, 31 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 9633f822f3..bc0cd218f9 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -1688,6 +1688,34 @@ static uint64_t
>>> virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
>>>       return arm_cpu_mp_affinity(idx, clustersz);
>>>   }
>>>   +static void virt_memmap_fits(VirtMachineState *vms, int index,
>>> +                             bool *enabled, hwaddr *base, int pa_bits)
>>> +{
>>> +    hwaddr size = extended_memmap[index].size;
>>> +
>>> +    /* The region will be disabled if its size isn't given */
>>> +    if (!*enabled || !size) {
>> In which case do you have !size?
>
> Yeah, we don't have !size and the condition should be removed.
>
>>> +        *enabled = false;
>>> +        vms->memmap[index].base = 0;
>>> +        vms->memmap[index].size = 0;
>> It looks dangerous to me to reset the region's base and size like that.
>> for instance fdt_add_gic_node() will add dummy data in the dt.
>
> I would guess you missed that the high memory regions won't be exported
> through device-tree if they have been disabled. We have a check there,
> which is "if (nb_redist_regions == 1)"
OK I missed a check was added in virt_gicv3_redist_region_count.
Nevertheless, your comment "The region will be disabled if its size
isn't given */ is not obvious to me. To me the region is disabled if the
corresponding flag is not set. From your comment I have the impression
the size is checked to see if the region is exposed, it does not look
obvious.
>
>>> +        return;
>>> +    }
>>> +
>>> +    /*
>>> +     * Check if the memory region fits in the PA space. The memory map
>>> +     * and highest_gpa are updated if it fits. Otherwise, it's
>>> disabled.
>>> +     */
>>> +    *enabled = (ROUND_UP(*base, size) + size <= BIT_ULL(pa_bits));
>> using a 'fits' local variable would make the code more obvious I think
>
> Lets confirm if you're suggesting something like below?
>
>         bool fits;
>
>         fits = (ROUND_UP(*base, size) + size <= BIT_ULL(pa_bits));
>
>         if (fits) {
>            /* update *base, memory mapping, highest_gpa */
>         } else {
>            *enabled = false;
>         }
yes that's what I suggested.
>
> I guess we can simply do
>
>         if (ROUND_UP(*base, size) + size <= BIT_ULL(pa_bits)) {
>            /* update *base, memory mapping, highest_gpa */
>         } else {
>            *enabled = false;
>         }
>
> Please let me know which one looks best to you.
>
>>> +    if (*enabled) {
>>> +        *base = ROUND_UP(*base, size);
>>> +        vms->memmap[index].base = *base;
>>> +        vms->memmap[index].size = size;
>>> +        vms->highest_gpa = *base + size - 1;
>>> +
>>> +    *base = *base + size;
>>> +    }
>>> +}
>>> +
>>>   static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
>>>   {
>>>       MachineState *ms = MACHINE(vms);
>>> @@ -1744,37 +1772,17 @@ static void virt_set_memmap(VirtMachineState
>>> *vms, int pa_bits)
>>>       vms->highest_gpa = memtop - 1;
>>>         for (i = VIRT_LOWMEMMAP_LAST; i <
>>> ARRAY_SIZE(extended_memmap); i++) {
>>> -        hwaddr size = extended_memmap[i].size;
>>> -        bool fits;
>>> -
>>> -        base = ROUND_UP(base, size);
>>> -        vms->memmap[i].base = base;
>>> -        vms->memmap[i].size = size;
>>> -
>>> -        /*
>>> -         * Check each device to see if they fit in the PA space,
>>> -         * moving highest_gpa as we go.
>>> -         *
>>> -         * For each device that doesn't fit, disable it.
>>> -         */
>>> -        fits = (base + size) <= BIT_ULL(pa_bits);
>>> -        if (fits) {
>>> -            vms->highest_gpa = base + size - 1;
>>> -        }
>>> -
>>
>> we could avoid running the code below in case highmem is not set. We
>> would need to reset that flags though.
>>
>
> Yeah, I think it's not a big deal. My though is to update the flag in
> virt_memmap_fits().

Eric
>
>>>           switch (i) {
>>>           case VIRT_HIGH_GIC_REDIST2:
>>> -            vms->highmem_redists &= fits;
>>> +            virt_memmap_fits(vms, i, &vms->highmem_redists, &base,
>>> pa_bits);
>>>               break;
>>>           case VIRT_HIGH_PCIE_ECAM:
>>> -            vms->highmem_ecam &= fits;
>>> +            virt_memmap_fits(vms, i, &vms->highmem_ecam, &base,
>>> pa_bits);
>>>               break;
>>>           case VIRT_HIGH_PCIE_MMIO:
>>> -            vms->highmem_mmio &= fits;
>>> +            virt_memmap_fits(vms, i, &vms->highmem_mmio, &base,
>>> pa_bits);
>>>               break;
>>>           }
>>> -
>>> -        base += size;
>>>       }
>>>         if (device_memory_size > 0) {
>
> Thanks,
> Gavin
>