Re: [Qemu-devel] [PATCH] spapr: Correctly set LPCR[GTSE] in H_REGISTER_PROCESS_TABLE

[David Gibson]

On Wed, Mar 13, 2019 at 08:17:42AM +0100, Cédric Le Goater wrote:
> On 3/13/19 4:20 AM, David Gibson wrote:
> > 176dccee "target/ppc/spapr: Clear partition table entry when allocating
> > hash table" reworked the H_REGISTER_PROCESS_TABLE hypercall, but
> > unfortunately due to a small error no longer correctly sets the LPCR[GTSE]
> > bit which allows the guest to directly execute (some types of) tlbie (TLB
> > flush) instructions without involving the hypervisor.
> > 
> > We got away with this, initially, because POWER9 did not have hypervisor
> > mode enabled in its msr_mask, which meant we didn't actually run hypervisor
> > privilege checks in TCG at all.  However, da874d90 "target/ppc: add HV
> > support for POWER9" turned on HV support on POWER9 for the benefit of the
> > powernv machine type.
> > 
> > This exposed the earlier bug in H_REGISTER_PROCESS_TABLE, and causes guests
> > which rely on LPCR[GTSE] (i.e. basically all of them) to crash during early
> > boot when their first tlbie instruction causes an unexpected trap.
> > 
> > Fixes: 176dccee target/ppc/spapr: Clear partition table entry when 
> > allocating hash table
> > Signed-off-by: David Gibson <address@hidden>
> 
> Nice catch. 
> 
> May be we should rename the flags from FLAG_XXXX to PROC_TBL_XXXX_FLAG ? 

Meh, the current names are theoretically ambiguous, but they're
short.  If they were truly global I'd be concerned, but given they're
#defined right above I don't think there's any urgency.

> Reviewed-by: Cédric Le Goater <address@hidden>
> 
> Thanks,
> 
> C.
> 
> > ---
> >  hw/ppc/spapr_hcall.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 0761e10142..8a736797b9 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1400,7 +1400,8 @@ static target_ulong 
> > h_register_process_table(PowerPCCPU *cpu,
> >      else if (flags & FLAG_HASH_PROC_TBL) /* Hash with process tables */
> >          update_lpcr |= LPCR_UPRT;
> >      if (flags & FLAG_GTSE)      /* Guest translation shootdown enable */
> > -        update_lpcr |= FLAG_GTSE;
> > +        update_lpcr |= LPCR_GTSE;
> > +
> >      spapr_set_all_lpcrs(update_lpcr, LPCR_UPRT | LPCR_HR | LPCR_GTSE);
> >  
> >      if (kvm_enabled()) {
> > 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature