Re: [Qemu-arm] [PATCH RFC 6/7] ARM64: KVM: Support heterogeneous system

[Andrew Jones]

On Wed, Mar 15, 2017 at 12:50:44PM +0100, Christoffer Dall wrote:
> Hi Drew,
> 
> [Replying here to try to capture the discussion about this patch we had
> at connect].
> 
> On Sat, Jan 28, 2017 at 03:55:51PM +0100, Andrew Jones wrote:
> > On Mon, Jan 16, 2017 at 05:33:33PM +0800, Shannon Zhao wrote:
> > > From: Shannon Zhao <address@hidden>
> > > 
> > > When initializing KVM, check whether physical hardware is a
> > > heterogeneous system through the MIDR values. If so, force userspace to
> > > set the KVM_ARM_VCPU_CROSS feature bit. Otherwise, it should fail to
> > > initialize VCPUs.
> > > 
> > > Signed-off-by: Shannon Zhao <address@hidden>
> > > ---
> > >  arch/arm/kvm/arm.c       | 26 ++++++++++++++++++++++++++
> > >  include/uapi/linux/kvm.h |  1 +
> > >  2 files changed, 27 insertions(+)
> > > 
> > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> > > index bdceb19..21ec070 100644
> > > --- a/arch/arm/kvm/arm.c
> > > +++ b/arch/arm/kvm/arm.c
> > > @@ -46,6 +46,7 @@
> > >  #include <asm/kvm_coproc.h>
> > >  #include <asm/kvm_psci.h>
> > >  #include <asm/sections.h>
> > > +#include <asm/cputype.h>
> > >  
> > >  #ifdef REQUIRES_VIRT
> > >  __asm__(".arch_extension virt");
> > > @@ -65,6 +66,7 @@ static unsigned int kvm_vmid_bits __read_mostly;
> > >  static DEFINE_SPINLOCK(kvm_vmid_lock);
> > >  
> > >  static bool vgic_present;
> > > +static bool heterogeneous_system;
> > >  
> > >  static DEFINE_PER_CPU(unsigned char, kvm_arm_hardware_enabled);
> > >  
> > > @@ -210,6 +212,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, 
> > > long ext)
> > >   case KVM_CAP_ARM_CROSS_VCPU:
> > >           r = 1;
> > >           break;
> > > + case KVM_CAP_ARM_HETEROGENEOUS:
> > > +         r = heterogeneous_system;
> > > +         break;
> > 
> > What's this for? When/why would usespace check it?
> > 
> 
> Without a capability, how can userspace tell the difference between "I
> got -EINVAL because I'm on an old kernel" or "I asked for something that
> any kernel cannot emulate"?
> 
> Do we need to distinguish between these cases?

Yup, I'm in full agreement that we need a capability for the
cross-vcpu support. Above this heterogeneous one there's the
CROSS_VCPU one though. Do we need both? If QEMU wants to know
whether or not the host it's running on is heterogeneous, then
it can just query sysfs, rather than ask KVM.

> 
> > >   case KVM_CAP_COALESCED_MMIO:
> > >           r = KVM_COALESCED_MMIO_PAGE_OFFSET;
> > >           break;
> > > @@ -812,6 +817,12 @@ static int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
> > >   int phys_target = kvm_target_cpu();
> > >   bool cross_vcpu = kvm_vcpu_has_feature_cross_cpu(init);
> > >  
> > > + if (heterogeneous_system && !cross_vcpu) {
> > > +         kvm_err("%s:Host is a heterogeneous system, set 
> > > KVM_ARM_VCPU_CROSS bit\n",
> > > +                 __func__);
> > > +         return -EINVAL;
> > > + }
> > 
> > Instead of forcing userspace to set a bit, why not just confirm the
> > target selected will work? E.g. if only generic works on a heterogeneous
> > system then just 
> > 
> >  if (heterogeneous_system && init->target != GENERIC)
> >     return -EINVAL
> > 
> > should work
> > 
> 
> Yes, I think we concluded that if we advertise if we can do the
> cross type emulation or not, then if we can do the emulation we should
> just do it when possible, for maximum user experience.

Your agreement here implies to me that we only need the one capability.

> 
> I'm sure I missed some aspect of this discussion though.

Me too. As we discussed, it's probably time to try and hash out a fresh
design doc. It'd be good to get a clear design agreed upon before
returning to the patches.

Thanks,
drew