[PATCH v3 0/2] KVM: PPC: Book3S HV: Nested guest HFSCR changes

[PATCH v3 0/2] KVM: PPC: Book3S HV: Nested guest HFSCR changes

[Fabiano Rosas]

Applied Nick's suggestions and added a new patch for the Cause bits
issue.

I'm thinking maybe the approach of crashing L1 when L2 tries to access
a facility that L0 has denied is too heavy-handed. But on the other
hand, if L1 were to access the facility itself, the same thing would
happen and L2 runs "inside of L1" in a sense.

Currently, both L0 and L1s handle only msgsndp. All other HV Facility
Unavailable causes are already met with a Program interrupt.

Changes since v2:

- removed the sanitise functions
- moved the entry code into a new load_l2_hv_regs and the exit code
  into the existing save_hv_return_state
- new patch: removes the cause bits when L0 has disabled the
  corresponding facility

v2:

- made the change more generic, not only applies to hfscr anymore;
- sanitisation is now done directly on the vcpu struct, l2_hv is left unchanged;

https://lkml.kernel.org/r/20210406214645.3315819-1-farosas@linux.ibm.com

v1:
https://lkml.kernel.org/r/20210305231055.2913892-1-farosas@linux.ibm.com

Fabiano Rosas (2):
  KVM: PPC: Book3S HV: Sanitise vcpu registers in nested path
  KVM: PPC: Book3S HV: Stop forwarding all HFSCR cause bits to L1

 arch/powerpc/kvm/book3s_hv_nested.c | 72 ++++++++++++++++++++---------
 1 file changed, 51 insertions(+), 21 deletions(-)

--
2.29.2

[PATCH v3 1/2] KVM: PPC: Book3S HV: Sanitise vcpu registers in nested path

[Fabiano Rosas]

As one of the arguments of the H_ENTER_NESTED hypercall, the nested
hypervisor (L1) prepares a structure containing the values of various
hypervisor-privileged registers with which it wants the nested guest
(L2) to run. Since the nested HV runs in supervisor mode it needs the
host to write to these registers.

To stop a nested HV manipulating this mechanism and using a nested
guest as a proxy to access a facility that has been made unavailable
to it, we have a routine that sanitises the values of the HV registers
before copying them into the nested guest's vcpu struct.

However, when coming out of the guest the values are copied as they
were back into L1 memory, which means that any sanitisation we did
during guest entry will be exposed to L1 after H_ENTER_NESTED returns.

This patch alters this sanitisation to have effect on the vcpu->arch
registers directly before entering and after exiting the guest,
leaving the structure that is copied back into L1 unchanged (except
when we really want L1 to access the value, e.g the Cause bits of
HFSCR).

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 arch/powerpc/kvm/book3s_hv_nested.c | 55 ++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
index 0cd0e7aad588..270552dd42c5 100644
--- a/arch/powerpc/kvm/book3s_hv_nested.c
+++ b/arch/powerpc/kvm/book3s_hv_nested.c
@@ -102,8 +102,17 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
 {
 	struct kvmppc_vcore *vc = vcpu->arch.vcore;
 
+	/*
+	 * When loading the hypervisor-privileged registers to run L2,
+	 * we might have used bits from L1 state to restrict what the
+	 * L2 state is allowed to be. Since L1 is not allowed to read
+	 * the HV registers, do not include these modifications in the
+	 * return state.
+	 */
+	hr->hfscr = ((~HFSCR_INTR_CAUSE & hr->hfscr) |
+		     (HFSCR_INTR_CAUSE & vcpu->arch.hfscr));
+
 	hr->dpdes = vc->dpdes;
-	hr->hfscr = vcpu->arch.hfscr;
 	hr->purr = vcpu->arch.purr;
 	hr->spurr = vcpu->arch.spurr;
 	hr->ic = vcpu->arch.ic;
@@ -132,24 +141,7 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
 	}
 }
 
-static void sanitise_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
-{
-	/*
-	 * Don't let L1 enable features for L2 which we've disabled for L1,
-	 * but preserve the interrupt cause field.
-	 */
-	hr->hfscr &= (HFSCR_INTR_CAUSE | vcpu->arch.hfscr);
-
-	/* Don't let data address watchpoint match in hypervisor state */
-	hr->dawrx0 &= ~DAWRX_HYP;
-	hr->dawrx1 &= ~DAWRX_HYP;
-
-	/* Don't let completed instruction address breakpt match in HV state */
-	if ((hr->ciabr & CIABR_PRIV) == CIABR_PRIV_HYPER)
-		hr->ciabr &= ~CIABR_PRIV;
-}
-
-static void restore_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
+static void restore_hv_regs(struct kvm_vcpu *vcpu, const struct hv_guest_state *hr)
 {
 	struct kvmppc_vcore *vc = vcpu->arch.vcore;
 
@@ -261,6 +253,27 @@ static int kvmhv_write_guest_state_and_regs(struct kvm_vcpu *vcpu,
 				     sizeof(struct pt_regs));
 }
 
