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

[Eric Auger]

Hi,
On 8/3/22 09:01, Marc Zyngier wrote:
> On Wed, 03 Aug 2022 04:01:04 +0100,
> Gavin Shan <gshan@redhat.com> 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.
> I don't get this. The current behaviour is to disable highmem_ecam if
> it doesn't fit in the PA space. I can't see anything that enables it
> if it was disabled the first place.
>
>> - 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.
> Why is that a problem? It matches the expected behaviour, as the
> highmem IO region is floating and is pushed up by the memory region.

the only concern I have is we changed the user experience with
d9afe24c29 ("hw/arm/virt: Disable highmem devices that don't fit in the
PA range")

before we returned an error if the highmem devices could not fit within
the IPA range and exited:

-m and ,maxmem option values require an IPA range (41 bits) larger than the one 
supported by the host (40 bits)


Now we skip them silently.
Most probably we should have gated that change with an option for
compatibility.

>
>> 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().
> Huh. By this principle, the user can change anything. Why is it
> important?
>
>>>> Improve address assignment for highmem IO regions to avoid the waste
>>>> in the PA space by putting the logic into virt_memmap_fits().
> I guess that this is what I understand the least. What do you mean by
> "wasted PA space"? Either the regions fit in the PA space, and
> computing their addresses in relevant, or they fall outside of it and
> what we stick in memap[index].base is completely irrelevant.

agreed. To me the only 'waste' we can have is when highmem ECAM is
disabled through a combination of user options and we provision for the
unused space when computing the highest_gpa. But it is 256 MB and the
case where it is disabled is marginal.

Eric
>
>>>> 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)"
>>
>>>> +        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;
>>         }
>>
>> 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.
> Why should the 'enabled' flag be updated by this function, instead of
> returning the value and keeping it as an assignment in the caller
> function? It is purely stylistic though.
>
> Thanks,
>
>       M.
>