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

[Cédric Le Goater]

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 ? 

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()) {
>