Re: [PATCH v2 1/5] hw/arm/virt: Key enablement of highmem PCIe on highmem_ecam

[Marc Zyngier <maz@kernel.org>]

Hi Eric,

On Tue, 04 Jan 2022 15:31:33 +0000,
Eric Auger <eric.auger@redhat.com> wrote:
> 
> Hi Marc,
> 
> On 12/27/21 4:53 PM, Marc Zyngier wrote:
> > Hi Eric,
> >
> > Picking this up again after a stupidly long time...
> >
> > On Mon, 04 Oct 2021 13:00:21 +0100,
> > Eric Auger <eric.auger@redhat.com> wrote:
> >> Hi Marc,
> >>
> >> On 10/3/21 6:46 PM, Marc Zyngier wrote:
> >>> Currently, the highmem PCIe region is oddly keyed on the highmem
> >>> attribute instead of highmem_ecam. Move the enablement of this PCIe
> >>> region over to highmem_ecam.
> >>>
> >>> Signed-off-by: Marc Zyngier <maz@kernel.org>
> >>> ---
> >>>  hw/arm/virt-acpi-build.c | 10 ++++------
> >>>  hw/arm/virt.c            |  4 ++--
> >>>  2 files changed, 6 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >>> index 037cc1fd82..d7bef0e627 100644
> >>> --- a/hw/arm/virt-acpi-build.c
> >>> +++ b/hw/arm/virt-acpi-build.c
> >>> @@ -157,10 +157,9 @@ static void acpi_dsdt_add_virtio(Aml *scope,
> >>>  }
> >>>  
> >>>  static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
> >>> -                              uint32_t irq, bool use_highmem, bool highmem_ecam,
> >>> -                              VirtMachineState *vms)
> >>> +                              uint32_t irq, VirtMachineState *vms)
> >>>  {
> >>> -    int ecam_id = VIRT_ECAM_ID(highmem_ecam);
> >>> +    int ecam_id = VIRT_ECAM_ID(vms->highmem_ecam);
> >>>      struct GPEXConfig cfg = {
> >>>          .mmio32 = memmap[VIRT_PCIE_MMIO],
> >>>          .pio    = memmap[VIRT_PCIE_PIO],
> >>> @@ -169,7 +168,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
> >>>          .bus    = vms->bus,
> >>>      };
> >>>  
> >>> -    if (use_highmem) {
> >>> +    if (vms->highmem_ecam) {
> >> highmem_ecam is more restrictive than use_highmem:
> >> vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64);
> >>
> >> If I remember correctly there was a problem using highmem ECAM with 32b
> >> AAVMF FW.
> >>
> >> However 5125f9cd2532 ("hw/arm/virt: Add high MMIO PCI region, 512G in
> >> size") introduced high MMIO PCI region without this constraint.
> > Then I really don't understand the point of this highmem_ecam. We only
> > register the highmem version if highmem_ecam is set (see the use of
> > VIRT_ECAM_ID() to pick the right ECAM window).
> 
> but aren't we talking about different regions? On one hand the [high]
> MMIO region (512GB wide) and the [high] ECAM region (256MB large).
> To me you can enable either independently. High MMIO region is used by
> some devices likes ivshmem/video cards while high ECAM was introduced to
> extend the number of supported buses: 601d626d148a (hw/arm/virt: Add a
> new 256MB ECAM region).
> 
> with the above change the high MMIO region won't be set with 32b
> FW+kernel and LPAE whereas it is currently.
> 
> high ECAM was not supported by 32b FW, hence the highmem_ecam.
> 
> but maybe I miss your point?

There are two issues.

First, I have been conflating the ECAM and MMIO ranges, and you only
made me realise that they were supposed to be independent.  I still
think the keying on highmem is wrong, but the main issue is that the
highmem* flags don't quite describe the shape of the platform.

All these booleans indicate is whether the feature they describe (the
high MMIO range, the high ECAM range, and in one of my patches the
high RD range) are *allowed* to live above 4GB, but do not express
whether then are actually usable (i.e. fit in the PA range).

Maybe we need to be more thorough in the way we describe the extended
region in the VirtMachineState structure:

- highmem: overall control for anything that *can* live above 4GB
- highmem_ecam: Has a PCIe ECAM region above 256GB
- highmem_mmio: Has a PCIe MMIO region above 256GB
- highmem_redist: Has 512 RDs above 256GB

Crucially, the last 3 items must fit in the PA range or be disabled.

We have highmem_ecam which is keyed on highmem, but not on the PA
range.  highmem_mmio doesn't exist at all (we use highmem instead),
and I'm only introducing highmem_redist.

For these 3 ranges, we should have something like

vms->highmem_xxx &= (vms->highmem &&
		     (vms->memmap[XXX].base + vms->vms->memmap[XXX].size) < vms->highest_gpa);

and treat them as independent entities.  Unless someone shouts, I'm
going to go ahead and implement this logic.

Thanks,

	M.

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