On Wed, Mar 15, 2017 at 01:51:13PM +0100, Andrew Jones wrote:
> 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 <shannon.zhao@xxxxxxxxxx>
> > > >
> > > > 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 <shannon.zhao@xxxxxxxxxx>
> > > > ---
> > > > 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?
Probably not.
> 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.
>
Can it? Is this information available in a reliable way from userspace?
> >
> > > > 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.
>
Yes.
> >
> > 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.
>
Yes, it's on my list.
Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
