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

[Eduardo Habkost]

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