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

[Nicholas Piggin <npiggin@gmail.com>]

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