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

[Marc Zyngier]

On Wed, 03 Aug 2022 14:02:04 +0100,
Gavin Shan <gshan@redhat.com> wrote:
> 
> Hi Marc,
> 
> On 8/3/22 5:01 PM, Marc Zyngier wrote:
> > On Wed, 03 Aug 2022 04:01:04 +0100,
> > Gavin Shan <gshan@redhat.com> wrote:
> >> 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.
> > 
> 
> There are several places or conditions where vms->highmem_ecam can be
> disabled:
> 
> - virt_instance_init() where vms->highmem_ecam is inherited from
>   !vmc->no_highmem_ecam. The option is set to true after virt-2.12
>   in virt_machine_2_12_options().
> 
> - machvirt_init() where vms->highmem_ecam can be disable if we have
>   32-bits vCPUs and failure on loading firmware.

Right. But at no point do we *enable* something that was disabled
beforehand, which is how I understood your previous comment.

> 
> - Another place is where we're talking about. It's address assignment
>   to fit the PA space.

Alignment? No, the alignment is cast into stone: it is set to the
smallest power-of-two containing the region (natural alignment).

> 
> >> 
> >> - 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.
> > 
> 
> Eric thought that VIRT_HIGH_GIC_REDIST2 and VIRT_HIGH_PCIE_MMIO regions
> aren't user selectable. I tended to explain why it's not true. 'maxmem='
> can affect the outcome. When 'maxmem=' value is big enough, there will be
> no free area in the PA space to hold those two regions.

Right, that's an interesting point. This is a consequence of these
upper regions floating above RAM.

> 
> >> 
> >> 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?
> > 
> 
> Still like above. I was explaining the possible cases where those
> 3 switches can be turned on/off by users or developers. Our code
> needs to be consistent and comprehensive.
> 
>   vms->highmem_redists
>   vms->highmem_ecam
>   vms->mmio
> 
> >> 
> >>>> 
> >>>> 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.
> > 
> 
> It's possible that we run into the following combination. we should
> have enough PA space to enable VIRT_HIGH_PCIE_MMIO region. However,
> the region is disabled in the original implementation because
> VIRT_HIGH_{GIC_REDIST2, PCIE_ECAM} regions consumed 1GB, which is
> unecessary and waste in the PA space.
> 
>     static MemMapEntry extended_memmap[] = {
>         [VIRT_HIGH_GIC_REDIST2] =   { 0x0, 64 * MiB },
>         [VIRT_HIGH_PCIE_ECAM] =     { 0x0, 256 * MiB },
>         [VIRT_HIGH_PCIE_MMIO] =     { 0x0, 512 * GiB },
>     };
> 
>     IPA_LIMIT           = (1UL << 40)
>     '-maxmem'           = 511GB              /* Memory starts from 1GB */
>     '-slots'            = 0
>     vms->highmem_rdist2 = false
>     vms->highmem_ecam   = false
>     vms->highmem_mmio   = true
>

Sure. But that's not how QEMU works today, and these regions are
enabled in order (though it could well be that my recent changes have
made the situation more complicated).

What you're suggesting is a pretty radical change in the way the
memory map is set. My hunch is that allowing the highmem IO regions to
be selectively enabled and allowed to float in the high IO space
should come as a new virt machine revision, with user-visible options.

Thanks,

        M.

-- 
Without deviation from the norm, progress is not possible.