+static void load_l2_hv_regs(struct kvm_vcpu *vcpu,
+			    const struct hv_guest_state *l2_hv,
+			    const struct hv_guest_state *l1_hv)
+{
+	restore_hv_regs(vcpu, l2_hv);
+
+	/*
+	 * Don't let L1 enable features for L2 which we've disabled for L1,
+	 * but preserve the interrupt cause field.
+	 */
+	vcpu->arch.hfscr = l2_hv->hfscr & (HFSCR_INTR_CAUSE | l1_hv->hfscr);
+
+	/* Don't let data address watchpoint match in hypervisor state */
+	vcpu->arch.dawrx0 = l2_hv->dawrx0 & ~DAWRX_HYP;
+	vcpu->arch.dawrx1 = l2_hv->dawrx1 & ~DAWRX_HYP;
+
+	/* Don't let completed instruction address breakpt match in HV state */
+	if ((l2_hv->ciabr & CIABR_PRIV) == CIABR_PRIV_HYPER)
+		vcpu->arch.ciabr = l2_hv->ciabr & ~CIABR_PRIV;
+}
+
 long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
 {
 	long int err, r;
@@ -324,8 +337,8 @@ long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
 	mask = LPCR_DPFD | LPCR_ILE | LPCR_TC | LPCR_AIL | LPCR_LD |
 		LPCR_LPES | LPCR_MER;
 	lpcr = (vc->lpcr & ~mask) | (l2_hv.lpcr & mask);
-	sanitise_hv_regs(vcpu, &l2_hv);
-	restore_hv_regs(vcpu, &l2_hv);
+
+	load_l2_hv_regs(vcpu, &l2_hv, &saved_l1_hv);
 
 	vcpu->arch.ret = RESUME_GUEST;
 	vcpu->arch.trap = 0;
-- 
2.29.2

Re: [PATCH v3 1/2] KVM: PPC: Book3S HV: Sanitise vcpu registers in nested path

[Nicholas Piggin]

Excerpts from Fabiano Rosas's message of April 16, 2021 9:09 am:
> As one of the arguments of the H_ENTER_NESTED hypercall, the nested
> hypervisor (L1) prepares a structure containing the values of various
> hypervisor-privileged registers with which it wants the nested guest
> (L2) to run. Since the nested HV runs in supervisor mode it needs the
> host to write to these registers.
> 
> To stop a nested HV manipulating this mechanism and using a nested
> guest as a proxy to access a facility that has been made unavailable
> to it, we have a routine that sanitises the values of the HV registers
> before copying them into the nested guest's vcpu struct.
> 
> However, when coming out of the guest the values are copied as they
> were back into L1 memory, which means that any sanitisation we did
> during guest entry will be exposed to L1 after H_ENTER_NESTED returns.
> 
> This patch alters this sanitisation to have effect on the vcpu->arch
> registers directly before entering and after exiting the guest,
> leaving the structure that is copied back into L1 unchanged (except
> when we really want L1 to access the value, e.g the Cause bits of
> HFSCR).
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  arch/powerpc/kvm/book3s_hv_nested.c | 55 ++++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
> index 0cd0e7aad588..270552dd42c5 100644
> --- a/arch/powerpc/kvm/book3s_hv_nested.c
> +++ b/arch/powerpc/kvm/book3s_hv_nested.c
> @@ -102,8 +102,17 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
>  {
>  	struct kvmppc_vcore *vc = vcpu->arch.vcore;
>  
> +	/*
> +	 * When loading the hypervisor-privileged registers to run L2,
> +	 * we might have used bits from L1 state to restrict what the
> +	 * L2 state is allowed to be. Since L1 is not allowed to read
> +	 * the HV registers, do not include these modifications in the
> +	 * return state.
> +	 */
> +	hr->hfscr = ((~HFSCR_INTR_CAUSE & hr->hfscr) |
> +		     (HFSCR_INTR_CAUSE & vcpu->arch.hfscr));
> +
>  	hr->dpdes = vc->dpdes;
> -	hr->hfscr = vcpu->arch.hfscr;
>  	hr->purr = vcpu->arch.purr;
>  	hr->spurr = vcpu->arch.spurr;
>  	hr->ic = vcpu->arch.ic;

Do we still have the problem here that hfac interrupts due to bits cleared
by the hfscr sanitisation would have the cause bits returned to the L1,
so in theory it could probe hfscr directly that way? I don't see a good
solution to this except either have the L0 intercept these faults and do
"something" transparent, or return error from H_ENTER_NESTED (which would
also allow trivial probing of the facilities).

Returning an hfac interrupt to a hypervisor that thought it enabled the 
bit would be strange. But so does appearing to modify the register 
underneath it and then returning a fault.

I think the sanest thing would actually be to return failure from the 
hcall.

> @@ -132,24 +141,7 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
>  	}
>  }
>  
> -static void sanitise_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
> -{
> -	/*
> -	 * Don't let L1 enable features for L2 which we've disabled for L1,
> -	 * but preserve the interrupt cause field.
> -	 */
> -	hr->hfscr &= (HFSCR_INTR_CAUSE | vcpu->arch.hfscr);
> -
> -	/* Don't let data address watchpoint match in hypervisor state */
> -	hr->dawrx0 &= ~DAWRX_HYP;
> -	hr->dawrx1 &= ~DAWRX_HYP;
> -
> -	/* Don't let completed instruction address breakpt match in HV state */
> -	if ((hr->ciabr & CIABR_PRIV) == CIABR_PRIV_HYPER)
> -		hr->ciabr &= ~CIABR_PRIV;
> -}
> -
> -static void restore_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
> +static void restore_hv_regs(struct kvm_vcpu *vcpu, const struct hv_guest_state *hr)
>  {
>  	struct kvmppc_vcore *vc = vcpu->arch.vcore;
>  
> @@ -261,6 +253,27 @@ static int kvmhv_write_guest_state_and_regs(struct kvm_vcpu *vcpu,
>  				     sizeof(struct pt_regs));
>  }
>  
> +static void load_l2_hv_regs(struct kvm_vcpu *vcpu,
> +			    const struct hv_guest_state *l2_hv,
> +			    const struct hv_guest_state *l1_hv)
> +{
> +	restore_hv_regs(vcpu, l2_hv);
> +
> +	/*
> +	 * Don't let L1 enable features for L2 which we've disabled for L1,
> +	 * but preserve the interrupt cause field.
> +	 */
> +	vcpu->arch.hfscr = l2_hv->hfscr & (HFSCR_INTR_CAUSE | l1_hv->hfscr);
> +
> +	/* Don't let data address watchpoint match in hypervisor state */
> +	vcpu->arch.dawrx0 = l2_hv->dawrx0 & ~DAWRX_HYP;
> +	vcpu->arch.dawrx1 = l2_hv->dawrx1 & ~DAWRX_HYP;
> +
> +	/* Don't let completed instruction address breakpt match in HV state */
> +	if ((l2_hv->ciabr & CIABR_PRIV) == CIABR_PRIV_HYPER)
> +		vcpu->arch.ciabr = l2_hv->ciabr & ~CIABR_PRIV;
> +}
> +
>  long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
>  {
>  	long int err, r;
> @@ -324,8 +337,8 @@ long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
>  	mask = LPCR_DPFD | LPCR_ILE | LPCR_TC | LPCR_AIL | LPCR_LD |
>  		LPCR_LPES | LPCR_MER;
>  	lpcr = (vc->lpcr & ~mask) | (l2_hv.lpcr & mask);
> -	sanitise_hv_regs(vcpu, &l2_hv);
> -	restore_hv_regs(vcpu, &l2_hv);
> +
> +	load_l2_hv_regs(vcpu, &l2_hv, &saved_l1_hv);
>  
>  	vcpu->arch.ret = RESUME_GUEST;
>  	vcpu->arch.trap = 0;
> -- 
> 2.29.2
> 
> 

Re: [PATCH v3 1/2] KVM: PPC: Book3S HV: Sanitise vcpu registers in nested path

[Paul Mackerras]

On Sat, May 01, 2021 at 11:58:36AM +1000, Nicholas Piggin wrote:
> Excerpts from Fabiano Rosas's message of April 16, 2021 9:09 am:
> > As one of the arguments of the H_ENTER_NESTED hypercall, the nested
> > hypervisor (L1) prepares a structure containing the values of various
> > hypervisor-privileged registers with which it wants the nested guest
> > (L2) to run. Since the nested HV runs in supervisor mode it needs the
> > host to write to these registers.
> > 
> > To stop a nested HV manipulating this mechanism and using a nested
> > guest as a proxy to access a facility that has been made unavailable
> > to it, we have a routine that sanitises the values of the HV registers
> > before copying them into the nested guest's vcpu struct.
> > 
> > However, when coming out of the guest the values are copied as they
> > were back into L1 memory, which means that any sanitisation we did
> > during guest entry will be exposed to L1 after H_ENTER_NESTED returns.
> > 
> > This patch alters this sanitisation to have effect on the vcpu->arch
> > registers directly before entering and after exiting the guest,
> > leaving the structure that is copied back into L1 unchanged (except
> > when we really want L1 to access the value, e.g the Cause bits of
> > HFSCR).
> > 
> > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> > ---
> >  arch/powerpc/kvm/book3s_hv_nested.c | 55 ++++++++++++++++++-----------
> >  1 file changed, 34 insertions(+), 21 deletions(-)
> > 
> > diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
> > index 0cd0e7aad588..270552dd42c5 100644
> > --- a/arch/powerpc/kvm/book3s_hv_nested.c
> > +++ b/arch/powerpc/kvm/book3s_hv_nested.c
> > @@ -102,8 +102,17 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
> >  {
> >  	struct kvmppc_vcore *vc = vcpu->arch.vcore;
> >  
> > +	/*
> > +	 * When loading the hypervisor-privileged registers to run L2,
> > +	 * we might have used bits from L1 state to restrict what the
> > +	 * L2 state is allowed to be. Since L1 is not allowed to read
> > +	 * the HV registers, do not include these modifications in the
> > +	 * return state.
> > +	 */
> > +	hr->hfscr = ((~HFSCR_INTR_CAUSE & hr->hfscr) |
> > +		     (HFSCR_INTR_CAUSE & vcpu->arch.hfscr));
> > +
> >  	hr->dpdes = vc->dpdes;
> > -	hr->hfscr = vcpu->arch.hfscr;
> >  	hr->purr = vcpu->arch.purr;
> >  	hr->spurr = vcpu->arch.spurr;
> >  	hr->ic = vcpu->arch.ic;
> 
> Do we still have the problem here that hfac interrupts due to bits cleared
> by the hfscr sanitisation would have the cause bits returned to the L1,
> so in theory it could probe hfscr directly that way? I don't see a good
> solution to this except either have the L0 intercept these faults and do
> "something" transparent, or return error from H_ENTER_NESTED (which would
> also allow trivial probing of the facilities).

It seems to me that there are various specific reasons why L0 would
clear HFSCR bits, and if we think about the specific reasons, what we
should do becomes clear.  (I say "L0" but in fact the same reasoning
applies to any hypervisor that lets its guest do hypervisor-ish
things.)

1. Emulating a version of the architecture which doesn't have the
feature in question - in that case the bit should appear to L1 as a
reserved bit in HFSCR (i.e. always read 0), the associated facility
code should never appear in the top 8 bits of any HFSCR value that L1
sees, and any HFU interrupt received by L0 for the facility should be
changed into an illegal instruction interrupt (or HEAI) forwarded to
L1.  In this case the real HFSCR should always have the enable bit for
the facility set to 0.

2. Lazy save/restore of the state associated with a facility - in this
case, while the system is in the "lazy" state (i.e. the state is not
that of the currently running guest), the real HFSCR bit for the
facility should be 0.  On an HFU interrupt for the facility, L0 looks
at L1's HFSCR value: if it's 0, forward the HFU interrupt to L1; if
it's 1, load up the facility state, set the facility's bit in HFSCR,
and resume the guest.

3. Emulating a facility in software - in this case, the real HFSCR
bit for the facility would always be 0.  On an HFU interrupt, L0 reads
the instruction and emulates it, then resumes the guest.

One thing this all makes clear is that the IC field of the "virtual"
HFSCR value seen by L1 should only ever be changed when L0 forwards a
HFU interrupt to L1.

In fact we currently never do (1) or (2), and we only do (3) for
msgsndp etc., so this discussion is mostly theoretical.

> Returning an hfac interrupt to a hypervisor that thought it enabled the 
> bit would be strange. But so does appearing to modify the register 
> underneath it and then returning a fault.

I don't think we should ever do either of those things.  The closest
would be (1) above, but in that case the fault has to be either an
illegal instruction type program interrupt, or a HEAI.

> I think the sanest thing would actually be to return failure from the 
> hcall.

I don't think we should do that either.

Paul.

Re: [PATCH v3 1/2] KVM: PPC: Book3S HV: Sanitise vcpu registers in nested path

[Nicholas Piggin]

Excerpts from Paul Mackerras's message of May 4, 2021 2:28 pm:
> On Sat, May 01, 2021 at 11:58:36AM +1000, Nicholas Piggin wrote:
>> Excerpts from Fabiano Rosas's message of April 16, 2021 9:09 am:
>> > As one of the arguments of the H_ENTER_NESTED hypercall, the nested
>> > hypervisor (L1) prepares a structure containing the values of various
>> > hypervisor-privileged registers with which it wants the nested guest
>> > (L2) to run. Since the nested HV runs in supervisor mode it needs the
>> > host to write to these registers.
>> > 
>> > To stop a nested HV manipulating this mechanism and using a nested
>> > guest as a proxy to access a facility that has been made unavailable
>> > to it, we have a routine that sanitises the values of the HV registers
>> > before copying them into the nested guest's vcpu struct.
>> > 
>> > However, when coming out of the guest the values are copied as they
>> > were back into L1 memory, which means that any sanitisation we did
>> > during guest entry will be exposed to L1 after H_ENTER_NESTED returns.
>> > 
>> > This patch alters this sanitisation to have effect on the vcpu->arch
>> > registers directly before entering and after exiting the guest,
>> > leaving the structure that is copied back into L1 unchanged (except
>> > when we really want L1 to access the value, e.g the Cause bits of
>> > HFSCR).
>> > 
>> > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> > ---
>> >  arch/powerpc/kvm/book3s_hv_nested.c | 55 ++++++++++++++++++-----------
>> >  1 file changed, 34 insertions(+), 21 deletions(-)
>> > 
>> > diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
>> > index 0cd0e7aad588..270552dd42c5 100644
>> > --- a/arch/powerpc/kvm/book3s_hv_nested.c
>> > +++ b/arch/powerpc/kvm/book3s_hv_nested.c
>> > @@ -102,8 +102,17 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
>> >  {
>> >  	struct kvmppc_vcore *vc = vcpu->arch.vcore;
>> >  
>> > +	/*
>> > +	 * When loading the hypervisor-privileged registers to run L2,
>> > +	 * we might have used bits from L1 state to restrict what the
>> > +	 * L2 state is allowed to be. Since L1 is not allowed to read
>> > +	 * the HV registers, do not include these modifications in the
>> > +	 * return state.
>> > +	 */
>> > +	hr->hfscr = ((~HFSCR_INTR_CAUSE & hr->hfscr) |
>> > +		     (HFSCR_INTR_CAUSE & vcpu->arch.hfscr));
>> > +
>> >  	hr->dpdes = vc->dpdes;
>> > -	hr->hfscr = vcpu->arch.hfscr;
>> >  	hr->purr = vcpu->arch.purr;
>> >  	hr->spurr = vcpu->arch.spurr;
>> >  	hr->ic = vcpu->arch.ic;
>> 
>> Do we still have the problem here that hfac interrupts due to bits cleared
>> by the hfscr sanitisation would have the cause bits returned to the L1,
>> so in theory it could probe hfscr directly that way? I don't see a good
>> solution to this except either have the L0 intercept these faults and do
>> "something" transparent, or return error from H_ENTER_NESTED (which would
>> also allow trivial probing of the facilities).
> 
> It seems to me that there are various specific reasons why L0 would
> clear HFSCR bits, and if we think about the specific reasons, what we
> should do becomes clear.  (I say "L0" but in fact the same reasoning
> applies to any hypervisor that lets its guest do hypervisor-ish
> things.)
> 
> 1. Emulating a version of the architecture which doesn't have the
> feature in question - in that case the bit should appear to L1 as a
> reserved bit in HFSCR (i.e. always read 0), the associated facility
> code should never appear in the top 8 bits of any HFSCR value that L1
> sees, and any HFU interrupt received by L0 for the facility should be
> changed into an illegal instruction interrupt (or HEAI) forwarded to
> L1.  In this case the real HFSCR should always have the enable bit for
> the facility set to 0.
> 
> 2. Lazy save/restore of the state associated with a facility - in this
> case, while the system is in the "lazy" state (i.e. the state is not
> that of the currently running guest), the real HFSCR bit for the
> facility should be 0.  On an HFU interrupt for the facility, L0 looks
> at L1's HFSCR value: if it's 0, forward the HFU interrupt to L1; if
> it's 1, load up the facility state, set the facility's bit in HFSCR,
> and resume the guest.
> 
> 3. Emulating a facility in software - in this case, the real HFSCR
> bit for the facility would always be 0.  On an HFU interrupt, L0 reads
> the instruction and emulates it, then resumes the guest.
> 
> One thing this all makes clear is that the IC field of the "virtual"
> HFSCR value seen by L1 should only ever be changed when L0 forwards a
> HFU interrupt to L1.
> 
> In fact we currently never do (1) or (2), and we only do (3) for
> msgsndp etc., so this discussion is mostly theoretical.

Yeah it's somewhat theoretical, and I guess I mostly agree with you.

Missing is the case where the L0 does not implement a feature at all.
Let's say TM is broken so it disables it, or nobody uses TAR so it 
doesn't bother to switch it.

In those cases what do you tell the L1 if it enables a bit that you
don't support at all, and it takes a fault?

I guess the right thing to do is advertise that to the guest by some
other means, and expect it does the right thing. And you could have
the proviso in the nested HV specification that the returned IC field
might trip for a feature you enabled in the L1 HFSCR.

> 
>> Returning an hfac interrupt to a hypervisor that thought it enabled the 
>> bit would be strange. But so does appearing to modify the register 
>> underneath it and then returning a fault.
> 
> I don't think we should ever do either of those things.  The closest
> would be (1) above, but in that case the fault has to be either an
> illegal instruction type program interrupt, or a HEAI.
> 
>> I think the sanest thing would actually be to return failure from the 
>> hcall.
> 
> I don't think we should do that either.

I still think it's preferable for case 4. No point waiting for the
guest to boot and some user program eventually hits a bad instruction,
even if it was due to some host vs guest configuration problem.

At any rate, this patch 1 not overwriting the L2 HV state with the
sanitization step is fine and clearly required for any kind of non
trivial handling of missing bits.

Thanks,
Nick

Re: [PATCH v3 1/2] KVM: PPC: Book3S HV: Sanitise vcpu registers in nested path

[Paul Mackerras]

On Tue, May 04, 2021 at 03:26:24PM +1000, Nicholas Piggin wrote:
> Excerpts from Paul Mackerras's message of May 4, 2021 2:28 pm:
> > On Sat, May 01, 2021 at 11:58:36AM +1000, Nicholas Piggin wrote:
> >> Excerpts from Fabiano Rosas's message of April 16, 2021 9:09 am:
> >> > As one of the arguments of the H_ENTER_NESTED hypercall, the nested
> >> > hypervisor (L1) prepares a structure containing the values of various
> >> > hypervisor-privileged registers with which it wants the nested guest
> >> > (L2) to run. Since the nested HV runs in supervisor mode it needs the
> >> > host to write to these registers.
> >> > 
> >> > To stop a nested HV manipulating this mechanism and using a nested
> >> > guest as a proxy to access a facility that has been made unavailable
> >> > to it, we have a routine that sanitises the values of the HV registers
> >> > before copying them into the nested guest's vcpu struct.
> >> > 
> >> > However, when coming out of the guest the values are copied as they
> >> > were back into L1 memory, which means that any sanitisation we did
> >> > during guest entry will be exposed to L1 after H_ENTER_NESTED returns.
> >> > 
> >> > This patch alters this sanitisation to have effect on the vcpu->arch
> >> > registers directly before entering and after exiting the guest,
> >> > leaving the structure that is copied back into L1 unchanged (except
> >> > when we really want L1 to access the value, e.g the Cause bits of
> >> > HFSCR).
> >> > 
> >> > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> >> > ---
> >> >  arch/powerpc/kvm/book3s_hv_nested.c | 55 ++++++++++++++++++-----------
> >> >  1 file changed, 34 insertions(+), 21 deletions(-)
> >> > 
> >> > diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
> >> > index 0cd0e7aad588..270552dd42c5 100644
> >> > --- a/arch/powerpc/kvm/book3s_hv_nested.c
> >> > +++ b/arch/powerpc/kvm/book3s_hv_nested.c
> >> > @@ -102,8 +102,17 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
> >> >  {
> >> >  	struct kvmppc_vcore *vc = vcpu->arch.vcore;
> >> >  
> >> > +	/*
> >> > +	 * When loading the hypervisor-privileged registers to run L2,
> >> > +	 * we might have used bits from L1 state to restrict what the
> >> > +	 * L2 state is allowed to be. Since L1 is not allowed to read
> >> > +	 * the HV registers, do not include these modifications in the
> >> > +	 * return state.
> >> > +	 */
> >> > +	hr->hfscr = ((~HFSCR_INTR_CAUSE & hr->hfscr) |
> >> > +		     (HFSCR_INTR_CAUSE & vcpu->arch.hfscr));
> >> > +
> >> >  	hr->dpdes = vc->dpdes;
> >> > -	hr->hfscr = vcpu->arch.hfscr;
> >> >  	hr->purr = vcpu->arch.purr;
> >> >  	hr->spurr = vcpu->arch.spurr;
> >> >  	hr->ic = vcpu->arch.ic;
> >> 
> >> Do we still have the problem here that hfac interrupts due to bits cleared
> >> by the hfscr sanitisation would have the cause bits returned to the L1,
> >> so in theory it could probe hfscr directly that way? I don't see a good
> >> solution to this except either have the L0 intercept these faults and do
> >> "something" transparent, or return error from H_ENTER_NESTED (which would
> >> also allow trivial probing of the facilities).
> > 
> > It seems to me that there are various specific reasons why L0 would
> > clear HFSCR bits, and if we think about the specific reasons, what we
> > should do becomes clear.  (I say "L0" but in fact the same reasoning
> > applies to any hypervisor that lets its guest do hypervisor-ish
> > things.)
> > 
> > 1. Emulating a version of the architecture which doesn't have the
> > feature in question - in that case the bit should appear to L1 as a
> > reserved bit in HFSCR (i.e. always read 0), the associated facility
> > code should never appear in the top 8 bits of any HFSCR value that L1
> > sees, and any HFU interrupt received by L0 for the facility should be
> > changed into an illegal instruction interrupt (or HEAI) forwarded to
> > L1.  In this case the real HFSCR should always have the enable bit for
> > the facility set to 0.
> > 
> > 2. Lazy save/restore of the state associated with a facility - in this
> > case, while the system is in the "lazy" state (i.e. the state is not
> > that of the currently running guest), the real HFSCR bit for the
> > facility should be 0.  On an HFU interrupt for the facility, L0 looks
> > at L1's HFSCR value: if it's 0, forward the HFU interrupt to L1; if
> > it's 1, load up the facility state, set the facility's bit in HFSCR,
> > and resume the guest.
> > 
> > 3. Emulating a facility in software - in this case, the real HFSCR
> > bit for the facility would always be 0.  On an HFU interrupt, L0 reads
> > the instruction and emulates it, then resumes the guest.
> > 
> > One thing this all makes clear is that the IC field of the "virtual"
> > HFSCR value seen by L1 should only ever be changed when L0 forwards a
> > HFU interrupt to L1.
> > 
> > In fact we currently never do (1) or (2), and we only do (3) for
> > msgsndp etc., so this discussion is mostly theoretical.
> 
> Yeah it's somewhat theoretical, and I guess I mostly agree with you.
> 
> Missing is the case where the L0 does not implement a feature at all.
> Let's say TM is broken so it disables it, or nobody uses TAR so it 
> doesn't bother to switch it.

I think that's the same as my case (1), where L0 is presenting an
architecture to L1 that doesn't implement the feature.  (Unless you
mean that L0 is running on a machine that has features that didn't
exist at the time that L0's code was written; to handle that, L0
should clear all HFSCR bits that it doesn't know about, and map any
unknown HFSCR[IC] code into illegal interrupt/HEAI.)

> In those cases what do you tell the L1 if it enables a bit that you
> don't support at all, and it takes a fault?

Illegal instruction interrupt, or HEAI.

> I guess the right thing to do is advertise that to the guest by some
> other means, and expect it does the right thing. And you could have
> the proviso in the nested HV specification that the returned IC field
> might trip for a feature you enabled in the L1 HFSCR.

I think that is confusing; better to return illegal instruction/HEAI.

> > 
> >> Returning an hfac interrupt to a hypervisor that thought it enabled the 
> >> bit would be strange. But so does appearing to modify the register 
> >> underneath it and then returning a fault.
> > 
> > I don't think we should ever do either of those things.  The closest
> > would be (1) above, but in that case the fault has to be either an
> > illegal instruction type program interrupt, or a HEAI.
> > 
> >> I think the sanest thing would actually be to return failure from the 
> >> hcall.
> > 
> > I don't think we should do that either.
> 
> I still think it's preferable for case 4. No point waiting for the
> guest to boot and some user program eventually hits a bad instruction,
> even if it was due to some host vs guest configuration problem.

If you let it boot to userspace, the administrator has a better chance
of recovering the situation.

Paul.

Re: [PATCH v3 1/2] KVM: PPC: Book3S HV: Sanitise vcpu registers in nested path

[Nicholas Piggin]

Excerpts from Paul Mackerras's message of May 4, 2021 5:36 pm:
> On Tue, May 04, 2021 at 03:26:24PM +1000, Nicholas Piggin wrote:
>> Excerpts from Paul Mackerras's message of May 4, 2021 2:28 pm:
>> > On Sat, May 01, 2021 at 11:58:36AM +1000, Nicholas Piggin wrote:
>> >> Excerpts from Fabiano Rosas's message of April 16, 2021 9:09 am:
>> >> > As one of the arguments of the H_ENTER_NESTED hypercall, the nested
>> >> > hypervisor (L1) prepares a structure containing the values of various
>> >> > hypervisor-privileged registers with which it wants the nested guest
>> >> > (L2) to run. Since the nested HV runs in supervisor mode it needs the
>> >> > host to write to these registers.
>> >> > 
>> >> > To stop a nested HV manipulating this mechanism and using a nested
>> >> > guest as a proxy to access a facility that has been made unavailable
>> >> > to it, we have a routine that sanitises the values of the HV registers
>> >> > before copying them into the nested guest's vcpu struct.
>> >> > 
>> >> > However, when coming out of the guest the values are copied as they
>> >> > were back into L1 memory, which means that any sanitisation we did
>> >> > during guest entry will be exposed to L1 after H_ENTER_NESTED returns.
>> >> > 
>> >> > This patch alters this sanitisation to have effect on the vcpu->arch
>> >> > registers directly before entering and after exiting the guest,
>> >> > leaving the structure that is copied back into L1 unchanged (except
>> >> > when we really want L1 to access the value, e.g the Cause bits of
>> >> > HFSCR).
>> >> > 
>> >> > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
>> >> > ---
>> >> >  arch/powerpc/kvm/book3s_hv_nested.c | 55 ++++++++++++++++++-----------
>> >> >  1 file changed, 34 insertions(+), 21 deletions(-)
>> >> > 
>> >> > diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
>> >> > index 0cd0e7aad588..270552dd42c5 100644
>> >> > --- a/arch/powerpc/kvm/book3s_hv_nested.c
>> >> > +++ b/arch/powerpc/kvm/book3s_hv_nested.c
>> >> > @@ -102,8 +102,17 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
>> >> >  {
>> >> >  	struct kvmppc_vcore *vc = vcpu->arch.vcore;
>> >> >  
>> >> > +	/*
>> >> > +	 * When loading the hypervisor-privileged registers to run L2,
>> >> > +	 * we might have used bits from L1 state to restrict what the
>> >> > +	 * L2 state is allowed to be. Since L1 is not allowed to read
>> >> > +	 * the HV registers, do not include these modifications in the
>> >> > +	 * return state.
>> >> > +	 */
>> >> > +	hr->hfscr = ((~HFSCR_INTR_CAUSE & hr->hfscr) |
>> >> > +		     (HFSCR_INTR_CAUSE & vcpu->arch.hfscr));
>> >> > +
>> >> >  	hr->dpdes = vc->dpdes;
>> >> > -	hr->hfscr = vcpu->arch.hfscr;
>> >> >  	hr->purr = vcpu->arch.purr;
>> >> >  	hr->spurr = vcpu->arch.spurr;
>> >> >  	hr->ic = vcpu->arch.ic;
>> >> 
>> >> Do we still have the problem here that hfac interrupts due to bits cleared
>> >> by the hfscr sanitisation would have the cause bits returned to the L1,
>> >> so in theory it could probe hfscr directly that way? I don't see a good
>> >> solution to this except either have the L0 intercept these faults and do
>> >> "something" transparent, or return error from H_ENTER_NESTED (which would
>> >> also allow trivial probing of the facilities).
>> > 
>> > It seems to me that there are various specific reasons why L0 would
>> > clear HFSCR bits, and if we think about the specific reasons, what we
>> > should do becomes clear.  (I say "L0" but in fact the same reasoning
>> > applies to any hypervisor that lets its guest do hypervisor-ish
>> > things.)
>> > 
>> > 1. Emulating a version of the architecture which doesn't have the
>> > feature in question - in that case the bit should appear to L1 as a
>> > reserved bit in HFSCR (i.e. always read 0), the associated facility
>> > code should never appear in the top 8 bits of any HFSCR value that L1
>> > sees, and any HFU interrupt received by L0 for the facility should be
>> > changed into an illegal instruction interrupt (or HEAI) forwarded to
>> > L1.  In this case the real HFSCR should always have the enable bit for
>> > the facility set to 0.
>> > 
>> > 2. Lazy save/restore of the state associated with a facility - in this
>> > case, while the system is in the "lazy" state (i.e. the state is not
>> > that of the currently running guest), the real HFSCR bit for the
>> > facility should be 0.  On an HFU interrupt for the facility, L0 looks
>> > at L1's HFSCR value: if it's 0, forward the HFU interrupt to L1; if
>> > it's 1, load up the facility state, set the facility's bit in HFSCR,
>> > and resume the guest.
>> > 
>> > 3. Emulating a facility in software - in this case, the real HFSCR
>> > bit for the facility would always be 0.  On an HFU interrupt, L0 reads
>> > the instruction and emulates it, then resumes the guest.
>> > 
>> > One thing this all makes clear is that the IC field of the "virtual"
>> > HFSCR value seen by L1 should only ever be changed when L0 forwards a
>> > HFU interrupt to L1.
>> > 
>> > In fact we currently never do (1) or (2), and we only do (3) for
>> > msgsndp etc., so this discussion is mostly theoretical.
>> 
>> Yeah it's somewhat theoretical, and I guess I mostly agree with you.
>> 
>> Missing is the case where the L0 does not implement a feature at all.
>> Let's say TM is broken so it disables it, or nobody uses TAR so it 
>> doesn't bother to switch it.
> 
> I think that's the same as my case (1), where L0 is presenting an
> architecture to L1 that doesn't implement the feature.  (Unless you
> mean that L0 is running on a machine that has features that didn't
> exist at the time that L0's code was written; to handle that, L0
> should clear all HFSCR bits that it doesn't know about, and map any
> unknown HFSCR[IC] code into illegal interrupt/HEAI.)

I don't think it's quite either of those. There is no ISA v3.0 where the
target address register facility does not exist, and the L0 is not going
to emulate it, for argument's sake.

>> In those cases what do you tell the L1 if it enables a bit that you
>> don't support at all, and it takes a fault?
> 
> Illegal instruction interrupt, or HEAI.

Okay, so that's just a roundabout way to tell the nested HV "this 
feature doesn't exist", asynchronously at some point when its guest
tries to use the facility. It's contrary to the architecture it
expected (maybe wrongly because it didn't query the right capability)
but it set HFSCR for the facility so it thought it should work.

>> I guess the right thing to do is advertise that to the guest by some
>> other means, and expect it does the right thing. And you could have
>> the proviso in the nested HV specification that the returned IC field
>> might trip for a feature you enabled in the L1 HFSCR.
> 
> I think that is confusing; better to return illegal instruction/HEAI.

I think both are more confusing than returning error from the hcall.

> 
>> > 
>> >> Returning an hfac interrupt to a hypervisor that thought it enabled the 
>> >> bit would be strange. But so does appearing to modify the register 
>> >> underneath it and then returning a fault.
>> > 
>> > I don't think we should ever do either of those things.  The closest
>> > would be (1) above, but in that case the fault has to be either an
>> > illegal instruction type program interrupt, or a HEAI.
>> > 
>> >> I think the sanest thing would actually be to return failure from the 
>> >> hcall.
>> > 
>> > I don't think we should do that either.
>> 
>> I still think it's preferable for case 4. No point waiting for the
>> guest to boot and some user program eventually hits a bad instruction,
>> even if it was due to some host vs guest configuration problem.
> 
> If you let it boot to userspace, the administrator has a better chance
> of recovering the situation.

Maybe. What about if it runs userspace until something executes
pthread_mutex_lock

An error message when you try to start the nested guest telling you
pass -machine cap-htm=off would be better... I guess that should
really all work with caps etc today though so TM's a bad example.
But assume we don't have a cap for the bit we disable? Maybe we
should have caps for all HFSCR bits, or I'm just worried about
something not very important.

Thanks,
Nick

Re: [PATCH v3 1/2] KVM: PPC: Book3S HV: Sanitise vcpu registers in nested path

[Fabiano Rosas]

Nicholas Piggin <npiggin@gmail.com> writes:

> An error message when you try to start the nested guest telling you
> pass -machine cap-htm=off would be better... I guess that should
> really all work with caps etc today though so TM's a bad example.
> But assume we don't have a cap for the bit we disable? Maybe we
> should have caps for all HFSCR bits, or I'm just worried about
> something not very important.

I'm avoiding returning an error from H_ENTER_NESTED at first run
specifically because of this. I think it interferes with L1 migration.

Say we have an L1 that has an workload that involves nested guests. It
can boot and run them just fine (i.e. it uses the same HFSCR value for
its guests as L0). If we migrate that L1 into a host that uses different
HFSCR bits and therefore will always fail the H_ENTER_NESTED, that is
effectively the same as migrating into a host that does not provide
KVM_CAP_PPC_NESTED_HV.

We would need some way to inform the migration code that the remote host
will not allow L1 to run nested guests with that particular HFSCR value
so that it can decide whether that host is a suitable migration
target. Otherwise we're migrating the guest into a host that will not
allow its operation to continue.

Returning an error later on during the nested guest lifetime I think it
is less harmful, but nothing really went wrong with the hypercall. It
did its job and ran L2 like L1 asked, although it ignored some bits. Can
we say that L1 misconfigured L2 when there is no way for L1 to negotiate
which bits it can use? The same set of bits could be considered valid by
another L0. It seems that as long as we hardcode some bits we shouldn't
fail the hcall because of them.

I think since this is all a bit theoretical right now, forwarding a
program interrupt into L1 is a good less permanent solution for the
moment, it does not alter the hcall's API and if we start stumbling into
similar issues in the future we'll have more information then to come up
with a proper solution.

>
> Thanks,
> Nick

Re: [PATCH v3 1/2] KVM: PPC: Book3S HV: Sanitise vcpu registers in nested path

[Nicholas Piggin]

Excerpts from Fabiano Rosas's message of April 16, 2021 9:09 am:
> As one of the arguments of the H_ENTER_NESTED hypercall, the nested
> hypervisor (L1) prepares a structure containing the values of various
> hypervisor-privileged registers with which it wants the nested guest
> (L2) to run. Since the nested HV runs in supervisor mode it needs the
> host to write to these registers.
> 
> To stop a nested HV manipulating this mechanism and using a nested
> guest as a proxy to access a facility that has been made unavailable
> to it, we have a routine that sanitises the values of the HV registers
> before copying them into the nested guest's vcpu struct.
> 
> However, when coming out of the guest the values are copied as they
> were back into L1 memory, which means that any sanitisation we did
> during guest entry will be exposed to L1 after H_ENTER_NESTED returns.
> 
> This patch alters this sanitisation to have effect on the vcpu->arch
> registers directly before entering and after exiting the guest,
> leaving the structure that is copied back into L1 unchanged (except
> when we really want L1 to access the value, e.g the Cause bits of
> HFSCR).
> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  arch/powerpc/kvm/book3s_hv_nested.c | 55 ++++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
> index 0cd0e7aad588..270552dd42c5 100644
> --- a/arch/powerpc/kvm/book3s_hv_nested.c
> +++ b/arch/powerpc/kvm/book3s_hv_nested.c
> @@ -102,8 +102,17 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
>  {
>  	struct kvmppc_vcore *vc = vcpu->arch.vcore;
>  
> +	/*
> +	 * When loading the hypervisor-privileged registers to run L2,
> +	 * we might have used bits from L1 state to restrict what the
> +	 * L2 state is allowed to be. Since L1 is not allowed to read
> +	 * the HV registers, do not include these modifications in the
> +	 * return state.
> +	 */
> +	hr->hfscr = ((~HFSCR_INTR_CAUSE & hr->hfscr) |
> +		     (HFSCR_INTR_CAUSE & vcpu->arch.hfscr));
> +
>  	hr->dpdes = vc->dpdes;
> -	hr->hfscr = vcpu->arch.hfscr;
>  	hr->purr = vcpu->arch.purr;
>  	hr->spurr = vcpu->arch.spurr;
>  	hr->ic = vcpu->arch.ic;

The below parts of the patch I have no problem with, I think it's good to 
be able to restore the hv_guest_state for return, e.g., for cases where 
the L0 might emulate some HV behaviour transparently it will be useful,
at least.

Thanks,
Nick

> @@ -132,24 +141,7 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
>  	}
>  }
>  
> -static void sanitise_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
> -{
> -	/*
> -	 * Don't let L1 enable features for L2 which we've disabled for L1,
> -	 * but preserve the interrupt cause field.
> -	 */
> -	hr->hfscr &= (HFSCR_INTR_CAUSE | vcpu->arch.hfscr);
> -
> -	/* Don't let data address watchpoint match in hypervisor state */
> -	hr->dawrx0 &= ~DAWRX_HYP;
> -	hr->dawrx1 &= ~DAWRX_HYP;
> -
> -	/* Don't let completed instruction address breakpt match in HV state */
> -	if ((hr->ciabr & CIABR_PRIV) == CIABR_PRIV_HYPER)
> -		hr->ciabr &= ~CIABR_PRIV;
> -}
> -
> -static void restore_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
> +static void restore_hv_regs(struct kvm_vcpu *vcpu, const struct hv_guest_state *hr)
>  {
>  	struct kvmppc_vcore *vc = vcpu->arch.vcore;
>  
> @@ -261,6 +253,27 @@ static int kvmhv_write_guest_state_and_regs(struct kvm_vcpu *vcpu,
>  				     sizeof(struct pt_regs));
>  }
>  
> +static void load_l2_hv_regs(struct kvm_vcpu *vcpu,
> +			    const struct hv_guest_state *l2_hv,
> +			    const struct hv_guest_state *l1_hv)
> +{
> +	restore_hv_regs(vcpu, l2_hv);
> +
> +	/*
> +	 * Don't let L1 enable features for L2 which we've disabled for L1,
> +	 * but preserve the interrupt cause field.
> +	 */
> +	vcpu->arch.hfscr = l2_hv->hfscr & (HFSCR_INTR_CAUSE | l1_hv->hfscr);
> +
> +	/* Don't let data address watchpoint match in hypervisor state */
> +	vcpu->arch.dawrx0 = l2_hv->dawrx0 & ~DAWRX_HYP;
> +	vcpu->arch.dawrx1 = l2_hv->dawrx1 & ~DAWRX_HYP;
> +
> +	/* Don't let completed instruction address breakpt match in HV state */
> +	if ((l2_hv->ciabr & CIABR_PRIV) == CIABR_PRIV_HYPER)
> +		vcpu->arch.ciabr = l2_hv->ciabr & ~CIABR_PRIV;
> +}
> +
>  long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
>  {
>  	long int err, r;
> @@ -324,8 +337,8 @@ long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
>  	mask = LPCR_DPFD | LPCR_ILE | LPCR_TC | LPCR_AIL | LPCR_LD |
>  		LPCR_LPES | LPCR_MER;
>  	lpcr = (vc->lpcr & ~mask) | (l2_hv.lpcr & mask);
> -	sanitise_hv_regs(vcpu, &l2_hv);
> -	restore_hv_regs(vcpu, &l2_hv);
> +
> +	load_l2_hv_regs(vcpu, &l2_hv, &saved_l1_hv);
>  
>  	vcpu->arch.ret = RESUME_GUEST;
>  	vcpu->arch.trap = 0;
> -- 
> 2.29.2
> 
> 

[PATCH v3 2/2] KVM: PPC: Book3S HV: Stop forwarding all HFSCR cause bits to L1

[Fabiano Rosas]

Since commit 73937deb4b2d ("KVM: PPC: Book3S HV: Sanitise hv_regs on
nested guest entry") we have been disabling for the nested guest the
hypervisor facility bits that its nested hypervisor don't have access
to.

If the nested guest tries to use one of those facilities, the hardware
will cause a Hypervisor Facility Unavailable interrupt. The HFSCR
register is modified by the hardware to contain information about the
cause of the interrupt.

We have been returning the cause bits to the nested hypervisor but
since commit 549e29b458c5 ("KVM: PPC: Book3S HV: Sanitise vcpu
registers in nested path") we are reducing the amount of information
exposed to L1, so it seems like a good idea to restrict some of the
cause bits as well.

With this patch the L1 guest will be allowed to handle only the
interrupts caused by facilities it has disabled for L2. The interrupts
caused by facilities that L0 denied will cause a Program Interrupt in
L1.

Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
 arch/powerpc/kvm/book3s_hv_nested.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
index 270552dd42c5..912a2bcdf7b0 100644
--- a/arch/powerpc/kvm/book3s_hv_nested.c
+++ b/arch/powerpc/kvm/book3s_hv_nested.c
@@ -138,6 +138,23 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
 	case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
 		hr->heir = vcpu->arch.emul_inst;
 		break;
+	case BOOK3S_INTERRUPT_H_FAC_UNAVAIL:
+	{
+		u8 cause = vcpu->arch.hfscr >> 56;
+
+		WARN_ON_ONCE(cause >= BITS_PER_LONG);
+
+		if (hr->hfscr & (1UL << cause)) {
+			hr->hfscr &= ~HFSCR_INTR_CAUSE;
+			/*
+			 * We have not restored L1 state yet, so queue
+			 * this interrupt instead of delivering it
+			 * immediately.
+			 */
+			kvmppc_book3s_queue_irqprio(vcpu, BOOK3S_INTERRUPT_PROGRAM);
+		}
+		break;
+	}
 	}
 }
 
-- 
2.29.2

Re: [PATCH v3 2/2] KVM: PPC: Book3S HV: Stop forwarding all HFSCR cause bits to L1

[Nicholas Piggin]

Oh sorry, I didn't skim this one before replying to the first.

Excerpts from Fabiano Rosas's message of April 16, 2021 9:09 am:
> Since commit 73937deb4b2d ("KVM: PPC: Book3S HV: Sanitise hv_regs on
> nested guest entry") we have been disabling for the nested guest the
> hypervisor facility bits that its nested hypervisor don't have access
> to.
> 
> If the nested guest tries to use one of those facilities, the hardware
> will cause a Hypervisor Facility Unavailable interrupt. The HFSCR
> register is modified by the hardware to contain information about the
> cause of the interrupt.
> 
> We have been returning the cause bits to the nested hypervisor but
> since commit 549e29b458c5 ("KVM: PPC: Book3S HV: Sanitise vcpu
> registers in nested path") we are reducing the amount of information
> exposed to L1, so it seems like a good idea to restrict some of the
> cause bits as well.
> 
> With this patch the L1 guest will be allowed to handle only the
> interrupts caused by facilities it has disabled for L2. The interrupts
> caused by facilities that L0 denied will cause a Program Interrupt in
> L1.

I'm not sure if this is a good solution. This would be randomly killing 
guest processes or kernels with no way for them to understand what's going
on or deal with it.

The problem is really a nested hypervisor mismatch / configuration 
error, so it should be handled between the L0 and L1. Returning failure
from H_ENTER_NESTED, for example (which is probe-able, but not really 
any less probe-able than this approach).

Thanks,
Nick

> 
> Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
> ---
>  arch/powerpc/kvm/book3s_hv_nested.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
> index 270552dd42c5..912a2bcdf7b0 100644
> --- a/arch/powerpc/kvm/book3s_hv_nested.c
> +++ b/arch/powerpc/kvm/book3s_hv_nested.c
> @@ -138,6 +138,23 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
>  	case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
>  		hr->heir = vcpu->arch.emul_inst;
>  		break;
> +	case BOOK3S_INTERRUPT_H_FAC_UNAVAIL:
> +	{
> +		u8 cause = vcpu->arch.hfscr >> 56;
> +
> +		WARN_ON_ONCE(cause >= BITS_PER_LONG);
> +
> +		if (hr->hfscr & (1UL << cause)) {
> +			hr->hfscr &= ~HFSCR_INTR_CAUSE;
> +			/*
> +			 * We have not restored L1 state yet, so queue
> +			 * this interrupt instead of delivering it
> +			 * immediately.
> +			 */
> +			kvmppc_book3s_queue_irqprio(vcpu, BOOK3S_INTERRUPT_PROGRAM);
> +		}
> +		break;
> +	}
>  	}
>  }
>  
> -- 
> 2.29.2
> 
>