Re: [PATCH v3 05/11] s390/mm/fault: Handle guest-related program interrupts in KVM

[Claudio Imbrenda]

On Wed, 16 Oct 2024 12:05:14 +0200
Heiko Carstens <hca@xxxxxxxxxxxxx> wrote:

> On Tue, Oct 15, 2024 at 06:43:20PM +0200, Claudio Imbrenda wrote:
> > Any program interrupt that happens in the host during the execution of
> > a KVM guest will now short circuit the fault handler and return to KVM
> > immediately. Guest fault handling (including pfault) will happen
> > entirely inside KVM.
> > 
> > When sie64a() returns zero, current->thread.gmap_int_code will contain
> > the program interrupt number that caused the exit, or zero if the exit
> > was not caused by a host program interrupt.
> > 
> > KVM will now take care of handling all guest faults in vcpu_post_run().
> > 
> > Since gmap faults will not be visible by the rest of the kernel, remove
> > GMAP_FAULT, the linux fault handlers for secure execution faults, the
> > exception table entries for the sie instruction, the nop padding after
> > the sie instruction, and all other references to guest faults from the
> > s390 code.  
> 
> ...
> 
> > diff --git a/arch/s390/kernel/traps.c b/arch/s390/kernel/traps.c  
> ...
> > @@ -317,9 +318,23 @@ void noinstr __do_pgm_check(struct pt_regs *regs)
> >  	struct lowcore *lc = get_lowcore();
> >  	irqentry_state_t state;
> >  	unsigned int trapnr;
> > +	union teid teid = { .val = lc->trans_exc_code };
> >  
> >  	regs->int_code = lc->pgm_int_code;
> > -	regs->int_parm_long = lc->trans_exc_code;
> > +	regs->int_parm_long = teid.val;
> > +
> > +	/*
> > +	 * In case of a guest fault, short-circuit the fault handler and return.
> > +	 * This way the sie64a() function will return 0; fault address and
> > +	 * other relevant bits are saved in current->thread.gmap_teid, and
> > +	 * the fault number in current->thread.gmap_int_code. KVM will be
> > +	 * able to use this information to handle the fault.
> > +	 */
> > +	if (test_pt_regs_flag(regs, PIF_GUEST_FAULT) && (teid.as == PSW_BITS_AS_PRIMARY)) {
> > +		current->thread.gmap_teid.val = regs->int_parm_long;
> > +		current->thread.gmap_int_code = regs->int_code & 0xffff;
> > +		return;
> > +	}  
> 
> This check looks suboptimal to me for two reasons:
> 
> - if PIF_GUEST_FAULT is set it should never happen that the normal
>   exception handling code is executed; it is clearly a bug if that
>   would happen, and with the above check this may or may not be
>   recognized with a kernel crash, if I'm not mistaken.
> 
> - __do_pgm_check() is executed for all program interruptions. This
>   includes those interruptions which do not write a teid. Therefore
>   the above check may do something unexpected depending on what teid a
>   previous program interruption wrote. I think the teid.as check
>   should be moved to kvm as well, and only be done for those cases
>   where it is known that the teid contains a valid value.

makes sense

I'll move the checks into the kvm part of the code, and trigger a
KVM_BUG if things are wrong