Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework

From: Eduardo Habkost <ehabkost@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: Richard Henderson <richard.henderson@linaro.org>,
	QEMU Developers <qemu-devel@nongnu.org>,
	Cameron Esfahani <dirty@apple.com>,
	Roman Bolshakov <r.bolshakov@yadro.com>,
	Alexander Graf <agraf@csgraf.de>,
	Claudio Fontana <cfontana@suse.de>,
	qemu-arm <qemu-arm@nongnu.org>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH 8/8] hw/arm/virt: Disable highmem when on hypervisor.framework
Date: Fri, 27 Nov 2020 11:26:33 -0500	[thread overview]
Message-ID: <20201127162633.GY2271382@habkost.net> (raw)
In-Reply-To: <CAFEAcA_Nc0Jp-3PPigt1YdqHfNhGToovCOO16DOPPC9Bt663qg@mail.gmail.com>

On Thu, Nov 26, 2020 at 10:29:01PM +0000, Peter Maydell wrote:
> On Thu, 26 Nov 2020 at 22:14, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > On Thu, Nov 26, 2020 at 10:50:17PM +0100, Alexander Graf wrote:
> > > The Apple M1 only supports up to 36 bits of physical address space. That
> > > means we can not fit the 64bit MMIO BAR region into our address space.
> > >
> > > To fix this, let's not expose a 64bit MMIO BAR region when running on
> > > Apple Silicon.
> > >
> > > I have not been able to find a way to enumerate that easily, so let's
> > > just assume we always have that little PA space on hypervisor.framework
> > > systems.
> > >
> > > Signed-off-by: Alexander Graf <agraf@csgraf.de>
> > > ---
> > >  hw/arm/virt.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index 27dbeb549e..d74053ecd4 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -45,6 +45,7 @@
> > >  #include "hw/display/ramfb.h"
> > >  #include "net/net.h"
> > >  #include "sysemu/device_tree.h"
> > > +#include "sysemu/hvf.h"
> > >  #include "sysemu/numa.h"
> > >  #include "sysemu/runstate.h"
> > >  #include "sysemu/sysemu.h"
> > > @@ -1746,6 +1747,14 @@ static void machvirt_init(MachineState *machine)
> > >      unsigned int smp_cpus = machine->smp.cpus;
> > >      unsigned int max_cpus = machine->smp.max_cpus;
> > >
> > > +    /*
> > > +     * On Hypervisor.framework capable systems, we only have 36 bits of PA
> > > +     * space, which is not enough to fit a 64bit BAR space
> > > +     */
> > > +    if (hvf_enabled()) {
> > > +        vms->highmem = false;
> > > +    }
> >
> > Direct checks for *_enabled() are a pain to clean up later when
> > we add support to new accelerators.  Can't this be implemented as
> > (e.g.) a AccelClass::max_physical_address_bits field?
> 
> It's a property of the CPU (eg our emulated TCG CPUs may have
> varying supported numbers of physical address bits). So the
> virt board ought to look at the CPU, and the CPU should be
> set up with the right information for all of KVM, TCG, HVF
> (either a specific max_phys_addr_bits value or just ensure
> its ID_AA64MMFR0_EL1.PARange is right, not sure which would
> be easier/nicer).

Agreed.

My suggestion would still apply to the CPU code that will pick
the address size; ideally, accel-specific behaviour should be
represented as meaningful fields in AccelClass (either data or
virtual methods) instead of direct *_enabled() checks.

-- 
Eduardo