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