kvm_pv_unhalt and kernel_irqchip=off

kvm_pv_unhalt and kernel_irqchip=off

[Eduardo Habkost]

Hi,

I got a bug report[1] that seems to be caused by kvm_pv_unhalt
not working with kernel_irqchip=off.

Is kvm_pv_unhalt supposed to work at all without kernel_irqchip?
Should QEMU prevent it from being enabled if kernel_irqchip=off?

Related question: is there any kvm-unit-test test for
kvm_pv_unhalt?

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1363679

-- 
Eduardo

Re: kvm_pv_unhalt and kernel_irqchip=off

[Radim Krčmář]

2016-08-10 15:27-0300, Eduardo Habkost:
> Hi,
> 
> I got a bug report[1] that seems to be caused by kvm_pv_unhalt
> not working with kernel_irqchip=off.
> 
> Is kvm_pv_unhalt supposed to work at all without kernel_irqchip?

No.  Guest interface of current PV unhalt is based on APIC ID and KVM
does not ask userspace to handle the unknown mapping to VCPU.

The code in kernel just somehow managed not to BUG. :)

> Should QEMU prevent it from being enabled if kernel_irqchip=off?

Yes.  We'd need a new guest or userspace interface to make it work.

KVM should not pretend that the feature works without kernel_irqchip.

> Related question: is there any kvm-unit-test test for
> kvm_pv_unhalt?

Not that I know of, thanks for the request.

> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1363679

Re: kvm_pv_unhalt and kernel_irqchip=off

[Radim Krčmář]

2016-08-10 21:04+0200, Radim Krčmář:
>> Related question: is there any kvm-unit-test test for
>> kvm_pv_unhalt?
> 
> Not that I know of, thanks for the request.

I attached a simple test below.  It will likely need some modifications
after the bug is fixed in QEMU and/or KVM.

For a fix in KVM, I used the solution from another feature that doesn't
work with irqchip=off, x2APIC.
Jokes aside, we can fix them both because SET_CPUID cannot be called
after CREATE_IRQCHIP.  I'll do so tomorrow.

---8<---
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index ee7f180237b1..f33c680e5dbf 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -402,6 +402,11 @@ static inline void safe_halt(void)
 	asm volatile("sti; hlt");
 }
 
