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

[Andrew Jones]

On Wed, Mar 15, 2017 at 04:22:37PM +0100, Christoffer Dall wrote:
> On Wed, Mar 15, 2017 at 03:06:33PM +0100, Andrew Jones wrote:
> > On Wed, Mar 15, 2017 at 02:36:45PM +0100, Christoffer Dall wrote:
> > > > 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?
> > 
> > I don't know much (anything) about it, but, afaict, yes. See
> > https://lkml.org/lkml/2017/1/19/380
> > 
> > > > > 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.
> > 
> > I'm happy to help with this, even to take a stab at the first draft. Just
> > let me know if you or Shannon prefer to do the draft yourselves. If you
> > don't have a preference, then I can start working on it pretty soon.
> > 
> I'd be really happy for you to draft a new design doc, and I can review
> in private or publicly first as you prefer.
>

OK, I'll hopefully have something pulled together by the end of next week.

Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Christoffer Dall]

On Wed, Mar 15, 2017 at 03:06:33PM +0100, Andrew Jones wrote:
> On Wed, Mar 15, 2017 at 02:36:45PM +0100, Christoffer Dall wrote:
> > > 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?
> 
> I don't know much (anything) about it, but, afaict, yes. See
> https://lkml.org/lkml/2017/1/19/380
> 
> > > > 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.
> 
> I'm happy to help with this, even to take a stab at the first draft. Just
> let me know if you or Shannon prefer to do the draft yourselves. If you
> don't have a preference, then I can start working on it pretty soon.
> 
I'd be really happy for you to draft a new design doc, and I can review
in private or publicly first as you prefer.

Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Mark Rutland]

On Wed, Mar 15, 2017 at 03:06:33PM +0100, Andrew Jones wrote:
> On Wed, Mar 15, 2017 at 02:36:45PM +0100, Christoffer Dall wrote:
> > > 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?
> 
> I don't know much (anything) about it, but, afaict, yes. See
> https://lkml.org/lkml/2017/1/19/380

The "capacity" of a CPU does *not* tell you if your system is
hetereogeneous. Two vastly different CPU implementations can stumble
upon the same capacity, and two identical implementations could be
assigned close but not identical capacities.

The "capacity" is purely a scheduler heuristic, and should not be relied
upon for functional correctness.

We have a sysfs interface to see the MIDR and REVIDR of (online) CPUs,
which can tell you. See Documentation/arm64/cpu-feature-registers.txt.

Whether a system is heterogeneous can change at runtime, as CPUs can be
brought online very late (e.g. if booted with maxcpus capped, or if we
get "real" hotplug in future).

Thanks,
Mark.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Andrew Jones]

On Wed, Mar 15, 2017 at 02:21:48PM +, Peter Maydell wrote:
> On 15 March 2017 at 14:06, Andrew Jones  wrote:
> > On Wed, Mar 15, 2017 at 02:36:45PM +0100, Christoffer Dall wrote:
> >> > 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?
> >
> > I don't know much (anything) about it, but, afaict, yes. See
> > https://lkml.org/lkml/2017/1/19/380
> 
> AFAICT that won't work if the CPU you're trying to
> look at the ID registers for happens to be offline at
> the time you're looking at it.
>

Hmm, OK, but I think the jury is still out on whether QEMU even
needs to know this information. If we push the burden up to the
user/libvirt, than this information can be obtained [somehow] at
the higher level. The higher level can then choose to use generic,
if no vcpu affinity is desired, or, if one of the heterogeneous
cpu types is preferred for the vcpu's model, then it can also
ensure the vcpu is affined to the appropriate cpuset.

Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Peter Maydell]

On 15 March 2017 at 14:06, Andrew Jones  wrote:
> On Wed, Mar 15, 2017 at 02:36:45PM +0100, Christoffer Dall wrote:
>> > 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?
>
> I don't know much (anything) about it, but, afaict, yes. See
> https://lkml.org/lkml/2017/1/19/380

AFAICT that won't work if the CPU you're trying to
look at the ID registers for happens to be offline at
the time you're looking at it.

thanks
-- PMM
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Andrew Jones]

On Wed, Mar 15, 2017 at 02:36:45PM +0100, Christoffer Dall wrote:
> > 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?

