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

[Marc Zyngier <maz@kernel.org>]

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).

So keying this on highmem makes it expose a device that may not be
there the first place since, as you pointed out that highmem_ecam can
be false in cases where highmem is true.

> So to me we should keep vms->highmem here

I really must be missing how this is supposed to work.

	M.

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