| Message ID | 20190313032020.4934-1-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr: Correctly set LPCR[GTSE] in H_REGISTER_PROCESS_TABLE | expand |
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 <david@gibson.dropbear.id.au> Nice catch. May be we should rename the flags from FLAG_XXXX to PROC_TBL_XXXX_FLAG ? Reviewed-by: Cédric Le Goater <clg@kaod.org> 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()) { >
On Wed, 13 Mar 2019 14:20:20 +1100 David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > 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()) {
On Wed, Mar 13, 2019 at 02:20:20PM +1100, 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 <david@gibson.dropbear.id.au> Tested-by: Cleber Rosa <crosa@redhat.com>
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 <david@gibson.dropbear.id.au> > > 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 <clg@kaod.org> > > 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()) { > > >
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()) {
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 <david@gibson.dropbear.id.au> --- hw/ppc/spapr_hcall.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)