I don't know much (anything) about it, but, afaict, yes. See
https://lkml.org/lkml/2017/1/19/380

> > > 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.

I'm happy to help with this, even to take a stab at the first draft. Just
let me know if you or Shannon prefer to do the draft yourselves. If you
don't have a preference, then I can start working on it pretty soon.

Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Christoffer Dall]

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 
> > > > 
> > > > 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 
> > > > ---
> > > >  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 
> > > >  #include 
> > > >  #include 
> > > > +#include 
> > > >  
> > > >  #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@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [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 
> > > 
> > > 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 
> > > ---
> > >  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 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  #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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Christoffer Dall]

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 
> > 
> > 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 
> > ---
> >  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 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #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?

> > 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.

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

Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Suzuki K Poulose]

On 28/01/17 14:55, Andrew Jones wrote:

On Mon, Jan 16, 2017 at 05:33:33PM +0800, Shannon Zhao wrote:

From: Shannon Zhao 

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 
---
 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 
 #include 
 #include 
+#include 

 #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?


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


+
if (!cross_vcpu && init->target != phys_target)
return -EINVAL;

@@ -1397,6 +1408,11 @@ static void check_kvm_target_cpu(void *ret)
*(int *)ret = kvm_target_cpu();
 }

+static void get_physical_cpu_midr(void *midr)
+{
+   *(u32 *)midr = read_cpuid_id();
+}
+
 struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr)
 {
struct kvm_vcpu *vcpu;
@@ -1417,6 +1433,7 @@ int kvm_arch_init(void *opaque)
 {
int err;
int ret, cpu;
+   u32 current_midr, midr;

if (!is_hyp_mode_available()) {
kvm_err("HYP mode not available\n");
@@ -1431,6 +1448,15 @@ int kvm_arch_init(void *opaque)
}
}

+   current_midr = read_cpuid_id();
+   for_each_online_cpu(cpu) {
+   smp_call_function_single(cpu, get_physical_cpu_midr, , 1);
+   if (current_midr != midr) {
+   heterogeneous_system = true;
+   break;
+   }
+   }


Is there no core kernel API that provides this?


On arm64, there is a per-cpu cpuinfo structure kept for each online CPU, which 
keeps
cached values of the ID registers. May be we could use that here.
See arch/arm64/kernel/cpuinfo.c::__cpuinfo_store_cpu()

Suzuki

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Andrew Jones]

On Mon, Jan 16, 2017 at 05:33:33PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> 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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  
>  #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?

>   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

> +
>   if (!cross_vcpu && init->target != phys_target)
>   return -EINVAL;
>  
> @@ -1397,6 +1408,11 @@ static void check_kvm_target_cpu(void *ret)
>   *(int *)ret = kvm_target_cpu();
>  }
>  
> +static void get_physical_cpu_midr(void *midr)
> +{
> + *(u32 *)midr = read_cpuid_id();
> +}
> +
>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr)
>  {
>   struct kvm_vcpu *vcpu;
> @@ -1417,6 +1433,7 @@ int kvm_arch_init(void *opaque)
>  {
>   int err;
>   int ret, cpu;
> + u32 current_midr, midr;
>  
>   if (!is_hyp_mode_available()) {
>   kvm_err("HYP mode not available\n");
> @@ -1431,6 +1448,15 @@ int kvm_arch_init(void *opaque)
>   }
>   }
>  
> + current_midr = read_cpuid_id();
> + for_each_online_cpu(cpu) {
> + smp_call_function_single(cpu, get_physical_cpu_midr, , 1);
> + if (current_midr != midr) {
> + heterogeneous_system = true;
> + break;
> + }
> + }

Is there no core kernel API that provides this?

> +
>   err = init_common_resources();
>   if (err)
>   return err;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 46115a2..cc2b63d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -872,6 +872,7 @@ struct kvm_ppc_smmu_info {
>  #define KVM_CAP_MSI_DEVID 131
>  #define KVM_CAP_PPC_HTM 132
>  #define KVM_CAP_ARM_CROSS_VCPU 133
> +#define KVM_CAP_ARM_HETEROGENEOUS 134
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> -- 
> 2.0.4
> 
>

drew 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm