[PATCH] KVM: PPC: Book3S HV: Don't lose hardware R/C bit updates in H_PROTECT

[PATCH] KVM: PPC: Book3S HV: Don't lose hardware R/C bit updates in H_PROTECT

[Paul Mackerras]

The hashed page table MMU in POWER processors can update the R
(reference) and C (change) bits in a HPTE at any time until the
HPTE has been invalidated and the TLB invalidation sequence has
completed.  In kvmppc_h_protect, which implements the H_PROTECT
hypercall, we read the HPTE, modify the second doubleword,
invalidate the HPTE in memory, do the TLB invalidation sequence,
and then write the modified value of the second doubleword back
to memory.  In doing so we could overwrite an R/C bit update done
by hardware between when we read the HPTE and when the TLB
invalidation completed.  To fix this we re-read the second
doubleword after the TLB invalidation and OR in the (possibly)
new values of R and C.  We can use an OR since hardware only ever
sets R and C, never clears them.

This race was found by code inspection.  In principle this bug could
cause occasional guest memory corruption under host memory pressure.

Fixes: a8606e20e41a ("KVM: PPC: Handle some PAPR hcalls in the kernel", 2011-06-29)
Cc: stable@vger.kernel.org # v3.19+
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
---
 arch/powerpc/kvm/book3s_hv_rm_mmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 752451f3..02786b3 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -670,6 +670,8 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, unsigned long flags,
 					      HPTE_V_ABSENT);
 			do_tlbies(kvm, &rb, 1, global_invalidates(kvm, flags),
 				  true);
+			/* Don't lose R/C bit updates done by hardware */
+			r |= be64_to_cpu(hpte[1]) & (HPTE_R_R | HPTE_R_C);
 			hpte[1] = cpu_to_be64(r);
 		}
 	}
-- 
2.10.2

[PATCH] KVM: PPC: Book3S HV: Don't lose hardware R/C bit updates in H_PROTECT

[Paul Mackerras]

The hashed page table MMU in POWER processors can update the R
(reference) and C (change) bits in a HPTE at any time until the
HPTE has been invalidated and the TLB invalidation sequence has
completed.  In kvmppc_h_protect, which implements the H_PROTECT
hypercall, we read the HPTE, modify the second doubleword,
invalidate the HPTE in memory, do the TLB invalidation sequence,
and then write the modified value of the second doubleword back
to memory.  In doing so we could overwrite an R/C bit update done
by hardware between when we read the HPTE and when the TLB
invalidation completed.  To fix this we re-read the second
doubleword after the TLB invalidation and OR in the (possibly)
new values of R and C.  We can use an OR since hardware only ever
sets R and C, never clears them.

This race was found by code inspection.  In principle this bug could
cause occasional guest memory corruption under host memory pressure.

Fixes: a8606e20e41a ("KVM: PPC: Handle some PAPR hcalls in the kernel", 2011-06-29)
Cc: stable@vger.kernel.org # v3.19+
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
---
 arch/powerpc/kvm/book3s_hv_rm_mmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 752451f3..02786b3 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -670,6 +670,8 @@ long kvmppc_h_protect(struct kvm_vcpu *vcpu, unsigned long flags,
 					      HPTE_V_ABSENT);
 			do_tlbies(kvm, &rb, 1, global_invalidates(kvm, flags),
 				  true);
+			/* Don't lose R/C bit updates done by hardware */
+			r |= be64_to_cpu(hpte[1]) & (HPTE_R_R | HPTE_R_C);
 			hpte[1] = cpu_to_be64(r);
 		}
 	}
-- 
2.10.2

Re: [PATCH] KVM: PPC: Book3S HV: Don't lose hardware R/C bit updates in H_PROTECT

[Paul Mackerras]

On Wed, Nov 16, 2016 at 04:43:28PM +1100, Paul Mackerras wrote:
> The hashed page table MMU in POWER processors can update the R
> (reference) and C (change) bits in a HPTE at any time until the
> HPTE has been invalidated and the TLB invalidation sequence has
> completed.  In kvmppc_h_protect, which implements the H_PROTECT
> hypercall, we read the HPTE, modify the second doubleword,
> invalidate the HPTE in memory, do the TLB invalidation sequence,
> and then write the modified value of the second doubleword back
> to memory.  In doing so we could overwrite an R/C bit update done
> by hardware between when we read the HPTE and when the TLB
> invalidation completed.  To fix this we re-read the second
> doubleword after the TLB invalidation and OR in the (possibly)
> new values of R and C.  We can use an OR since hardware only ever
> sets R and C, never clears them.
> 
> This race was found by code inspection.  In principle this bug could
> cause occasional guest memory corruption under host memory pressure.
> 
> Fixes: a8606e20e41a ("KVM: PPC: Handle some PAPR hcalls in the kernel", 2011-06-29)
> Cc: stable@vger.kernel.org # v3.19+
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>

Applied to kvm-ppc-next.

Paul.

Re: [PATCH] KVM: PPC: Book3S HV: Don't lose hardware R/C bit updates in H_PROTECT

[Paul Mackerras]

On Wed, Nov 16, 2016 at 04:43:28PM +1100, Paul Mackerras wrote:
> The hashed page table MMU in POWER processors can update the R
> (reference) and C (change) bits in a HPTE at any time until the
> HPTE has been invalidated and the TLB invalidation sequence has
> completed.  In kvmppc_h_protect, which implements the H_PROTECT
> hypercall, we read the HPTE, modify the second doubleword,
> invalidate the HPTE in memory, do the TLB invalidation sequence,
> and then write the modified value of the second doubleword back
> to memory.  In doing so we could overwrite an R/C bit update done
> by hardware between when we read the HPTE and when the TLB
> invalidation completed.  To fix this we re-read the second
> doubleword after the TLB invalidation and OR in the (possibly)
> new values of R and C.  We can use an OR since hardware only ever
> sets R and C, never clears them.
> 
> This race was found by code inspection.  In principle this bug could
> cause occasional guest memory corruption under host memory pressure.
> 
> Fixes: a8606e20e41a ("KVM: PPC: Handle some PAPR hcalls in the kernel", 2011-06-29)
> Cc: stable@vger.kernel.org # v3.19+
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>

Applied to kvm-ppc-next.

Paul.

[PATCH] KVM: PPC: Book3S HV: Don't lose pending doorbell request on migration on P9

[Paul Mackerras]

On POWER9, when userspace reads the value of the DPDES register on a
vCPU, it is possible for 0 to be returned although there is a doorbell
interrupt pending for the vCPU.  This can lead to a doorbell interrupt
being lost across migration.  If the guest kernel uses doorbell
interrupts for IPIs, then it could malfunction because of the lost
interrupt.

This happens because a newly-generated doorbell interrupt is signalled
by setting vcpu->arch.doorbell_request to 1; the DPDES value in
vcpu->arch.vcore->dpdes is not updated, because it can only be updated
when holding the vcpu mutex, in order to avoid races.

To fix this, we OR in vcpu->arch.doorbell_request when reading the
DPDES value.

Cc: stable@vger.kernel.org # v4.13+
Fixes: 579006944e0d ("KVM: PPC: Book3S HV: Virtualize doorbell facility on POWER9")
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
---
 arch/powerpc/kvm/book3s_hv.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index ca6c6ec..88c42e7 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1678,7 +1678,14 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
 		*val = get_reg_val(id, vcpu->arch.pspb);
 		break;
 	case KVM_REG_PPC_DPDES:
-		*val = get_reg_val(id, vcpu->arch.vcore->dpdes);
+		/*
+		 * On POWER9, where we are emulating msgsndp etc.,
+		 * we return 1 bit for each vcpu, which can come from
+		 * either vcore->dpdes or doorbell_request.
+		 * On POWER8, doorbell_request is 0.
+		 */
+		*val = get_reg_val(id, vcpu->arch.vcore->dpdes |
+				   vcpu->arch.doorbell_request);
 		break;
 	case KVM_REG_PPC_VTB:
 		*val = get_reg_val(id, vcpu->arch.vcore->vtb);
-- 
2.7.4

[PATCH] KVM: PPC: Book3S HV: Don't lose pending doorbell request on migration on P9

[Paul Mackerras]

On POWER9, when userspace reads the value of the DPDES register on a
vCPU, it is possible for 0 to be returned although there is a doorbell
interrupt pending for the vCPU.  This can lead to a doorbell interrupt
being lost across migration.  If the guest kernel uses doorbell
interrupts for IPIs, then it could malfunction because of the lost
interrupt.

This happens because a newly-generated doorbell interrupt is signalled
by setting vcpu->arch.doorbell_request to 1; the DPDES value in
vcpu->arch.vcore->dpdes is not updated, because it can only be updated
when holding the vcpu mutex, in order to avoid races.

To fix this, we OR in vcpu->arch.doorbell_request when reading the
DPDES value.

Cc: stable@vger.kernel.org # v4.13+
Fixes: 579006944e0d ("KVM: PPC: Book3S HV: Virtualize doorbell facility on POWER9")
Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
---
 arch/powerpc/kvm/book3s_hv.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index ca6c6ec..88c42e7 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1678,7 +1678,14 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
 		*val = get_reg_val(id, vcpu->arch.pspb);
 		break;
 	case KVM_REG_PPC_DPDES:
