Hi Eric,
Picking this up again after a stupidly long time...
On Mon, 04 Oct 2021 13:00:21 +0100,
Eric Auger <eric.auger@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