+static inline void cpu_relax(void)
+{
+	asm volatile ("rep; nop");
+}
+
 static inline u32 read_pkru(void)
 {
     unsigned int eax, edx;
diff --git a/x86/Makefile.common b/x86/Makefile.common
index 356d879a986b..6a3bbc86df68 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -45,6 +45,7 @@ tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
                $(TEST_DIR)/tsc_adjust.flat $(TEST_DIR)/asyncpf.flat \
                $(TEST_DIR)/init.flat $(TEST_DIR)/smap.flat \
                $(TEST_DIR)/hyperv_synic.flat $(TEST_DIR)/hyperv_stimer.flat \
+               $(TEST_DIR)/pv_unhalt.flat \
 
 ifdef API
 tests-common += api/api-sample
diff --git a/x86/pv_unhalt.c b/x86/pv_unhalt.c
new file mode 100644
index 000000000000..d717e309eab0
--- /dev/null
+++ b/x86/pv_unhalt.c
@@ -0,0 +1,73 @@
+#include "libcflat.h"
+#include "vm.h"
+#include "desc.h"
+#include "smp.h"
+
+/* Intel and AMD hypercalls should be interchangeable. */
+#define KVM_HYPERCALL ".byte 0x0f,0x01,0xc1"
+
+#define KVM_HC_KICK_CPU			5
+
+#define KVM_CPUID_FEATURES	0x40000001
+#define KVM_FEATURE_PV_UNHALT	(1 << 7)
+
+static volatile bool pv_unhalt_works;
+
+static inline long kvm_hypercall2(unsigned int nr, unsigned long p1,
+				  unsigned long p2)
+{
+	long ret;
+	asm volatile(KVM_HYPERCALL
+		     : "=a"(ret)
+		     : "a"(nr), "b"(p1), "c"(p2)
+		     : "memory");
+	return ret;
+}
+
+static void halt(void * nothing)
+{
+	asm volatile("cli; hlt; sti" ::: "memory");
+
+	pv_unhalt_works = true;
+}
+
+static void test_pv_unhalt(void)
+{
+	unsigned long delay;
+
+	if (cpu_count() < 2) {
+		report_skip("pv unhalt");
+		return;
+	}
+
+	/* XXX: KVM_CPUID_FEATURES can have a different base offset */
+	if (!(cpuid(KVM_CPUID_FEATURES).a & KVM_FEATURE_PV_UNHALT)) {
+		/* TODO: use argv to decide whether the presence of pv_unhalt
+		 * is a skip or pass? */
+		report("pv unhalt", 1);
+		return;
+	}
+
+	on_cpu_async(1, halt, 0);
+	/* we need a small delay because CPU1 can't notify that it halted */
+	for (delay = 1<<28; delay; delay--)
+		cpu_relax();
+
+	kvm_hypercall2(KVM_HC_KICK_CPU, 0, 1);
+
+	/* TODO: sane delay loops (ignoring pv_unhalt_works is fine) */
+	for (delay = 1<<28; delay && !pv_unhalt_works; delay--)
+		cpu_relax();
+
+	report("pv unhalt", pv_unhalt_works);
+}
+
+int main(int ac, char **av)
+{
+	setup_vm();
+	smp_init();
+
+	test_pv_unhalt();
+
+	return report_summary();
+}
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 60747cfca94e..6fe688d01357 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -197,3 +197,13 @@ extra_params = -cpu kvm64,hv_synic -device hyperv-testdev
 file = hyperv_stimer.flat
 smp = 2
 extra_params = -cpu kvm64,hv_time,hv_synic,hv_stimer -device hyperv-testdev
+
+[pv_unhalt]
+file = pv_unhalt.flat
+smp = 2
+extra_params = -cpu kvm64,+kvm_pv_unhalt
+
+[pv_unhalt_irqchip_off]
+file = pv_unhalt.flat
+smp = 2
+extra_params = -cpu kvm64,+kvm_pv_unhalt -machine kernel_irqchip=off

GET_SUPPORTED_CPUID and irqchip (was Re: kvm_pv_unhalt and kernel_irqchip=off)

[Eduardo Habkost]

On Wed, Aug 10, 2016 at 09:04:13PM +0200, Radim Krčmář wrote:
> 2016-08-10 15:27-0300, Eduardo Habkost:
> > Hi,
> > 
> > I got a bug report[1] that seems to be caused by kvm_pv_unhalt
> > not working with kernel_irqchip=off.
> > 
> > Is kvm_pv_unhalt supposed to work at all without kernel_irqchip?
> 
> No.  Guest interface of current PV unhalt is based on APIC ID and KVM
> does not ask userspace to handle the unknown mapping to VCPU.
> 
> The code in kernel just somehow managed not to BUG. :)
> 
> > Should QEMU prevent it from being enabled if kernel_irqchip=off?
> 
> Yes.  We'd need a new guest or userspace interface to make it work.

Thanks for the answers!

Now I have another question: are features that require the
in-kernel irqchip supposed to be present in GET_SUPPORTED_CPUID?

We have examples of both cases in KVM:

* TSC_DEADLINE_TIMER is _not_ present in GET_SUPPORTED_CPUID,
  and is reported through KVM_CAP_TSC_DEADLINE_TIMER.
* X2APIC is present in GET_SUPPORTED_CPUID, but the bit
  makes sense only if the in-kernel irqchip is used.
* KVM_PV_UNHALT is present in GET_SUPPORTED_CPUID, but
  requires the in-kernel irqchip to work.

Should userspace expect more cases like x2apic and kvm_pv_unhalt
in the future?

-- 
Eduardo

Re: GET_SUPPORTED_CPUID and irqchip (was Re: kvm_pv_unhalt and kernel_irqchip=off)

[Radim Krčmář]

2016-08-12 15:37-0300, Eduardo Habkost:
> Now I have another question: are features that require the
> in-kernel irqchip supposed to be present in GET_SUPPORTED_CPUID?

I don't think so.  Simply removing X2APIC and PV_UNHALT would disable
them on old userspaces, though, which would probably cause more bugs.

(The blunder doesn't seem to be bad enough for a new capability or
 interface and a deprecation protocol on these features.)

> We have examples of both cases in KVM:
> 
> * TSC_DEADLINE_TIMER is _not_ present in GET_SUPPORTED_CPUID,
>   and is reported through KVM_CAP_TSC_DEADLINE_TIMER.
> * X2APIC is present in GET_SUPPORTED_CPUID, but the bit
>   makes sense only if the in-kernel irqchip is used.
> * KVM_PV_UNHALT is present in GET_SUPPORTED_CPUID, but
>   requires the in-kernel irqchip to work.