-		*val = get_reg_val(id, vcpu->arch.vcore->dpdes);
+		/*
+		 * On POWER9, where we are emulating msgsndp etc.,
+		 * we return 1 bit for each vcpu, which can come from
+		 * either vcore->dpdes or doorbell_request.
+		 * On POWER8, doorbell_request is 0.
+		 */
+		*val = get_reg_val(id, vcpu->arch.vcore->dpdes |
+				   vcpu->arch.doorbell_request);
 		break;
 	case KVM_REG_PPC_VTB:
 		*val = get_reg_val(id, vcpu->arch.vcore->vtb);
-- 
2.7.4

Re: [PATCH] KVM: PPC: Book3S HV: Don't lose pending doorbell request on migration on P9

[Alexey Kardashevskiy]

On 27/08/2019 11:35, Paul Mackerras wrote:
> On POWER9, when userspace reads the value of the DPDES register on a
> vCPU, it is possible for 0 to be returned although there is a doorbell
> interrupt pending for the vCPU.  This can lead to a doorbell interrupt
> being lost across migration.  If the guest kernel uses doorbell
> interrupts for IPIs, then it could malfunction because of the lost
> interrupt.
> 
> This happens because a newly-generated doorbell interrupt is signalled
> by setting vcpu->arch.doorbell_request to 1; the DPDES value in
> vcpu->arch.vcore->dpdes is not updated, because it can only be updated
> when holding the vcpu mutex, in order to avoid races.
> 
> To fix this, we OR in vcpu->arch.doorbell_request when reading the
> DPDES value.
> 
> Cc: stable@vger.kernel.org # v4.13+
> Fixes: 579006944e0d ("KVM: PPC: Book3S HV: Virtualize doorbell facility on POWER9")
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>



Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru>


> ---
>   arch/powerpc/kvm/book3s_hv.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index ca6c6ec..88c42e7 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1678,7 +1678,14 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
>   		*val = get_reg_val(id, vcpu->arch.pspb);
>   		break;
>   	case KVM_REG_PPC_DPDES:
> -		*val = get_reg_val(id, vcpu->arch.vcore->dpdes);
> +		/*
> +		 * On POWER9, where we are emulating msgsndp etc.,
> +		 * we return 1 bit for each vcpu, which can come from
> +		 * either vcore->dpdes or doorbell_request.
> +		 * On POWER8, doorbell_request is 0.
> +		 */
> +		*val = get_reg_val(id, vcpu->arch.vcore->dpdes |
> +				   vcpu->arch.doorbell_request);
>   		break;
>   	case KVM_REG_PPC_VTB:
>   		*val = get_reg_val(id, vcpu->arch.vcore->vtb);
> 

-- 
Alexey

Re: [PATCH] KVM: PPC: Book3S HV: Don't lose pending doorbell request on migration on P9

[Alexey Kardashevskiy]

On 27/08/2019 11:35, Paul Mackerras wrote:
> On POWER9, when userspace reads the value of the DPDES register on a
> vCPU, it is possible for 0 to be returned although there is a doorbell
> interrupt pending for the vCPU.  This can lead to a doorbell interrupt
> being lost across migration.  If the guest kernel uses doorbell
> interrupts for IPIs, then it could malfunction because of the lost
> interrupt.
> 
> This happens because a newly-generated doorbell interrupt is signalled
> by setting vcpu->arch.doorbell_request to 1; the DPDES value in
> vcpu->arch.vcore->dpdes is not updated, because it can only be updated
> when holding the vcpu mutex, in order to avoid races.
> 
> To fix this, we OR in vcpu->arch.doorbell_request when reading the
> DPDES value.
> 
> Cc: stable@vger.kernel.org # v4.13+
> Fixes: 579006944e0d ("KVM: PPC: Book3S HV: Virtualize doorbell facility on POWER9")
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>



Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru>


> ---
>   arch/powerpc/kvm/book3s_hv.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index ca6c6ec..88c42e7 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1678,7 +1678,14 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
>   		*val = get_reg_val(id, vcpu->arch.pspb);
>   		break;
>   	case KVM_REG_PPC_DPDES:
> -		*val = get_reg_val(id, vcpu->arch.vcore->dpdes);
> +		/*
> +		 * On POWER9, where we are emulating msgsndp etc.,
> +		 * we return 1 bit for each vcpu, which can come from
> +		 * either vcore->dpdes or doorbell_request.
> +		 * On POWER8, doorbell_request is 0.
> +		 */
> +		*val = get_reg_val(id, vcpu->arch.vcore->dpdes |
> +				   vcpu->arch.doorbell_request);
>   		break;
>   	case KVM_REG_PPC_VTB:
>   		*val = get_reg_val(id, vcpu->arch.vcore->vtb);
> 

-- 
Alexey