* [PATCH 0/5] Some fixes for HV KVM on PPC
@ 2014-11-03 4:51 ` Paul Mackerras
0 siblings, 0 replies; 26+ messages in thread
From: Paul Mackerras @ 2014-11-03 4:51 UTC (permalink / raw)
To: Alexander Graf, kvm-ppc; +Cc: kvm
Here are fixes for five bugs which were found in the testing of our
PowerKVM product. The bugs range from guest performance issues to
guest crashes and memory corruption. Please apply.
Paul.
---
arch/powerpc/include/asm/kvm_book3s_64.h | 2 +-
arch/powerpc/kvm/book3s_hv.c | 22 ++++++++++++++++
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 44 ++++++++++++--------------------
arch/powerpc/kvm/book3s_hv_rm_xics.c | 36 ++++++++++++++++++++------
arch/powerpc/kvm/book3s_xics.c | 30 +++++++++++++++++-----
arch/powerpc/kvm/book3s_xics.h | 1 +
6 files changed, 92 insertions(+), 43 deletions(-)
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/5] KVM: PPC: Book3S HV: Fix computation of tlbie operand
2014-11-03 4:51 ` Paul Mackerras
@ 2014-11-03 4:51 ` Paul Mackerras
-1 siblings, 0 replies; 26+ messages in thread
From: Paul Mackerras @ 2014-11-03 4:51 UTC (permalink / raw)
To: Alexander Graf, kvm-ppc; +Cc: kvm, Paul Mackerras, Aneesh Kumar K.V
The B (segment size) field in the RB operand for the tlbie
instruction is two bits, which we get from the top two bits of
the first doubleword of the HPT entry to be invalidated. These
bits go in bits 8 and 9 of the RB operand (bits 54 and 55 in IBM
bit numbering).
The compute_tlbie_rb() function gets these bits as v >> (62 - 8),
which is not correct as it will bring in the top 10 bits, not
just the top two. These extra bits could corrupt the AP, AVAL
and L fields in the RB value. To fix this we shift right 62 bits
and then shift left 8 bits, so we only get the two bits of the
B field.
The first doubleword of the HPT entry is under the control of the
guest kernel. In fact, Linux guests will always put zeroes in bits
54 -- 61 (IBM bits 2 -- 9), but we should not rely on guests doing
this.
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/include/asm/kvm_book3s_64.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
index 0aa8179..a37f1a4 100644
--- a/arch/powerpc/include/asm/kvm_book3s_64.h
+++ b/arch/powerpc/include/asm/kvm_book3s_64.h
@@ -148,7 +148,7 @@ static inline unsigned long compute_tlbie_rb(unsigned long v, unsigned long r,
/* This covers 14..54 bits of va*/
rb = (v & ~0x7fUL) << 16; /* AVA field */
- rb |= v >> (62 - 8); /* B field */
+ rb |= (v >> HPTE_V_SSIZE_SHIFT) << 8; /* B field */
/*
* AVA in v had cleared lower 23 bits. We need to derive
* that from pteg index
--
2.1.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 1/5] KVM: PPC: Book3S HV: Fix computation of tlbie operand
@ 2014-11-03 4:51 ` Paul Mackerras
0 siblings, 0 replies; 26+ messages in thread
From: Paul Mackerras @ 2014-11-03 4:51 UTC (permalink / raw)
To: Alexander Graf, kvm-ppc; +Cc: kvm, Paul Mackerras, Aneesh Kumar K.V
The B (segment size) field in the RB operand for the tlbie
instruction is two bits, which we get from the top two bits of
the first doubleword of the HPT entry to be invalidated. These
bits go in bits 8 and 9 of the RB operand (bits 54 and 55 in IBM
bit numbering).
The compute_tlbie_rb() function gets these bits as v >> (62 - 8),
which is not correct as it will bring in the top 10 bits, not
just the top two. These extra bits could corrupt the AP, AVAL
and L fields in the RB value. To fix this we shift right 62 bits
and then shift left 8 bits, so we only get the two bits of the
B field.
The first doubleword of the HPT entry is under the control of the
guest kernel. In fact, Linux guests will always put zeroes in bits
54 -- 61 (IBM bits 2 -- 9), but we should not rely on guests doing
this.
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/include/asm/kvm_book3s_64.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
index 0aa8179..a37f1a4 100644
--- a/arch/powerpc/include/asm/kvm_book3s_64.h
+++ b/arch/powerpc/include/asm/kvm_book3s_64.h
@@ -148,7 +148,7 @@ static inline unsigned long compute_tlbie_rb(unsigned long v, unsigned long r,
/* This covers 14..54 bits of va*/
rb = (v & ~0x7fUL) << 16; /* AVA field */
- rb |= v >> (62 - 8); /* B field */
+ rb |= (v >> HPTE_V_SSIZE_SHIFT) << 8; /* B field */
/*
* AVA in v had cleared lower 23 bits. We need to derive
* that from pteg index
--
2.1.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] KVM: PPC: Book3S HV: Fix computation of tlbie operand
2014-11-03 4:51 ` Paul Mackerras
@ 2014-11-03 6:11 ` Aneesh Kumar K.V
-1 siblings, 0 replies; 26+ messages in thread
From: Aneesh Kumar K.V @ 2014-11-03 5:59 UTC (permalink / raw)
To: Paul Mackerras, Alexander Graf, kvm-ppc; +Cc: kvm, Paul Mackerras
Paul Mackerras <paulus@samba.org> writes:
> The B (segment size) field in the RB operand for the tlbie
> instruction is two bits, which we get from the top two bits of
> the first doubleword of the HPT entry to be invalidated. These
> bits go in bits 8 and 9 of the RB operand (bits 54 and 55 in IBM
> bit numbering).
>
> The compute_tlbie_rb() function gets these bits as v >> (62 - 8),
> which is not correct as it will bring in the top 10 bits, not
> just the top two. These extra bits could corrupt the AP, AVAL
> and L fields in the RB value. To fix this we shift right 62 bits
> and then shift left 8 bits, so we only get the two bits of the
> B field.
Good catch.
>
> The first doubleword of the HPT entry is under the control of the
> guest kernel. In fact, Linux guests will always put zeroes in bits
> 54 -- 61 (IBM bits 2 -- 9), but we should not rely on guests doing
> this.
>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Signed-off-by: Paul Mackerras <paulus@samba.org>
Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/kvm_book3s_64.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
> index 0aa8179..a37f1a4 100644
> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
> @@ -148,7 +148,7 @@ static inline unsigned long compute_tlbie_rb(unsigned long v, unsigned long r,
> /* This covers 14..54 bits of va*/
> rb = (v & ~0x7fUL) << 16; /* AVA field */
>
> - rb |= v >> (62 - 8); /* B field */
> + rb |= (v >> HPTE_V_SSIZE_SHIFT) << 8; /* B field */
> /*
> * AVA in v had cleared lower 23 bits. We need to derive
> * that from pteg index
> --
> 2.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] KVM: PPC: Book3S HV: Fix computation of tlbie operand
@ 2014-11-03 6:11 ` Aneesh Kumar K.V
0 siblings, 0 replies; 26+ messages in thread
From: Aneesh Kumar K.V @ 2014-11-03 6:11 UTC (permalink / raw)
To: Paul Mackerras, Alexander Graf, kvm-ppc; +Cc: kvm, Paul Mackerras
Paul Mackerras <paulus@samba.org> writes:
> The B (segment size) field in the RB operand for the tlbie
> instruction is two bits, which we get from the top two bits of
> the first doubleword of the HPT entry to be invalidated. These
> bits go in bits 8 and 9 of the RB operand (bits 54 and 55 in IBM
> bit numbering).
>
> The compute_tlbie_rb() function gets these bits as v >> (62 - 8),
> which is not correct as it will bring in the top 10 bits, not
> just the top two. These extra bits could corrupt the AP, AVAL
> and L fields in the RB value. To fix this we shift right 62 bits
> and then shift left 8 bits, so we only get the two bits of the
> B field.
Good catch.
>
> The first doubleword of the HPT entry is under the control of the
> guest kernel. In fact, Linux guests will always put zeroes in bits
> 54 -- 61 (IBM bits 2 -- 9), but we should not rely on guests doing
> this.
>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Signed-off-by: Paul Mackerras <paulus@samba.org>
Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/kvm_book3s_64.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
> index 0aa8179..a37f1a4 100644
> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
> @@ -148,7 +148,7 @@ static inline unsigned long compute_tlbie_rb(unsigned long v, unsigned long r,
> /* This covers 14..54 bits of va*/
> rb = (v & ~0x7fUL) << 16; /* AVA field */
>
> - rb |= v >> (62 - 8); /* B field */
> + rb |= (v >> HPTE_V_SSIZE_SHIFT) << 8; /* B field */
> /*
> * AVA in v had cleared lower 23 bits. We need to derive
> * that from pteg index
> --
> 2.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/5] KVM: PPC: Book3S HV: Fix computation of tlbie operand
2014-11-03 6:11 ` Aneesh Kumar K.V
@ 2014-11-03 6:41 ` Aneesh Kumar K.V
-1 siblings, 0 replies; 26+ messages in thread
From: Aneesh Kumar K.V @ 2014-11-03 6:29 UTC (permalink / raw)
To: Paul Mackerras, Alexander Graf, kvm-ppc; +Cc: kvm, Paul Mackerras
"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
> Paul Mackerras <paulus@samba.org> writes:
>
>> The B (segment size) field in the RB operand for the tlbie
>> instruction is two bits, which we get from the top two bits of
>> the first doubleword of the HPT entry to be invalidated. These
>> bits go in bits 8 and 9 of the RB operand (bits 54 and 55 in IBM
>> bit numbering).
>>
>> The compute_tlbie_rb() function gets these bits as v >> (62 - 8),
>> which is not correct as it will bring in the top 10 bits, not
>> just the top two. These extra bits could corrupt the AP, AVAL
>> and L fields in the RB value. To fix this we shift right 62 bits
>> and then shift left 8 bits, so we only get the two bits of the
>> B field.
>
> Good catch.
>
>>
>> The first doubleword of the HPT entry is under the control of the
>> guest kernel. In fact, Linux guests will always put zeroes in bits
>> 54 -- 61 (IBM bits 2 -- 9), but we should not rely on guests doing
>> this.
>>
>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> Signed-off-by: Paul Mackerras <paulus@samba.org>
>
>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>
>> ---
>> arch/powerpc/include/asm/kvm_book3s_64.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
>> index 0aa8179..a37f1a4 100644
>> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
>> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
>> @@ -148,7 +148,7 @@ static inline unsigned long compute_tlbie_rb(unsigned long v, unsigned long r,
>> /* This covers 14..54 bits of va*/
>> rb = (v & ~0x7fUL) << 16; /* AVA field */
>>
>> - rb |= v >> (62 - 8); /* B field */
>> + rb |= (v >> HPTE_V_SSIZE_SHIFT) << 8; /* B field */
or should we do. I guess the below is more closer to what we have in
rest of the code ?
rb |= ((v >> (HPTE_V_SSIZE_SHIFT - 8)) & ~0xffUL);
>> /*
>> * AVA in v had cleared lower 23 bits. We need to derive
>> * that from pteg index
>> --
>> 2.1.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/5] KVM: PPC: Book3S HV: Fix computation of tlbie operand
@ 2014-11-03 6:41 ` Aneesh Kumar K.V
0 siblings, 0 replies; 26+ messages in thread
From: Aneesh Kumar K.V @ 2014-11-03 6:41 UTC (permalink / raw)
To: Paul Mackerras, Alexander Graf, kvm-ppc; +Cc: kvm, Paul Mackerras
"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:
> Paul Mackerras <paulus@samba.org> writes:
>
>> The B (segment size) field in the RB operand for the tlbie
>> instruction is two bits, which we get from the top two bits of
>> the first doubleword of the HPT entry to be invalidated. These
>> bits go in bits 8 and 9 of the RB operand (bits 54 and 55 in IBM
>> bit numbering).
>>
>> The compute_tlbie_rb() function gets these bits as v >> (62 - 8),
>> which is not correct as it will bring in the top 10 bits, not
>> just the top two. These extra bits could corrupt the AP, AVAL
>> and L fields in the RB value. To fix this we shift right 62 bits
>> and then shift left 8 bits, so we only get the two bits of the
>> B field.
>
> Good catch.
>
>>
>> The first doubleword of the HPT entry is under the control of the
>> guest kernel. In fact, Linux guests will always put zeroes in bits
>> 54 -- 61 (IBM bits 2 -- 9), but we should not rely on guests doing
>> this.
>>
>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> Signed-off-by: Paul Mackerras <paulus@samba.org>
>
>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>
>> ---
>> arch/powerpc/include/asm/kvm_book3s_64.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
>> index 0aa8179..a37f1a4 100644
>> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
>> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
>> @@ -148,7 +148,7 @@ static inline unsigned long compute_tlbie_rb(unsigned long v, unsigned long r,
>> /* This covers 14..54 bits of va*/
>> rb = (v & ~0x7fUL) << 16; /* AVA field */
>>
>> - rb |= v >> (62 - 8); /* B field */
>> + rb |= (v >> HPTE_V_SSIZE_SHIFT) << 8; /* B field */
or should we do. I guess the below is more closer to what we have in
rest of the code ?
rb |= ((v >> (HPTE_V_SSIZE_SHIFT - 8)) & ~0xffUL);
>> /*
>> * AVA in v had cleared lower 23 bits. We need to derive
>> * that from pteg index
>> --
>> 2.1.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/5] KVM: PPC: Book3S HV: Fix an issue where guest is paused on receiving HMI
2014-11-03 4:51 ` Paul Mackerras
@ 2014-11-03 4:51 ` Paul Mackerras
-1 siblings, 0 replies; 26+ messages in thread
From: Paul Mackerras @ 2014-11-03 4:51 UTC (permalink / raw)
To: Alexander Graf, kvm-ppc; +Cc: kvm, Mahesh Salgaonkar, Paul Mackerras
From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
When we get an HMI (hypervisor maintenance interrupt) while in a
guest, we see that guest enters into paused state. The reason is, in
kvmppc_handle_exit_hv it falls through default path and returns to
host instead of resuming guest. This causes guest to enter into
paused state. HMI is a hypervisor only interrupt and it is safe to
resume the guest since the host has handled it already. This patch
adds a switch case to resume the guest.
Without this patch we see guest entering into paused state with following
console messages:
[ 3003.329351] Severe Hypervisor Maintenance interrupt [Recovered]
[ 3003.329356] Error detail: Timer facility experienced an error
[ 3003.329359] HMER: 0840000000000000
[ 3003.329360] TFMR: 4a12000980a84000
[ 3003.329366] vcpu c0000007c35094c0 (40):
[ 3003.329368] pc = c0000000000c2ba0 msr = 8000000000009032 trap = e60
[ 3003.329370] r 0 = c00000000021ddc0 r16 = 0000000000000046
[ 3003.329372] r 1 = c00000007a02bbd0 r17 = 00003ffff27d5d98
[ 3003.329375] r 2 = c0000000010980b8 r18 = 00001fffffc9a0b0
[ 3003.329377] r 3 = c00000000142d6b8 r19 = c00000000142d6b8
[ 3003.329379] r 4 = 0000000000000002 r20 = 0000000000000000
[ 3003.329381] r 5 = c00000000524a110 r21 = 0000000000000000
[ 3003.329383] r 6 = 0000000000000001 r22 = 0000000000000000
[ 3003.329386] r 7 = 0000000000000000 r23 = c00000000524a110
[ 3003.329388] r 8 = 0000000000000000 r24 = 0000000000000001
[ 3003.329391] r 9 = 0000000000000001 r25 = c00000007c31da38
[ 3003.329393] r10 = c0000000014280b8 r26 = 0000000000000002
[ 3003.329395] r11 = 746f6f6c2f68656c r27 = c00000000524a110
[ 3003.329397] r12 = 0000000028004484 r28 = c00000007c31da38
[ 3003.329399] r13 = c00000000fe01400 r29 = 0000000000000002
[ 3003.329401] r14 = 0000000000000046 r30 = c000000003011e00
[ 3003.329403] r15 = ffffffffffffffba r31 = 0000000000000002
[ 3003.329404] ctr = c00000000041a670 lr = c000000000272520
[ 3003.329405] srr0 = c00000000007e8d8 srr1 = 9000000000001002
[ 3003.329406] sprg0 = 0000000000000000 sprg1 = c00000000fe01400
[ 3003.329407] sprg2 = c00000000fe01400 sprg3 = 0000000000000005
[ 3003.329408] cr = 48004482 xer = 2000000000000000 dsisr = 42000000
[ 3003.329409] dar = 0000010015020048
[ 3003.329410] fault dar = 0000010015020048 dsisr = 42000000
[ 3003.329411] SLB (8 entries):
[ 3003.329412] ESID = c000000008000000 VSID = 40016e7779000510
[ 3003.329413] ESID = d000000008000001 VSID = 400142add1000510
[ 3003.329414] ESID = f000000008000004 VSID = 4000eb1a81000510
[ 3003.329415] ESID = 00001f000800000b VSID = 40004fda0a000d90
[ 3003.329416] ESID = 00003f000800000c VSID = 400039f536000d90
[ 3003.329417] ESID = 000000001800000d VSID = 0001251b35150d90
[ 3003.329417] ESID = 000001000800000e VSID = 4001e46090000d90
[ 3003.329418] ESID = d000080008000019 VSID = 40013d349c000400
[ 3003.329419] lpcr = c048800001847001 sdr1 = 0000001b19000006 last_inst = ffffffff
[ 3003.329421] trap=0xe60 | pc=0xc0000000000c2ba0 | msr=0x8000000000009032
[ 3003.329524] Severe Hypervisor Maintenance interrupt [Recovered]
[ 3003.329526] Error detail: Timer facility experienced an error
[ 3003.329527] HMER: 0840000000000000
[ 3003.329527] TFMR: 4a12000980a94000
[ 3006.359786] Severe Hypervisor Maintenance interrupt [Recovered]
[ 3006.359792] Error detail: Timer facility experienced an error
[ 3006.359795] HMER: 0840000000000000
[ 3006.359797] TFMR: 4a12000980a84000
Id Name State
----------------------------------------------------
2 guest2 running
3 guest3 paused
4 guest4 running
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/kvm/book3s_hv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index e63587d..cd7e030 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -769,6 +769,8 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
vcpu->stat.ext_intr_exits++;
r = RESUME_GUEST;
break;
+ /* HMI is hypervisor interrupt and host has handled it. Resume guest.*/
+ case BOOK3S_INTERRUPT_HMI:
case BOOK3S_INTERRUPT_PERFMON:
r = RESUME_GUEST;
break;
--
2.1.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/5] KVM: PPC: Book3S HV: Fix an issue where guest is paused on receiving HMI
@ 2014-11-03 4:51 ` Paul Mackerras
0 siblings, 0 replies; 26+ messages in thread
From: Paul Mackerras @ 2014-11-03 4:51 UTC (permalink / raw)
To: Alexander Graf, kvm-ppc; +Cc: kvm, Mahesh Salgaonkar, Paul Mackerras
From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
When we get an HMI (hypervisor maintenance interrupt) while in a
guest, we see that guest enters into paused state. The reason is, in
kvmppc_handle_exit_hv it falls through default path and returns to
host instead of resuming guest. This causes guest to enter into
paused state. HMI is a hypervisor only interrupt and it is safe to
resume the guest since the host has handled it already. This patch
adds a switch case to resume the guest.
Without this patch we see guest entering into paused state with following
console messages:
[ 3003.329351] Severe Hypervisor Maintenance interrupt [Recovered]
[ 3003.329356] Error detail: Timer facility experienced an error
[ 3003.329359] HMER: 0840000000000000
[ 3003.329360] TFMR: 4a12000980a84000
[ 3003.329366] vcpu c0000007c35094c0 (40):
[ 3003.329368] pc = c0000000000c2ba0 msr = 8000000000009032 trap = e60
[ 3003.329370] r 0 = c00000000021ddc0 r16 = 0000000000000046
[ 3003.329372] r 1 = c00000007a02bbd0 r17 = 00003ffff27d5d98
[ 3003.329375] r 2 = c0000000010980b8 r18 = 00001fffffc9a0b0
[ 3003.329377] r 3 = c00000000142d6b8 r19 = c00000000142d6b8
[ 3003.329379] r 4 = 0000000000000002 r20 = 0000000000000000
[ 3003.329381] r 5 = c00000000524a110 r21 = 0000000000000000
[ 3003.329383] r 6 = 0000000000000001 r22 = 0000000000000000
[ 3003.329386] r 7 = 0000000000000000 r23 = c00000000524a110
[ 3003.329388] r 8 = 0000000000000000 r24 = 0000000000000001
[ 3003.329391] r 9 = 0000000000000001 r25 = c00000007c31da38
[ 3003.329393] r10 = c0000000014280b8 r26 = 0000000000000002
[ 3003.329395] r11 = 746f6f6c2f68656c r27 = c00000000524a110
[ 3003.329397] r12 = 0000000028004484 r28 = c00000007c31da38
[ 3003.329399] r13 = c00000000fe01400 r29 = 0000000000000002
[ 3003.329401] r14 = 0000000000000046 r30 = c000000003011e00
[ 3003.329403] r15 = ffffffffffffffba r31 = 0000000000000002
[ 3003.329404] ctr = c00000000041a670 lr = c000000000272520
[ 3003.329405] srr0 = c00000000007e8d8 srr1 = 9000000000001002
[ 3003.329406] sprg0 = 0000000000000000 sprg1 = c00000000fe01400
[ 3003.329407] sprg2 = c00000000fe01400 sprg3 = 0000000000000005
[ 3003.329408] cr = 48004482 xer = 2000000000000000 dsisr = 42000000
[ 3003.329409] dar = 0000010015020048
[ 3003.329410] fault dar = 0000010015020048 dsisr = 42000000
[ 3003.329411] SLB (8 entries):
[ 3003.329412] ESID = c000000008000000 VSID = 40016e7779000510
[ 3003.329413] ESID = d000000008000001 VSID = 400142add1000510
[ 3003.329414] ESID = f000000008000004 VSID = 4000eb1a81000510
[ 3003.329415] ESID = 00001f000800000b VSID = 40004fda0a000d90
[ 3003.329416] ESID = 00003f000800000c VSID = 400039f536000d90
[ 3003.329417] ESID = 000000001800000d VSID = 0001251b35150d90
[ 3003.329417] ESID = 000001000800000e VSID = 4001e46090000d90
[ 3003.329418] ESID = d000080008000019 VSID = 40013d349c000400
[ 3003.329419] lpcr = c048800001847001 sdr1 = 0000001b19000006 last_inst = ffffffff
[ 3003.329421] trap=0xe60 | pc=0xc0000000000c2ba0 | msr=0x8000000000009032
[ 3003.329524] Severe Hypervisor Maintenance interrupt [Recovered]
[ 3003.329526] Error detail: Timer facility experienced an error
[ 3003.329527] HMER: 0840000000000000
[ 3003.329527] TFMR: 4a12000980a94000
[ 3006.359786] Severe Hypervisor Maintenance interrupt [Recovered]
[ 3006.359792] Error detail: Timer facility experienced an error
[ 3006.359795] HMER: 0840000000000000
[ 3006.359797] TFMR: 4a12000980a84000
Id Name State
----------------------------------------------------
2 guest2 running
3 guest3 paused
4 guest4 running
Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/kvm/book3s_hv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index e63587d..cd7e030 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -769,6 +769,8 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
vcpu->stat.ext_intr_exits++;
r = RESUME_GUEST;
break;
+ /* HMI is hypervisor interrupt and host has handled it. Resume guest.*/
+ case BOOK3S_INTERRUPT_HMI:
case BOOK3S_INTERRUPT_PERFMON:
r = RESUME_GUEST;
break;
--
2.1.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] KVM: PPC: Book3S HV: Fix an issue where guest is paused on receiving HMI
2014-11-03 4:51 ` Paul Mackerras
@ 2014-11-20 17:32 ` Alexander Graf
-1 siblings, 0 replies; 26+ messages in thread
From: Alexander Graf @ 2014-11-20 17:32 UTC (permalink / raw)
To: Paul Mackerras, kvm-ppc; +Cc: kvm, Mahesh Salgaonkar
On 03.11.14 05:51, Paul Mackerras wrote:
> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>
> When we get an HMI (hypervisor maintenance interrupt) while in a
> guest, we see that guest enters into paused state. The reason is, in
> kvmppc_handle_exit_hv it falls through default path and returns to
> host instead of resuming guest. This causes guest to enter into
> paused state. HMI is a hypervisor only interrupt and it is safe to
> resume the guest since the host has handled it already. This patch
> adds a switch case to resume the guest.
>
> Without this patch we see guest entering into paused state with following
> console messages:
>
> [ 3003.329351] Severe Hypervisor Maintenance interrupt [Recovered]
> [ 3003.329356] Error detail: Timer facility experienced an error
> [ 3003.329359] HMER: 0840000000000000
> [ 3003.329360] TFMR: 4a12000980a84000
> [ 3003.329366] vcpu c0000007c35094c0 (40):
> [ 3003.329368] pc = c0000000000c2ba0 msr = 8000000000009032 trap = e60
> [ 3003.329370] r 0 = c00000000021ddc0 r16 = 0000000000000046
> [ 3003.329372] r 1 = c00000007a02bbd0 r17 = 00003ffff27d5d98
> [ 3003.329375] r 2 = c0000000010980b8 r18 = 00001fffffc9a0b0
> [ 3003.329377] r 3 = c00000000142d6b8 r19 = c00000000142d6b8
> [ 3003.329379] r 4 = 0000000000000002 r20 = 0000000000000000
> [ 3003.329381] r 5 = c00000000524a110 r21 = 0000000000000000
> [ 3003.329383] r 6 = 0000000000000001 r22 = 0000000000000000
> [ 3003.329386] r 7 = 0000000000000000 r23 = c00000000524a110
> [ 3003.329388] r 8 = 0000000000000000 r24 = 0000000000000001
> [ 3003.329391] r 9 = 0000000000000001 r25 = c00000007c31da38
> [ 3003.329393] r10 = c0000000014280b8 r26 = 0000000000000002
> [ 3003.329395] r11 = 746f6f6c2f68656c r27 = c00000000524a110
> [ 3003.329397] r12 = 0000000028004484 r28 = c00000007c31da38
> [ 3003.329399] r13 = c00000000fe01400 r29 = 0000000000000002
> [ 3003.329401] r14 = 0000000000000046 r30 = c000000003011e00
> [ 3003.329403] r15 = ffffffffffffffba r31 = 0000000000000002
> [ 3003.329404] ctr = c00000000041a670 lr = c000000000272520
> [ 3003.329405] srr0 = c00000000007e8d8 srr1 = 9000000000001002
> [ 3003.329406] sprg0 = 0000000000000000 sprg1 = c00000000fe01400
> [ 3003.329407] sprg2 = c00000000fe01400 sprg3 = 0000000000000005
> [ 3003.329408] cr = 48004482 xer = 2000000000000000 dsisr = 42000000
> [ 3003.329409] dar = 0000010015020048
> [ 3003.329410] fault dar = 0000010015020048 dsisr = 42000000
> [ 3003.329411] SLB (8 entries):
> [ 3003.329412] ESID = c000000008000000 VSID = 40016e7779000510
> [ 3003.329413] ESID = d000000008000001 VSID = 400142add1000510
> [ 3003.329414] ESID = f000000008000004 VSID = 4000eb1a81000510
> [ 3003.329415] ESID = 00001f000800000b VSID = 40004fda0a000d90
> [ 3003.329416] ESID = 00003f000800000c VSID = 400039f536000d90
> [ 3003.329417] ESID = 000000001800000d VSID = 0001251b35150d90
> [ 3003.329417] ESID = 000001000800000e VSID = 4001e46090000d90
> [ 3003.329418] ESID = d000080008000019 VSID = 40013d349c000400
> [ 3003.329419] lpcr = c048800001847001 sdr1 = 0000001b19000006 last_inst = ffffffff
> [ 3003.329421] trap=0xe60 | pc=0xc0000000000c2ba0 | msr=0x8000000000009032
> [ 3003.329524] Severe Hypervisor Maintenance interrupt [Recovered]
> [ 3003.329526] Error detail: Timer facility experienced an error
> [ 3003.329527] HMER: 0840000000000000
> [ 3003.329527] TFMR: 4a12000980a94000
> [ 3006.359786] Severe Hypervisor Maintenance interrupt [Recovered]
> [ 3006.359792] Error detail: Timer facility experienced an error
> [ 3006.359795] HMER: 0840000000000000
> [ 3006.359797] TFMR: 4a12000980a84000
>
> Id Name State
> ----------------------------------------------------
> 2 guest2 running
> 3 guest3 paused
> 4 guest4 running
>
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> Signed-off-by: Paul Mackerras <paulus@samba.org>
Do we need this for PR running on bare metal as well?
Alex
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/5] KVM: PPC: Book3S HV: Fix an issue where guest is paused on receiving HMI
@ 2014-11-20 17:32 ` Alexander Graf
0 siblings, 0 replies; 26+ messages in thread
From: Alexander Graf @ 2014-11-20 17:32 UTC (permalink / raw)
To: Paul Mackerras, kvm-ppc; +Cc: kvm, Mahesh Salgaonkar
On 03.11.14 05:51, Paul Mackerras wrote:
> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>
> When we get an HMI (hypervisor maintenance interrupt) while in a
> guest, we see that guest enters into paused state. The reason is, in
> kvmppc_handle_exit_hv it falls through default path and returns to
> host instead of resuming guest. This causes guest to enter into
> paused state. HMI is a hypervisor only interrupt and it is safe to
> resume the guest since the host has handled it already. This patch
> adds a switch case to resume the guest.
>
> Without this patch we see guest entering into paused state with following
> console messages:
>
> [ 3003.329351] Severe Hypervisor Maintenance interrupt [Recovered]
> [ 3003.329356] Error detail: Timer facility experienced an error
> [ 3003.329359] HMER: 0840000000000000
> [ 3003.329360] TFMR: 4a12000980a84000
> [ 3003.329366] vcpu c0000007c35094c0 (40):
> [ 3003.329368] pc = c0000000000c2ba0 msr = 8000000000009032 trap = e60
> [ 3003.329370] r 0 = c00000000021ddc0 r16 = 0000000000000046
> [ 3003.329372] r 1 = c00000007a02bbd0 r17 = 00003ffff27d5d98
> [ 3003.329375] r 2 = c0000000010980b8 r18 = 00001fffffc9a0b0
> [ 3003.329377] r 3 = c00000000142d6b8 r19 = c00000000142d6b8
> [ 3003.329379] r 4 = 0000000000000002 r20 = 0000000000000000
> [ 3003.329381] r 5 = c00000000524a110 r21 = 0000000000000000
> [ 3003.329383] r 6 = 0000000000000001 r22 = 0000000000000000
> [ 3003.329386] r 7 = 0000000000000000 r23 = c00000000524a110
> [ 3003.329388] r 8 = 0000000000000000 r24 = 0000000000000001
> [ 3003.329391] r 9 = 0000000000000001 r25 = c00000007c31da38
> [ 3003.329393] r10 = c0000000014280b8 r26 = 0000000000000002
> [ 3003.329395] r11 = 746f6f6c2f68656c r27 = c00000000524a110
> [ 3003.329397] r12 = 0000000028004484 r28 = c00000007c31da38
> [ 3003.329399] r13 = c00000000fe01400 r29 = 0000000000000002
> [ 3003.329401] r14 = 0000000000000046 r30 = c000000003011e00
> [ 3003.329403] r15 = ffffffffffffffba r31 = 0000000000000002
> [ 3003.329404] ctr = c00000000041a670 lr = c000000000272520
> [ 3003.329405] srr0 = c00000000007e8d8 srr1 = 9000000000001002
> [ 3003.329406] sprg0 = 0000000000000000 sprg1 = c00000000fe01400
> [ 3003.329407] sprg2 = c00000000fe01400 sprg3 = 0000000000000005
> [ 3003.329408] cr = 48004482 xer = 2000000000000000 dsisr = 42000000
> [ 3003.329409] dar = 0000010015020048
> [ 3003.329410] fault dar = 0000010015020048 dsisr = 42000000
> [ 3003.329411] SLB (8 entries):
> [ 3003.329412] ESID = c000000008000000 VSID = 40016e7779000510
> [ 3003.329413] ESID = d000000008000001 VSID = 400142add1000510
> [ 3003.329414] ESID = f000000008000004 VSID = 4000eb1a81000510
> [ 3003.329415] ESID = 00001f000800000b VSID = 40004fda0a000d90
> [ 3003.329416] ESID = 00003f000800000c VSID = 400039f536000d90
> [ 3003.329417] ESID = 000000001800000d VSID = 0001251b35150d90
> [ 3003.329417] ESID = 000001000800000e VSID = 4001e46090000d90
> [ 3003.329418] ESID = d000080008000019 VSID = 40013d349c000400
> [ 3003.329419] lpcr = c048800001847001 sdr1 = 0000001b19000006 last_inst = ffffffff
> [ 3003.329421] trap=0xe60 | pc=0xc0000000000c2ba0 | msr=0x8000000000009032
> [ 3003.329524] Severe Hypervisor Maintenance interrupt [Recovered]
> [ 3003.329526] Error detail: Timer facility experienced an error
> [ 3003.329527] HMER: 0840000000000000
> [ 3003.329527] TFMR: 4a12000980a94000
> [ 3006.359786] Severe Hypervisor Maintenance interrupt [Recovered]
> [ 3006.359792] Error detail: Timer facility experienced an error
> [ 3006.359795] HMER: 0840000000000000
> [ 3006.359797] TFMR: 4a12000980a84000
>
> Id Name State
> ----------------------------------------------------
> 2 guest2 running
> 3 guest3 paused
> 4 guest4 running
>
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> Signed-off-by: Paul Mackerras <paulus@samba.org>
Do we need this for PR running on bare metal as well?
Alex
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/5] KVM: PPC: Book3S HV: Fix KSM memory corruption
2014-11-03 4:51 ` Paul Mackerras
@ 2014-11-03 4:51 ` Paul Mackerras
-1 siblings, 0 replies; 26+ messages in thread
From: Paul Mackerras @ 2014-11-03 4:51 UTC (permalink / raw)
To: Alexander Graf, kvm-ppc; +Cc: kvm, Paul Mackerras
Testing with KSM active in the host showed occasional corruption of
guest memory. Typically a page that should have contained zeroes
would contain values that look like the contents of a user process
stack (values such as 0x0000_3fff_xxxx_xxx).
Code inspection in kvmppc_h_protect revealed that there was a race
condition with the possibility of granting write access to a page
which is read-only in the host page tables. The code attempts to keep
the host mapping read-only if the host userspace PTE is read-only, but
if that PTE had been temporarily made invalid for any reason, the
read-only check would not trigger and the host HPTE could end up
read-write. Examination of the guest HPT in the failure situation
revealed that there were indeed shared pages which should have been
read-only that were mapped read-write.
To close this race, we don't let a page go from being read-only to
being read-write, as far as the real HPTE mapping the page is
concerned (the guest view can go to read-write, but the actual mapping
stays read-only). When the guest tries to write to the page, we take
an HDSI and let kvmppc_book3s_hv_page_fault take care of providing a
writable HPTE for the page.
This eliminates the occasional corruption of shared pages
that was previously seen with KSM active.
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 44 ++++++++++++++-----------------------
1 file changed, 17 insertions(+), 27 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 084ad54..411720f 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -667,40 +667,30 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, unsigned long flags,
rev->guest_rpte = r;
note_hpte_modification(kvm, rev);
}
- r = (be64_to_cpu(hpte[1]) & ~mask) | bits;
/* Update HPTE */
if (v & HPTE_V_VALID) {
- rb = compute_tlbie_rb(v, r, pte_index);
- hpte[0] = cpu_to_be64(v & ~HPTE_V_VALID);
- do_tlbies(kvm, &rb, 1, global_invalidates(kvm, flags), true);
/*
- * If the host has this page as readonly but the guest
- * wants to make it read/write, reduce the permissions.
- * Checking the host permissions involves finding the
- * memslot and then the Linux PTE for the page.
+ * If the page is valid, don't let it transition from
+ * readonly to writable. If it should be writable, we'll
+ * take a trap and let the page fault code sort it out.
*/
- if (hpte_is_writable(r) && kvm->arch.using_mmu_notifiers) {
- unsigned long psize, gfn, hva;
- struct kvm_memory_slot *memslot;
- pgd_t *pgdir = vcpu->arch.pgdir;
- pte_t pte;
-
- psize = hpte_page_size(v, r);
- gfn = ((r & HPTE_R_RPN) & ~(psize - 1)) >> PAGE_SHIFT;
- memslot = __gfn_to_memslot(kvm_memslots_raw(kvm), gfn);
- if (memslot) {
- hva = __gfn_to_hva_memslot(memslot, gfn);
- pte = lookup_linux_pte_and_update(pgdir, hva,
- 1, &psize);
- if (pte_present(pte) && !pte_write(pte))
- r = hpte_make_readonly(r);
- }
+ pte = be64_to_cpu(hpte[1]);
+ r = (pte & ~mask) | bits;
+ if (hpte_is_writable(r) && kvm->arch.using_mmu_notifiers &&
+ !hpte_is_writable(pte))
+ r = hpte_make_readonly(r);
+ /* If the PTE is changing, invalidate it first */
+ if (r != pte) {
+ rb = compute_tlbie_rb(v, r, pte_index);
+ hpte[0] = cpu_to_be64((v & ~HPTE_V_VALID) |
+ HPTE_V_ABSENT);
+ do_tlbies(kvm, &rb, 1, global_invalidates(kvm, flags),
+ true);
+ hpte[1] = cpu_to_be64(r);
}
}
- hpte[1] = cpu_to_be64(r);
- eieio();
- hpte[0] = cpu_to_be64(v & ~HPTE_V_HVLOCK);
+ unlock_hpte(hpte, v & ~HPTE_V_HVLOCK);
asm volatile("ptesync" : : : "memory");
return H_SUCCESS;
}
--
2.1.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 3/5] KVM: PPC: Book3S HV: Fix KSM memory corruption
@ 2014-11-03 4:51 ` Paul Mackerras
0 siblings, 0 replies; 26+ messages in thread
From: Paul Mackerras @ 2014-11-03 4:51 UTC (permalink / raw)
To: Alexander Graf, kvm-ppc; +Cc: kvm, Paul Mackerras
Testing with KSM active in the host showed occasional corruption of
guest memory. Typically a page that should have contained zeroes
would contain values that look like the contents of a user process
stack (values such as 0x0000_3fff_xxxx_xxx).
Code inspection in kvmppc_h_protect revealed that there was a race
condition with the possibility of granting write access to a page
which is read-only in the host page tables. The code attempts to keep
the host mapping read-only if the host userspace PTE is read-only, but
if that PTE had been temporarily made invalid for any reason, the
read-only check would not trigger and the host HPTE could end up
read-write. Examination of the guest HPT in the failure situation
revealed that there were indeed shared pages which should have been
read-only that were mapped read-write.
To close this race, we don't let a page go from being read-only to
being read-write, as far as the real HPTE mapping the page is
concerned (the guest view can go to read-write, but the actual mapping
stays read-only). When the guest tries to write to the page, we take
an HDSI and let kvmppc_book3s_hv_page_fault take care of providing a
writable HPTE for the page.
This eliminates the occasional corruption of shared pages
that was previously seen with KSM active.
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 44 ++++++++++++++-----------------------
1 file changed, 17 insertions(+), 27 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 084ad54..411720f 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -667,40 +667,30 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, unsigned long flags,
rev->guest_rpte = r;
note_hpte_modification(kvm, rev);
}
- r = (be64_to_cpu(hpte[1]) & ~mask) | bits;
/* Update HPTE */
if (v & HPTE_V_VALID) {
- rb = compute_tlbie_rb(v, r, pte_index);
- hpte[0] = cpu_to_be64(v & ~HPTE_V_VALID);
- do_tlbies(kvm, &rb, 1, global_invalidates(kvm, flags), true);
/*
- * If the host has this page as readonly but the guest
- * wants to make it read/write, reduce the permissions.
- * Checking the host permissions involves finding the
- * memslot and then the Linux PTE for the page.
+ * If the page is valid, don't let it transition from
+ * readonly to writable. If it should be writable, we'll
+ * take a trap and let the page fault code sort it out.
*/
- if (hpte_is_writable(r) && kvm->arch.using_mmu_notifiers) {
- unsigned long psize, gfn, hva;
- struct kvm_memory_slot *memslot;
- pgd_t *pgdir = vcpu->arch.pgdir;
- pte_t pte;
-
- psize = hpte_page_size(v, r);
- gfn = ((r & HPTE_R_RPN) & ~(psize - 1)) >> PAGE_SHIFT;
- memslot = __gfn_to_memslot(kvm_memslots_raw(kvm), gfn);
- if (memslot) {
- hva = __gfn_to_hva_memslot(memslot, gfn);
- pte = lookup_linux_pte_and_update(pgdir, hva,
- 1, &psize);
- if (pte_present(pte) && !pte_write(pte))
- r = hpte_make_readonly(r);
- }
+ pte = be64_to_cpu(hpte[1]);
+ r = (pte & ~mask) | bits;
+ if (hpte_is_writable(r) && kvm->arch.using_mmu_notifiers &&
+ !hpte_is_writable(pte))
+ r = hpte_make_readonly(r);
+ /* If the PTE is changing, invalidate it first */
+ if (r != pte) {
+ rb = compute_tlbie_rb(v, r, pte_index);
+ hpte[0] = cpu_to_be64((v & ~HPTE_V_VALID) |
+ HPTE_V_ABSENT);
+ do_tlbies(kvm, &rb, 1, global_invalidates(kvm, flags),
+ true);
+ hpte[1] = cpu_to_be64(r);
}
}
- hpte[1] = cpu_to_be64(r);
- eieio();
- hpte[0] = cpu_to_be64(v & ~HPTE_V_HVLOCK);
+ unlock_hpte(hpte, v & ~HPTE_V_HVLOCK);
asm volatile("ptesync" : : : "memory");
return H_SUCCESS;
}
--
2.1.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/5] KVM: PPC: Book3S HV: Fix inaccuracies in ICP emulation for H_IPI
2014-11-03 4:51 ` Paul Mackerras
@ 2014-11-03 4:51 ` Paul Mackerras
-1 siblings, 0 replies; 26+ messages in thread
From: Paul Mackerras @ 2014-11-03 4:51 UTC (permalink / raw)
To: Alexander Graf, kvm-ppc; +Cc: kvm, Suresh E. Warrier, Paul Mackerras
From: "Suresh E. Warrier" <warrier@linux.vnet.ibm.com>
This fixes some inaccuracies in the state machine for the virtualized
ICP when implementing the H_IPI hcall (Set_MFFR and related states):
1. The old code wipes out any pending interrupts when the new MFRR is
more favored than the CPPR but less favored than a pending
interrupt (by always modifying xisr and the pending_pri). This can
cause us to lose a pending external interrupt.
The correct code here is to only modify the pending_pri and xisr in
the ICP if the MFRR is equal to or more favored than the current
pending pri (since in this case, it is guaranteed that that there
cannot be a pending external interrupt). The code changes are
required in both kvmppc_rm_h_ipi and kvmppc_h_ipi.
2. Again, in both kvmppc_rm_h_ipi and kvmppc_h_ipi, there is a check
for whether MFRR is being made less favored AND further if new MFFR
is also less favored than the current CPPR, we check for any
resends pending in the ICP. These checks look like they are
designed to cover the case where if the MFRR is being made less
favored, we opportunistically trigger a resend of any interrupts
that had been previously rejected. Although, this is not a state
described by PAPR, this is an action we actually need to do
especially if the CPPR is already at 0xFF. Because in this case,
the resend bit will stay on until another ICP state change which
may be a long time coming and the interrupt stays pending until
then. The current code which checks for MFRR < CPPR is broken when
CPPR is 0xFF since it will not get triggered in that case.
Ideally, we would want to do a resend only if
prio(pending_interrupt) < mfrr && prio(pending_interrupt) < cppr
where pending interrupt is the one that was rejected. But we don't
have the priority of the pending interrupt state saved, so we
simply trigger a resend whenever the MFRR is made less favored.
3. In kvmppc_rm_h_ipi, where we save state to pass resends to the
virtual mode, we also need to save the ICP whose need_resend we
reset since this does not need to be my ICP (vcpu->arch.icp) as is
incorrectly assumed by the current code. A new field rm_resend_icp
is added to the kvmppc_icp structure for this purpose.
Signed-off-by: Suresh Warrier <warrier@linux.vnet.ibm.com>
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/kvm/book3s_hv_rm_xics.c | 36 ++++++++++++++++++++++++++++--------
arch/powerpc/kvm/book3s_xics.c | 30 +++++++++++++++++++++++-------
arch/powerpc/kvm/book3s_xics.h | 1 +
3 files changed, 52 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_rm_xics.c b/arch/powerpc/kvm/book3s_hv_rm_xics.c
index 3ee38e6..7b066f6 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_xics.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_xics.c
@@ -183,8 +183,10 @@ static void icp_rm_down_cppr(struct kvmppc_xics *xics, struct kvmppc_icp *icp,
* state update in HW (ie bus transactions) so we can handle them
* separately here as well.
*/
- if (resend)
+ if (resend) {
icp->rm_action |= XICS_RM_CHECK_RESEND;
+ icp->rm_resend_icp = icp;
+ }
}
@@ -254,10 +256,25 @@ int kvmppc_rm_h_ipi(struct kvm_vcpu *vcpu, unsigned long server,
* nothing needs to be done as there can be no XISR to
* reject.
*
+ * ICP state: Check_IPI
+ *
* If the CPPR is less favored, then we might be replacing
- * an interrupt, and thus need to possibly reject it as in
+ * an interrupt, and thus need to possibly reject it.
*
- * ICP state: Check_IPI
+ * ICP State: IPI
+ *
+ * Besides rejecting any pending interrupts, we also
+ * update XISR and pending_pri to mark IPI as pending.
+ *
+ * PAPR does not describe this state, but if the MFRR is being
+ * made less favored than its earlier value, there might be
+ * a previously-rejected interrupt needing to be resent.
+ * Ideally, we would want to resend only if
+ * prio(pending_interrupt) < mfrr &&
+ * prio(pending_interrupt) < cppr
+ * where pending interrupt is the one that was rejected. But
+ * we don't have that state, so we simply trigger a resend
+ * whenever the MFRR is made less favored.
*/
do {
old_state = new_state = ACCESS_ONCE(icp->state);
@@ -270,13 +287,14 @@ int kvmppc_rm_h_ipi(struct kvm_vcpu *vcpu, unsigned long server,
resend = false;
if (mfrr < new_state.cppr) {
/* Reject a pending interrupt if not an IPI */
- if (mfrr <= new_state.pending_pri)
+ if (mfrr <= new_state.pending_pri) {
reject = new_state.xisr;
- new_state.pending_pri = mfrr;
- new_state.xisr = XICS_IPI;
+ new_state.pending_pri = mfrr;
+ new_state.xisr = XICS_IPI;
+ }
}
- if (mfrr > old_state.mfrr && mfrr > new_state.cppr) {
+ if (mfrr > old_state.mfrr) {
resend = new_state.need_resend;
new_state.need_resend = 0;
}
@@ -289,8 +307,10 @@ int kvmppc_rm_h_ipi(struct kvm_vcpu *vcpu, unsigned long server,
}
/* Pass resends to virtual mode */
- if (resend)
+ if (resend) {
this_icp->rm_action |= XICS_RM_CHECK_RESEND;
+ this_icp->rm_resend_icp = icp;
+ }
return check_too_hard(xics, this_icp);
}
diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c
index eaeb780..807351f 100644
--- a/arch/powerpc/kvm/book3s_xics.c
+++ b/arch/powerpc/kvm/book3s_xics.c
@@ -613,10 +613,25 @@ static noinline int kvmppc_h_ipi(struct kvm_vcpu *vcpu, unsigned long server,
* there might be a previously-rejected interrupt needing
* to be resent.
*
+ * ICP state: Check_IPI
+ *
* If the CPPR is less favored, then we might be replacing
- * an interrupt, and thus need to possibly reject it as in
+ * an interrupt, and thus need to possibly reject it.
*
- * ICP state: Check_IPI
+ * ICP State: IPI
+ *
+ * Besides rejecting any pending interrupts, we also
+ * update XISR and pending_pri to mark IPI as pending.
+ *
+ * PAPR does not describe this state, but if the MFRR is being
+ * made less favored than its earlier value, there might be
+ * a previously-rejected interrupt needing to be resent.
+ * Ideally, we would want to resend only if
+ * prio(pending_interrupt) < mfrr &&
+ * prio(pending_interrupt) < cppr
+ * where pending interrupt is the one that was rejected. But
+ * we don't have that state, so we simply trigger a resend
+ * whenever the MFRR is made less favored.
*/
do {
old_state = new_state = ACCESS_ONCE(icp->state);
@@ -629,13 +644,14 @@ static noinline int kvmppc_h_ipi(struct kvm_vcpu *vcpu, unsigned long server,
resend = false;
if (mfrr < new_state.cppr) {
/* Reject a pending interrupt if not an IPI */
- if (mfrr <= new_state.pending_pri)
+ if (mfrr <= new_state.pending_pri) {
reject = new_state.xisr;
- new_state.pending_pri = mfrr;
- new_state.xisr = XICS_IPI;
+ new_state.pending_pri = mfrr;
+ new_state.xisr = XICS_IPI;
+ }
}
- if (mfrr > old_state.mfrr && mfrr > new_state.cppr) {
+ if (mfrr > old_state.mfrr) {
resend = new_state.need_resend;
new_state.need_resend = 0;
}
@@ -789,7 +805,7 @@ static noinline int kvmppc_xics_rm_complete(struct kvm_vcpu *vcpu, u32 hcall)
if (icp->rm_action & XICS_RM_KICK_VCPU)
kvmppc_fast_vcpu_kick(icp->rm_kick_target);
if (icp->rm_action & XICS_RM_CHECK_RESEND)
- icp_check_resend(xics, icp);
+ icp_check_resend(xics, icp->rm_resend_icp);
if (icp->rm_action & XICS_RM_REJECT)
icp_deliver_irq(xics, icp, icp->rm_reject);
if (icp->rm_action & XICS_RM_NOTIFY_EOI)
diff --git a/arch/powerpc/kvm/book3s_xics.h b/arch/powerpc/kvm/book3s_xics.h
index e8aaa7a..73f0f27 100644
--- a/arch/powerpc/kvm/book3s_xics.h
+++ b/arch/powerpc/kvm/book3s_xics.h
@@ -74,6 +74,7 @@ struct kvmppc_icp {
#define XICS_RM_NOTIFY_EOI 0x8
u32 rm_action;
struct kvm_vcpu *rm_kick_target;
+ struct kvmppc_icp *rm_resend_icp;
u32 rm_reject;
u32 rm_eoied_irq;
--
2.1.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 4/5] KVM: PPC: Book3S HV: Fix inaccuracies in ICP emulation for H_IPI
@ 2014-11-03 4:51 ` Paul Mackerras
0 siblings, 0 replies; 26+ messages in thread
From: Paul Mackerras @ 2014-11-03 4:51 UTC (permalink / raw)
To: Alexander Graf, kvm-ppc; +Cc: kvm, Suresh E. Warrier, Paul Mackerras
From: "Suresh E. Warrier" <warrier@linux.vnet.ibm.com>
This fixes some inaccuracies in the state machine for the virtualized
ICP when implementing the H_IPI hcall (Set_MFFR and related states):
1. The old code wipes out any pending interrupts when the new MFRR is
more favored than the CPPR but less favored than a pending
interrupt (by always modifying xisr and the pending_pri). This can
cause us to lose a pending external interrupt.
The correct code here is to only modify the pending_pri and xisr in
the ICP if the MFRR is equal to or more favored than the current
pending pri (since in this case, it is guaranteed that that there
cannot be a pending external interrupt). The code changes are
required in both kvmppc_rm_h_ipi and kvmppc_h_ipi.
2. Again, in both kvmppc_rm_h_ipi and kvmppc_h_ipi, there is a check
for whether MFRR is being made less favored AND further if new MFFR
is also less favored than the current CPPR, we check for any
resends pending in the ICP. These checks look like they are
designed to cover the case where if the MFRR is being made less
favored, we opportunistically trigger a resend of any interrupts
that had been previously rejected. Although, this is not a state
described by PAPR, this is an action we actually need to do
especially if the CPPR is already at 0xFF. Because in this case,
the resend bit will stay on until another ICP state change which
may be a long time coming and the interrupt stays pending until
then. The current code which checks for MFRR < CPPR is broken when
CPPR is 0xFF since it will not get triggered in that case.
Ideally, we would want to do a resend only if
prio(pending_interrupt) < mfrr && prio(pending_interrupt) < cppr
where pending interrupt is the one that was rejected. But we don't
have the priority of the pending interrupt state saved, so we
simply trigger a resend whenever the MFRR is made less favored.
3. In kvmppc_rm_h_ipi, where we save state to pass resends to the
virtual mode, we also need to save the ICP whose need_resend we
reset since this does not need to be my ICP (vcpu->arch.icp) as is
incorrectly assumed by the current code. A new field rm_resend_icp
is added to the kvmppc_icp structure for this purpose.
Signed-off-by: Suresh Warrier <warrier@linux.vnet.ibm.com>
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/kvm/book3s_hv_rm_xics.c | 36 ++++++++++++++++++++++++++++--------
arch/powerpc/kvm/book3s_xics.c | 30 +++++++++++++++++++++++-------
arch/powerpc/kvm/book3s_xics.h | 1 +
3 files changed, 52 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_rm_xics.c b/arch/powerpc/kvm/book3s_hv_rm_xics.c
index 3ee38e6..7b066f6 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_xics.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_xics.c
@@ -183,8 +183,10 @@ static void icp_rm_down_cppr(struct kvmppc_xics *xics, struct kvmppc_icp *icp,
* state update in HW (ie bus transactions) so we can handle them
* separately here as well.
*/
- if (resend)
+ if (resend) {
icp->rm_action |= XICS_RM_CHECK_RESEND;
+ icp->rm_resend_icp = icp;
+ }
}
@@ -254,10 +256,25 @@ int kvmppc_rm_h_ipi(struct kvm_vcpu *vcpu, unsigned long server,
* nothing needs to be done as there can be no XISR to
* reject.
*
+ * ICP state: Check_IPI
+ *
* If the CPPR is less favored, then we might be replacing
- * an interrupt, and thus need to possibly reject it as in
+ * an interrupt, and thus need to possibly reject it.
*
- * ICP state: Check_IPI
+ * ICP State: IPI
+ *
+ * Besides rejecting any pending interrupts, we also
+ * update XISR and pending_pri to mark IPI as pending.
+ *
+ * PAPR does not describe this state, but if the MFRR is being
+ * made less favored than its earlier value, there might be
+ * a previously-rejected interrupt needing to be resent.
+ * Ideally, we would want to resend only if
+ * prio(pending_interrupt) < mfrr &&
+ * prio(pending_interrupt) < cppr
+ * where pending interrupt is the one that was rejected. But
+ * we don't have that state, so we simply trigger a resend
+ * whenever the MFRR is made less favored.
*/
do {
old_state = new_state = ACCESS_ONCE(icp->state);
@@ -270,13 +287,14 @@ int kvmppc_rm_h_ipi(struct kvm_vcpu *vcpu, unsigned long server,
resend = false;
if (mfrr < new_state.cppr) {
/* Reject a pending interrupt if not an IPI */
- if (mfrr <= new_state.pending_pri)
+ if (mfrr <= new_state.pending_pri) {
reject = new_state.xisr;
- new_state.pending_pri = mfrr;
- new_state.xisr = XICS_IPI;
+ new_state.pending_pri = mfrr;
+ new_state.xisr = XICS_IPI;
+ }
}
- if (mfrr > old_state.mfrr && mfrr > new_state.cppr) {
+ if (mfrr > old_state.mfrr) {
resend = new_state.need_resend;
new_state.need_resend = 0;
}
@@ -289,8 +307,10 @@ int kvmppc_rm_h_ipi(struct kvm_vcpu *vcpu, unsigned long server,
}
/* Pass resends to virtual mode */
- if (resend)
+ if (resend) {
this_icp->rm_action |= XICS_RM_CHECK_RESEND;
+ this_icp->rm_resend_icp = icp;
+ }
return check_too_hard(xics, this_icp);
}
diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c
index eaeb780..807351f 100644
--- a/arch/powerpc/kvm/book3s_xics.c
+++ b/arch/powerpc/kvm/book3s_xics.c
@@ -613,10 +613,25 @@ static noinline int kvmppc_h_ipi(struct kvm_vcpu *vcpu, unsigned long server,
* there might be a previously-rejected interrupt needing
* to be resent.
*
+ * ICP state: Check_IPI
+ *
* If the CPPR is less favored, then we might be replacing
- * an interrupt, and thus need to possibly reject it as in
+ * an interrupt, and thus need to possibly reject it.
*
- * ICP state: Check_IPI
+ * ICP State: IPI
+ *
+ * Besides rejecting any pending interrupts, we also
+ * update XISR and pending_pri to mark IPI as pending.
+ *
+ * PAPR does not describe this state, but if the MFRR is being
+ * made less favored than its earlier value, there might be
+ * a previously-rejected interrupt needing to be resent.
+ * Ideally, we would want to resend only if
+ * prio(pending_interrupt) < mfrr &&
+ * prio(pending_interrupt) < cppr
+ * where pending interrupt is the one that was rejected. But
+ * we don't have that state, so we simply trigger a resend
+ * whenever the MFRR is made less favored.
*/
do {
old_state = new_state = ACCESS_ONCE(icp->state);
@@ -629,13 +644,14 @@ static noinline int kvmppc_h_ipi(struct kvm_vcpu *vcpu, unsigned long server,
resend = false;
if (mfrr < new_state.cppr) {
/* Reject a pending interrupt if not an IPI */
- if (mfrr <= new_state.pending_pri)
+ if (mfrr <= new_state.pending_pri) {
reject = new_state.xisr;
- new_state.pending_pri = mfrr;
- new_state.xisr = XICS_IPI;
+ new_state.pending_pri = mfrr;
+ new_state.xisr = XICS_IPI;
+ }
}
- if (mfrr > old_state.mfrr && mfrr > new_state.cppr) {
+ if (mfrr > old_state.mfrr) {
resend = new_state.need_resend;
new_state.need_resend = 0;
}
@@ -789,7 +805,7 @@ static noinline int kvmppc_xics_rm_complete(struct kvm_vcpu *vcpu, u32 hcall)
if (icp->rm_action & XICS_RM_KICK_VCPU)
kvmppc_fast_vcpu_kick(icp->rm_kick_target);
if (icp->rm_action & XICS_RM_CHECK_RESEND)
- icp_check_resend(xics, icp);
+ icp_check_resend(xics, icp->rm_resend_icp);
if (icp->rm_action & XICS_RM_REJECT)
icp_deliver_irq(xics, icp, icp->rm_reject);
if (icp->rm_action & XICS_RM_NOTIFY_EOI)
diff --git a/arch/powerpc/kvm/book3s_xics.h b/arch/powerpc/kvm/book3s_xics.h
index e8aaa7a..73f0f27 100644
--- a/arch/powerpc/kvm/book3s_xics.h
+++ b/arch/powerpc/kvm/book3s_xics.h
@@ -74,6 +74,7 @@ struct kvmppc_icp {
#define XICS_RM_NOTIFY_EOI 0x8
u32 rm_action;
struct kvm_vcpu *rm_kick_target;
+ struct kvmppc_icp *rm_resend_icp;
u32 rm_reject;
u32 rm_eoied_irq;
--
2.1.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 5/5] KVM: PPC: Book3S HV: Check wait conditions before sleeping in kvmppc_vcore_blocked
2014-11-03 4:51 ` Paul Mackerras
@ 2014-11-03 4:52 ` Paul Mackerras
-1 siblings, 0 replies; 26+ messages in thread
From: Paul Mackerras @ 2014-11-03 4:52 UTC (permalink / raw)
To: Alexander Graf, kvm-ppc; +Cc: kvm, Suresh E. Warrier, Paul Mackerras
From: "Suresh E. Warrier" <warrier@linux.vnet.ibm.com>
The kvmppc_vcore_blocked() code does not check for the wait condition
after putting the process on the wait queue. This means that it is
possible for an external interrupt to become pending, but the vcpu to
remain asleep until the next decrementer interrupt. The fix is to
make one last check for pending exceptions and ceded state before
calling schedule().
Signed-off-by: Suresh Warrier <warrier@linux.vnet.ibm.com>
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/kvm/book3s_hv.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index cd7e030..1a7a281 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1828,9 +1828,29 @@ static void kvmppc_wait_for_exec(struct kvm_vcpu *vcpu, int wait_state)
*/
static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
{
+ struct kvm_vcpu *vcpu;
+ int do_sleep = 1;
+
DEFINE_WAIT(wait);
prepare_to_wait(&vc->wq, &wait, TASK_INTERRUPTIBLE);
+
+ /*
+ * Check one last time for pending exceptions and ceded state after
+ * we put ourselves on the wait queue
+ */
+ list_for_each_entry(vcpu, &vc->runnable_threads, arch.run_list) {
+ if (vcpu->arch.pending_exceptions || !vcpu->arch.ceded) {
+ do_sleep = 0;
+ break;
+ }
+ }
+
+ if (!do_sleep) {
+ finish_wait(&vc->wq, &wait);
+ return;
+ }
+
vc->vcore_state = VCORE_SLEEPING;
spin_unlock(&vc->lock);
schedule();
--
2.1.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 5/5] KVM: PPC: Book3S HV: Check wait conditions before sleeping in kvmppc_vcore_blocked
@ 2014-11-03 4:52 ` Paul Mackerras
0 siblings, 0 replies; 26+ messages in thread
From: Paul Mackerras @ 2014-11-03 4:52 UTC (permalink / raw)
To: Alexander Graf, kvm-ppc; +Cc: kvm, Suresh E. Warrier, Paul Mackerras
From: "Suresh E. Warrier" <warrier@linux.vnet.ibm.com>
The kvmppc_vcore_blocked() code does not check for the wait condition
after putting the process on the wait queue. This means that it is
possible for an external interrupt to become pending, but the vcpu to
remain asleep until the next decrementer interrupt. The fix is to
make one last check for pending exceptions and ceded state before
calling schedule().
Signed-off-by: Suresh Warrier <warrier@linux.vnet.ibm.com>
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
arch/powerpc/kvm/book3s_hv.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index cd7e030..1a7a281 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1828,9 +1828,29 @@ static void kvmppc_wait_for_exec(struct kvm_vcpu *vcpu, int wait_state)
*/
static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
{
+ struct kvm_vcpu *vcpu;
+ int do_sleep = 1;
+
DEFINE_WAIT(wait);
prepare_to_wait(&vc->wq, &wait, TASK_INTERRUPTIBLE);
+
+ /*
+ * Check one last time for pending exceptions and ceded state after
+ * we put ourselves on the wait queue
+ */
+ list_for_each_entry(vcpu, &vc->runnable_threads, arch.run_list) {
+ if (vcpu->arch.pending_exceptions || !vcpu->arch.ceded) {
+ do_sleep = 0;
+ break;
+ }
+ }
+
+ if (!do_sleep) {
+ finish_wait(&vc->wq, &wait);
+ return;
+ }
+
vc->vcore_state = VCORE_SLEEPING;
spin_unlock(&vc->lock);
schedule();
--
2.1.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 5/5] KVM: PPC: Book3S HV: Check wait conditions before sleeping in kvmppc_vcore_blocked
2014-11-03 4:52 ` Paul Mackerras
@ 2014-11-20 17:36 ` Alexander Graf
-1 siblings, 0 replies; 26+ messages in thread
From: Alexander Graf @ 2014-11-20 17:36 UTC (permalink / raw)
To: Paul Mackerras, kvm-ppc; +Cc: kvm, Suresh E. Warrier
On 03.11.14 05:52, Paul Mackerras wrote:
> From: "Suresh E. Warrier" <warrier@linux.vnet.ibm.com>
>
> The kvmppc_vcore_blocked() code does not check for the wait condition
> after putting the process on the wait queue. This means that it is
> possible for an external interrupt to become pending, but the vcpu to
> remain asleep until the next decrementer interrupt. The fix is to
> make one last check for pending exceptions and ceded state before
> calling schedule().
>
> Signed-off-by: Suresh Warrier <warrier@linux.vnet.ibm.com>
> Signed-off-by: Paul Mackerras <paulus@samba.org>
I don't understand the race you're fixing here. Can you please explain it?
Alex
> ---
> arch/powerpc/kvm/book3s_hv.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index cd7e030..1a7a281 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1828,9 +1828,29 @@ static void kvmppc_wait_for_exec(struct kvm_vcpu *vcpu, int wait_state)
> */
> static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
> {
> + struct kvm_vcpu *vcpu;
> + int do_sleep = 1;
> +
> DEFINE_WAIT(wait);
>
> prepare_to_wait(&vc->wq, &wait, TASK_INTERRUPTIBLE);
> +
> + /*
> + * Check one last time for pending exceptions and ceded state after
> + * we put ourselves on the wait queue
> + */
> + list_for_each_entry(vcpu, &vc->runnable_threads, arch.run_list) {
> + if (vcpu->arch.pending_exceptions || !vcpu->arch.ceded) {
> + do_sleep = 0;
> + break;
> + }
> + }
> +
> + if (!do_sleep) {
> + finish_wait(&vc->wq, &wait);
> + return;
> + }
> +
> vc->vcore_state = VCORE_SLEEPING;
> spin_unlock(&vc->lock);
> schedule();
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 5/5] KVM: PPC: Book3S HV: Check wait conditions before sleeping in kvmppc_vcore_blocked
@ 2014-11-20 17:36 ` Alexander Graf
0 siblings, 0 replies; 26+ messages in thread
From: Alexander Graf @ 2014-11-20 17:36 UTC (permalink / raw)
To: Paul Mackerras, kvm-ppc; +Cc: kvm, Suresh E. Warrier
On 03.11.14 05:52, Paul Mackerras wrote:
> From: "Suresh E. Warrier" <warrier@linux.vnet.ibm.com>
>
> The kvmppc_vcore_blocked() code does not check for the wait condition
> after putting the process on the wait queue. This means that it is
> possible for an external interrupt to become pending, but the vcpu to
> remain asleep until the next decrementer interrupt. The fix is to
> make one last check for pending exceptions and ceded state before
> calling schedule().
>
> Signed-off-by: Suresh Warrier <warrier@linux.vnet.ibm.com>
> Signed-off-by: Paul Mackerras <paulus@samba.org>
I don't understand the race you're fixing here. Can you please explain it?
Alex
> ---
> arch/powerpc/kvm/book3s_hv.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index cd7e030..1a7a281 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1828,9 +1828,29 @@ static void kvmppc_wait_for_exec(struct kvm_vcpu *vcpu, int wait_state)
> */
> static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
> {
> + struct kvm_vcpu *vcpu;
> + int do_sleep = 1;
> +
> DEFINE_WAIT(wait);
>
> prepare_to_wait(&vc->wq, &wait, TASK_INTERRUPTIBLE);
> +
> + /*
> + * Check one last time for pending exceptions and ceded state after
> + * we put ourselves on the wait queue
> + */
> + list_for_each_entry(vcpu, &vc->runnable_threads, arch.run_list) {
> + if (vcpu->arch.pending_exceptions || !vcpu->arch.ceded) {
> + do_sleep = 0;
> + break;
> + }
> + }
> +
> + if (!do_sleep) {
> + finish_wait(&vc->wq, &wait);
> + return;
> + }
> +
> vc->vcore_state = VCORE_SLEEPING;
> spin_unlock(&vc->lock);
> schedule();
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 5/5] KVM: PPC: Book3S HV: Check wait conditions before sleeping in kvmppc_vcore_blocked
2014-11-20 17:36 ` Alexander Graf
@ 2014-11-20 19:31 ` Suresh E. Warrier
-1 siblings, 0 replies; 26+ messages in thread
From: Suresh E. Warrier @ 2014-11-20 19:31 UTC (permalink / raw)
To: Alexander Graf, Paul Mackerras, kvm-ppc; +Cc: kvm
On 11/20/2014 11:36 AM, Alexander Graf wrote:
>
>
> On 03.11.14 05:52, Paul Mackerras wrote:
>> From: "Suresh E. Warrier" <warrier@linux.vnet.ibm.com>
>>
>> The kvmppc_vcore_blocked() code does not check for the wait condition
>> after putting the process on the wait queue. This means that it is
>> possible for an external interrupt to become pending, but the vcpu to
>> remain asleep until the next decrementer interrupt. The fix is to
>> make one last check for pending exceptions and ceded state before
>> calling schedule().
>>
>> Signed-off-by: Suresh Warrier <warrier@linux.vnet.ibm.com>
>> Signed-off-by: Paul Mackerras <paulus@samba.org>
>
> I don't understand the race you're fixing here. Can you please explain it?
>
When a virtual interrupt needs to be delivered to the guest, and the
virtual ICS state for the interrupt and virtual ICP state for the VCPU
allow for the VCPU to be immediately interrupted, we
1. Set the BOOK3S_INTERRUPT_EXTERNAL_LEVEL bit in pending_exceptions.
2. Call kvmppc_fast_vcpu_kick_hv(), which checks the wait queue at vcpu->wq
to wake the VCPU up.
The caller of kvmppc_vcore_blocked() does the check for pending exceptions, but
there is a race condition here and we do need to check again after the VCPU
is put on the wait queue.
-suresh
>
> Alex
>
>> ---
>> arch/powerpc/kvm/book3s_hv.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index cd7e030..1a7a281 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -1828,9 +1828,29 @@ static void kvmppc_wait_for_exec(struct kvm_vcpu *vcpu, int wait_state)
>> */
>> static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
>> {
>> + struct kvm_vcpu *vcpu;
>> + int do_sleep = 1;
>> +
>> DEFINE_WAIT(wait);
>>
>> prepare_to_wait(&vc->wq, &wait, TASK_INTERRUPTIBLE);
>> +
>> + /*
>> + * Check one last time for pending exceptions and ceded state after
>> + * we put ourselves on the wait queue
>> + */
>> + list_for_each_entry(vcpu, &vc->runnable_threads, arch.run_list) {
>> + if (vcpu->arch.pending_exceptions || !vcpu->arch.ceded) {
>> + do_sleep = 0;
>> + break;
>> + }
>> + }
>> +
>> + if (!do_sleep) {
>> + finish_wait(&vc->wq, &wait);
>> + return;
>> + }
>> +
>> vc->vcore_state = VCORE_SLEEPING;
>> spin_unlock(&vc->lock);
>> schedule();
>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 5/5] KVM: PPC: Book3S HV: Check wait conditions before sleeping in kvmppc_vcore_blocked
@ 2014-11-20 19:31 ` Suresh E. Warrier
0 siblings, 0 replies; 26+ messages in thread
From: Suresh E. Warrier @ 2014-11-20 19:31 UTC (permalink / raw)
To: Alexander Graf, Paul Mackerras, kvm-ppc; +Cc: kvm
On 11/20/2014 11:36 AM, Alexander Graf wrote:
>
>
> On 03.11.14 05:52, Paul Mackerras wrote:
>> From: "Suresh E. Warrier" <warrier@linux.vnet.ibm.com>
>>
>> The kvmppc_vcore_blocked() code does not check for the wait condition
>> after putting the process on the wait queue. This means that it is
>> possible for an external interrupt to become pending, but the vcpu to
>> remain asleep until the next decrementer interrupt. The fix is to
>> make one last check for pending exceptions and ceded state before
>> calling schedule().
>>
>> Signed-off-by: Suresh Warrier <warrier@linux.vnet.ibm.com>
>> Signed-off-by: Paul Mackerras <paulus@samba.org>
>
> I don't understand the race you're fixing here. Can you please explain it?
>
When a virtual interrupt needs to be delivered to the guest, and the
virtual ICS state for the interrupt and virtual ICP state for the VCPU
allow for the VCPU to be immediately interrupted, we
1. Set the BOOK3S_INTERRUPT_EXTERNAL_LEVEL bit in pending_exceptions.
2. Call kvmppc_fast_vcpu_kick_hv(), which checks the wait queue at vcpu->wq
to wake the VCPU up.
The caller of kvmppc_vcore_blocked() does the check for pending exceptions, but
there is a race condition here and we do need to check again after the VCPU
is put on the wait queue.
-suresh
>
> Alex
>
>> ---
>> arch/powerpc/kvm/book3s_hv.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index cd7e030..1a7a281 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -1828,9 +1828,29 @@ static void kvmppc_wait_for_exec(struct kvm_vcpu *vcpu, int wait_state)
>> */
>> static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
>> {
>> + struct kvm_vcpu *vcpu;
>> + int do_sleep = 1;
>> +
>> DEFINE_WAIT(wait);
>>
>> prepare_to_wait(&vc->wq, &wait, TASK_INTERRUPTIBLE);
>> +
>> + /*
>> + * Check one last time for pending exceptions and ceded state after
>> + * we put ourselves on the wait queue
>> + */
>> + list_for_each_entry(vcpu, &vc->runnable_threads, arch.run_list) {
>> + if (vcpu->arch.pending_exceptions || !vcpu->arch.ceded) {
>> + do_sleep = 0;
>> + break;
>> + }
>> + }
>> +
>> + if (!do_sleep) {
>> + finish_wait(&vc->wq, &wait);
>> + return;
>> + }
>> +
>> vc->vcore_state = VCORE_SLEEPING;
>> spin_unlock(&vc->lock);
>> schedule();
>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 5/5] KVM: PPC: Book3S HV: Check wait conditions before sleeping in kvmppc_vcore_blocked
2014-11-20 19:31 ` Suresh E. Warrier
@ 2014-11-23 0:41 ` Alexander Graf
-1 siblings, 0 replies; 26+ messages in thread
From: Alexander Graf @ 2014-11-23 0:41 UTC (permalink / raw)
To: Suresh E. Warrier, Paul Mackerras, kvm-ppc; +Cc: kvm
On 20.11.14 20:31, Suresh E. Warrier wrote:
>
>
> On 11/20/2014 11:36 AM, Alexander Graf wrote:
>>
>>
>> On 03.11.14 05:52, Paul Mackerras wrote:
>>> From: "Suresh E. Warrier" <warrier@linux.vnet.ibm.com>
>>>
>>> The kvmppc_vcore_blocked() code does not check for the wait condition
>>> after putting the process on the wait queue. This means that it is
>>> possible for an external interrupt to become pending, but the vcpu to
>>> remain asleep until the next decrementer interrupt. The fix is to
>>> make one last check for pending exceptions and ceded state before
>>> calling schedule().
>>>
>>> Signed-off-by: Suresh Warrier <warrier@linux.vnet.ibm.com>
>>> Signed-off-by: Paul Mackerras <paulus@samba.org>
>>
>> I don't understand the race you're fixing here. Can you please explain it?
>>
>
> When a virtual interrupt needs to be delivered to the guest, and the
> virtual ICS state for the interrupt and virtual ICP state for the VCPU
> allow for the VCPU to be immediately interrupted, we
> 1. Set the BOOK3S_INTERRUPT_EXTERNAL_LEVEL bit in pending_exceptions.
> 2. Call kvmppc_fast_vcpu_kick_hv(), which checks the wait queue at vcpu->wq
> to wake the VCPU up.
>
> The caller of kvmppc_vcore_blocked() does the check for pending exceptions, but
> there is a race condition here and we do need to check again after the VCPU
> is put on the wait queue.
I see. Thanks, applied to kvm-ppc-queue.
Alex
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] KVM: PPC: Book3S HV: Check wait conditions before sleeping in kvmppc_vcore_blocked
@ 2014-11-23 0:41 ` Alexander Graf
0 siblings, 0 replies; 26+ messages in thread
From: Alexander Graf @ 2014-11-23 0:41 UTC (permalink / raw)
To: Suresh E. Warrier, Paul Mackerras, kvm-ppc; +Cc: kvm
On 20.11.14 20:31, Suresh E. Warrier wrote:
>
>
> On 11/20/2014 11:36 AM, Alexander Graf wrote:
>>
>>
>> On 03.11.14 05:52, Paul Mackerras wrote:
>>> From: "Suresh E. Warrier" <warrier@linux.vnet.ibm.com>
>>>
>>> The kvmppc_vcore_blocked() code does not check for the wait condition
>>> after putting the process on the wait queue. This means that it is
>>> possible for an external interrupt to become pending, but the vcpu to
>>> remain asleep until the next decrementer interrupt. The fix is to
>>> make one last check for pending exceptions and ceded state before
>>> calling schedule().
>>>
>>> Signed-off-by: Suresh Warrier <warrier@linux.vnet.ibm.com>
>>> Signed-off-by: Paul Mackerras <paulus@samba.org>
>>
>> I don't understand the race you're fixing here. Can you please explain it?
>>
>
> When a virtual interrupt needs to be delivered to the guest, and the
> virtual ICS state for the interrupt and virtual ICP state for the VCPU
> allow for the VCPU to be immediately interrupted, we
> 1. Set the BOOK3S_INTERRUPT_EXTERNAL_LEVEL bit in pending_exceptions.
> 2. Call kvmppc_fast_vcpu_kick_hv(), which checks the wait queue at vcpu->wq
> to wake the VCPU up.
>
> The caller of kvmppc_vcore_blocked() does the check for pending exceptions, but
> there is a race condition here and we do need to check again after the VCPU
> is put on the wait queue.
I see. Thanks, applied to kvm-ppc-queue.
Alex
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/5] Some fixes for HV KVM on PPC
2014-11-03 4:51 ` Paul Mackerras
@ 2014-11-20 17:36 ` Alexander Graf
-1 siblings, 0 replies; 26+ messages in thread
From: Alexander Graf @ 2014-11-20 17:36 UTC (permalink / raw)
To: Paul Mackerras, kvm-ppc; +Cc: kvm
On 03.11.14 05:51, Paul Mackerras wrote:
> Here are fixes for five bugs which were found in the testing of our
> PowerKVM product. The bugs range from guest performance issues to
> guest crashes and memory corruption. Please apply.
Thanks, applied patches 1-4 to kvm-ppc-queue.
Alex
^ permalink raw reply [flat|nested] 26+ messages in thread