Well ... no excuses for that.

> Should userspace expect more cases like x2apic and kvm_pv_unhalt
> in the future?

At least one userspace (QEMU) doesn't filter unknown features from
GET_SUPPORTED_CPUID, so KVM cannot plan to pass conditionally buggy
features.
KVM would need to define a new interface to handle these issues, so I
think that userspace can ignore unknown KVM bugs.

I would like to return -EINVAL from KVM_SET_CPUID2 if userspace
requested a new CPUID feature that cannot work in given situation.

Another way would be to disable buggy features in KVM_SET_CPUID2, which
would require userspace to call KVM_GET_CPUID2 afterwards to learn what
the guest is actually using.

I have patches that implement the latter for X2APIC and PV_UNHALT, but
I'm not sure if it's better than leaving the bug unfixed, because QEMU
doesn't use KVM_GET_CPUID2 and migration to older KVM would change
CPUID, which is a very subtle bug.
What do you think?

Re: GET_SUPPORTED_CPUID and irqchip (was Re: kvm_pv_unhalt and kernel_irqchip=off)

[Eduardo Habkost]

On Sat, Aug 13, 2016 at 02:43:03PM +0200, Radim Krčmář wrote:
> 2016-08-12 15:37-0300, Eduardo Habkost:
> > Now I have another question: are features that require the
> > in-kernel irqchip supposed to be present in GET_SUPPORTED_CPUID?
> 
> I don't think so.  Simply removing X2APIC and PV_UNHALT would disable
> them on old userspaces, though, which would probably cause more bugs.

Yes, we can't remove it now.

> 
> (The blunder doesn't seem to be bad enough for a new capability or
>  interface and a deprecation protocol on these features.)

Agreed. After we notice it and work around it in userspace, it's
now part of the interface and not a problem anymore.

> 
> > We have examples of both cases in KVM:
> > 
> > * TSC_DEADLINE_TIMER is _not_ present in GET_SUPPORTED_CPUID,
> >   and is reported through KVM_CAP_TSC_DEADLINE_TIMER.
> > * X2APIC is present in GET_SUPPORTED_CPUID, but the bit
> >   makes sense only if the in-kernel irqchip is used.
> > * KVM_PV_UNHALT is present in GET_SUPPORTED_CPUID, but
> >   requires the in-kernel irqchip to work.
> 
> Well ... no excuses for that.
> 
> > Should userspace expect more cases like x2apic and kvm_pv_unhalt
> > in the future?
> 
> At least one userspace (QEMU) doesn't filter unknown features from
> GET_SUPPORTED_CPUID, so KVM cannot plan to pass conditionally buggy
> features.

I will add to my TODO-list a reminder to write a Documentation
update for GET_SUPPORTED_CPUID mentioning those details (unless
somebody else volunteers to do it).

> KVM would need to define a new interface to handle these issues, so I
> think that userspace can ignore unknown KVM bugs.

What can we do to avoid introducing those bugs in the future?

> 
> I would like to return -EINVAL from KVM_SET_CPUID2 if userspace
> requested a new CPUID feature that cannot work in given situation.
> 
> Another way would be to disable buggy features in KVM_SET_CPUID2, which
> would require userspace to call KVM_GET_CPUID2 afterwards to learn what
> the guest is actually using.

I'd prefer to get -EINVAL. QEMU needs to be 100% aware of the
resulting CPUID bits, so if we end up trying to enable something
that will never work, it's already too late to silently clear
CPUID bits.

