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

[Marc Zyngier]

Hi Gavin,

On Thu, 11 Aug 2022 06:32:36 +0100,
Gavin Shan <gshan@redhat.com> wrote:
> 
> Hi Marc,
> 
> On 8/8/22 7:17 PM, Marc Zyngier wrote:
> > On Wed, 03 Aug 2022 14:02:04 +0100,
> > Gavin Shan <gshan@redhat.com> wrote:

[...]

> Sorry for the delay. I think the original changelog is confusing
> enough and sorry about it. I will improve it if v2 is needed :)
> 
> Yes, we shouldn't assign address to VIRT_HIGH_PCIE_ECAM region
> and enable it when vms->highmem_ecam is false in virt_set_memmap().
> 
> In the original implementation of virt_set_memmap(), the memory
> regioin is disabled when when vms->highmem_ecam is false. However,
> the address is still assigned to the memory region, even when
> vms->highmem_ecam is false. This leads to waste in the PA space.

Yes, I got this point now. However, I think the approach you've chosen
leads to subtle issues, see below.

[...]

> >> 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.
> > 
> 
> Yep, it's fine for those high memory region floating above RAM, and to
> disable them if we run out of PA space. Something may be irrelevant
> to this topic: VIRT_HIGH_PCIE_MMIO region has 512GB, which is huge one.
> It may be nice to fall back smaller sizes when we're having tight PA
> space. For example, we can fall back to try 256GB if 512GB doesn't fit
> into the PA space.
> 
> However, I'm not sure how we had the size (512GB) for the region. Are
> there practical factors why we need a 512GB PCIe 64-bits MMIO space?

I have no idea. But this is something that is now relied upon by
existing VMs, and I'm not sure we can break this.

> >>>>>> 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.
> > 
> 
> Yeah, These regions are enabled in order. It also means they have ascending
> priorities. In other words, '-maxmem' has higher priority than those 3 high
> memory regions.
> 
> My suggested code changes just improve the address assignment for these 3
> high memory regions, without changing the mechanism fundamentally. The
> intention of the proposed changes are like below.
> 
> - In virt_set_memmap(), don't assign address for one specific high memory
>   region if it has been disabled.
> 
> - Put the logic into standalone helper, to simplify the code.
> 
> I'm not sure about the user-visible options. I would say it's going to
> increase user's load. I means the user experience will be degraded.
> Those user-visible options needs to be worried by users :)

Not necessarily. You could have a default that picks whatever fits.
But this is hard to get right, and I'm not convinced you will always
be able to satisfy everyone.

I'm also worried of seeing different behaviours if I dump a VM from
QEMU 7.x and reload it with 7.y, only to realise that I don't have the
same memory layout. In my opinion, these things need to be either
constant, or user-specified.

> Marc, lets improve the changelog and the code changes in v2, to seek
> your comments if you agree? :)

Of course!

        M.

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