This shouldn't make any difference for existing QEMU code,
because it already filters out all CPUID feature flags based
on GET_SUPPORTED_CPUID (+ the extra fixups in
QEMU's kvm_arch_get_supported_cpuid()).

But: note that returning -EINVAL might break very old QEMU
versions. Maybe we really want them to break (because they have
broken CPUID logic), but I am not sure.

> 
> I have patches that implement the latter for X2APIC and PV_UNHALT, but
> I'm not sure if it's better than leaving the bug unfixed, because QEMU
> doesn't use KVM_GET_CPUID2 and migration to older KVM would change
> CPUID, which is a very subtle bug.
> What do you think?

Implementing those checks basically mean moving the existing
kvm_arch_get_supported_cpuid() logic inside KVM. If we do that,
can we get an interface that will allow QEMU to just query KVM
instead of duplicating that logic?

Making KVM_SET_CPUID2 clear those bits and/or return -EINVAL
would probably be enough as an interface, as long as QEMU has a
way to know in advance if it needs to clear CPUID bits based on
the legacy kvm_arch_get_supported_cpuid() code (for older
kernels), or if it can just give everything to KVM_SET_CPUID2,
and check for unsupported/cleared flags later (for newer
kernels).

-- 
Eduardo

Re: GET_SUPPORTED_CPUID and irqchip (was Re: kvm_pv_unhalt and kernel_irqchip=off)

[Paolo Bonzini]

On 15/08/2016 15:03, Eduardo Habkost wrote:
> > KVM would need to define a new interface to handle these issues, so I
> > think that userspace can ignore unknown KVM bugs.
> 
> What can we do to avoid introducing those bugs in the future?

Nothing really except documenting the quirks in
Documentation/virtual/kvm/ (especially cpuid.txt in the case of
pv_unhalt).  This way the next time someone adds something there it will
hopefully catch the eyes of the reviewers.

It would probably be good to add a mention of KVM_ENABLE_CAP to
Documentation/virtual/kvm/review-checklist.txt.

Re: GET_SUPPORTED_CPUID and irqchip (was Re: kvm_pv_unhalt and kernel_irqchip=off)

[Radim Krčmář]

2016-08-15 10:03-0300, Eduardo Habkost:
> On Sat, Aug 13, 2016 at 02:43:03PM +0200, Radim Krčmář wrote:
>> I would like to return -EINVAL from KVM_SET_CPUID2 if userspace
>> requested a new CPUID feature that cannot work in given situation.
>> 
>> Another way would be to disable buggy features in KVM_SET_CPUID2, which
>> would require userspace to call KVM_GET_CPUID2 afterwards to learn what
>> the guest is actually using.
> 
> I'd prefer to get -EINVAL. QEMU needs to be 100% aware of the
> resulting CPUID bits, so if we end up trying to enable something
> that will never work, it's already too late to silently clear
> CPUID bits.
> 
> This shouldn't make any difference for existing QEMU code,
> because it already filters out all CPUID feature flags based
> on GET_SUPPORTED_CPUID (+ the extra fixups in
> QEMU's kvm_arch_get_supported_cpuid()).
> 
> But: note that returning -EINVAL might break very old QEMU
> versions. Maybe we really want them to break (because they have
> broken CPUID logic), but I am not sure.

Returning -EINVAL would break even current QEMU users, because they
might have '-cpu host' with a guest that doesn't use PV_UNHALT.

-EINVAL without any interface changes could be done only for future
features. :/

>> I have patches that implement the latter for X2APIC and PV_UNHALT, but
>> I'm not sure if it's better than leaving the bug unfixed, because QEMU
>> doesn't use KVM_GET_CPUID2 and migration to older KVM would change
>> CPUID, which is a very subtle bug.
>> What do you think?
> 
> Implementing those checks basically mean moving the existing
> kvm_arch_get_supported_cpuid() logic inside KVM. If we do that,
> can we get an interface that will allow QEMU to just query KVM
> instead of duplicating that logic?

The QEMU code for old kernels likely has to stay so only future features
wouldn't be duplicated.

An interface to check what KVM disabled already exists, KVM_GET_CPUID2.
We'd be hijacking it for "SET -> GET -> compare" where QEMU would exit()
if features got cleared (and/or added?).

It an extension of existing practice -- new features would be notified
with a capability and SET+GET would check if they really can be enabled.

We'd need a toggle (a way for QEMU to tell KVM that it will GET and
compare) to allow KVM safely clear already released features.
e.g. migration wouldn't enable this feature.

The toggle for SET+GET would be very simple in the kernel, but might
turn out to be complicated in QEMU ...

See the rough idea at the bottom.

> Making KVM_SET_CPUID2 clear those bits and/or return -EINVAL
> would probably be enough as an interface, as long as QEMU has a
> way to know in advance if it needs to clear CPUID bits based on
> the legacy kvm_arch_get_supported_cpuid() code (for older
> kernels), or if it can just give everything to KVM_SET_CPUID2,
> and check for unsupported/cleared flags later (for newer
> kernels).

A new IOCTL that returns valid-but-not-in-supported and
invalid-from-supported CPUIDs and forces KVM to -EINVAL if userspace
doesn't listen is in the game too.  It is more complicated, though.

---8<---
Rough (doesn't even define everything) idea of the clearing approach.

---
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 739db9ab16b2..8d085823c4af 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -545,6 +545,9 @@ struct kvm_cpuid {
 	struct kvm_cpuid_entry entries[0];
 };
 
+If KVM_CAP_CLEAR_INVALID_CPUID is enabled, then the ioctls can modify CPUID
+bits to prevent invalid configurations.
+
 
 4.21 KVM_SET_SIGNAL_MASK
 
@@ -3900,6 +3903,18 @@ to take care of that.
 This capability can be enabled dynamically even if VCPUs were already
 created and are running.
 
+7.9 KVM_CAP_CLEAR_INVALID_CPUID
+
+Architectures: x86
+Parameters: none
+
+After setting this capability, KVM_SET_CPUID2 can modify CPUID bits that are
+invalid in the given configuration, e.g. disable KVM_FEATURE_PV_UNHALT when the
+LAPIC is in the userspace.
+
+The userspace is expected to use KVM_GET_CPUID2 to retrieve the final CPUID.
+
+
 8. Other capabilities.
 ----------------------
 
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index afa7bbb596cd..afa05bb31cc3 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -160,6 +160,21 @@ static void cpuid_fix_nx_cap(struct kvm_vcpu *vcpu)
 	}
 }
 
+static void kvm_clear_invalid_cpuid(struct kvm_vcpu *vcpu)
+{
+	struct kvm *kvm = vcpu->kvm;
+	struct kvm_cpuid_entry2 *best;
+
+	if (!kvm->clear_invalid_cpuid)
+		return;
+
+	best = kvm_find_cpuid_entry(vcpu, 1, 0);
+	if (best && !lapic_in_kernel(vcpu))
+		best->ecx &= ~F(X2APIC);
+
+	/* TODO: to disable KVM_FEATURE_PV_UNHALT */
+}
+
 int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
@@ -234,6 +249,8 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
 			   cpuid->nent * sizeof(struct kvm_cpuid_entry2)))
 		goto out;
 	vcpu->arch.cpuid_nent = cpuid->nent;
+
+	kvm_clear_invalid_cpuid(vcpu);
 	kvm_apic_set_version(vcpu);
 	kvm_x86_ops->cpuid_update(vcpu);
 	r = kvm_update_cpuid(vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 19f9f9e05c2a..99028df615e7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3812,6 +3812,13 @@ split_irqchip_unlock:
 
 		r = 0;
 		break;
+	case KVM_CAP_CLEAR_INVALID_CPUID:
+		r = -EEXIST;
+		if (kvm->created_vcpus)
+			break;
+		kvm->clear_invalid_cpuid = true;
+		r = 0;
+		break;
 	default:
 		r = -EINVAL;
 		break;

Re: GET_SUPPORTED_CPUID and irqchip (was Re: kvm_pv_unhalt and kernel_irqchip=off)

[Eduardo Habkost]

On Mon, Aug 15, 2016 at 07:50:59PM +0200, Radim Krčmář wrote:
[...]
> +static void kvm_clear_invalid_cpuid(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +	struct kvm_cpuid_entry2 *best;
> +
> +	if (!kvm->clear_invalid_cpuid)
> +		return;
> +
> +	best = kvm_find_cpuid_entry(vcpu, 1, 0);
> +	if (best && !lapic_in_kernel(vcpu))
> +		best->ecx &= ~F(X2APIC);

Isn't it possible to implement x2apic support in userspace APIC
using the current KVM interfaces?

-- 
Eduardo

Re: GET_SUPPORTED_CPUID and irqchip (was Re: kvm_pv_unhalt and kernel_irqchip=off)

[Radim Krčmář]

2016-08-15 15:00-0300, Eduardo Habkost:
> On Mon, Aug 15, 2016 at 07:50:59PM +0200, Radim Krčmář wrote:
> [...]
>> +static void kvm_clear_invalid_cpuid(struct kvm_vcpu *vcpu)
>> +{
>> +	struct kvm *kvm = vcpu->kvm;
>> +	struct kvm_cpuid_entry2 *best;
>> +
>> +	if (!kvm->clear_invalid_cpuid)
>> +		return;
>> +
>> +	best = kvm_find_cpuid_entry(vcpu, 1, 0);
>> +	if (best && !lapic_in_kernel(vcpu))
>> +		best->ecx &= ~F(X2APIC);
> 
> Isn't it possible to implement x2apic support in userspace APIC
> using the current KVM interfaces?

No, MSR accesses never get to userspace.  We'll probably implement it in
the future as there already has been a proposal for unhandled MSRs.