Re: [RFC PATCH v0 1/1] target/ppc: Support for H_RPT_INVALIDATE hcall

[David Gibson]

On Fri, Jan 15, 2021 at 02:01:28PM +0530, Bharata B Rao wrote:
> On Wed, Jan 13, 2021 at 05:22:56PM +0100, Greg Kurz wrote:
> > Hi Bharata,
> > 
> > On Wed,  6 Jan 2021 14:29:10 +0530
> > Bharata B Rao <bharata@linux.ibm.com> wrote:
> > 
> > > If KVM_CAP_RPT_INVALIDATE KVM capability is enabled, then
> > > 
> > > - indicate the availability of H_RPT_INVALIDATE hcall to the guest via
> > >   ibm,hypertas-functions property.
> > > - Enable the hcall
> > > 
> > > Both the above are done only if the new sPAPR machine capability
> > > cap-rpt-invalidate is set.
> > > 
> > > Note: The KVM implementation of the hcall has been posted for upstream
> > > review here:
> > > https://lore.kernel.org/linuxppc-dev/20210105090557.2150104-1-bharata@linux.ibm.com/T/#t
> > > 
> > > Update to linux-headers/linux/kvm.h here is temporary, will be
> > > done via header updates once the kernel change is accepted upstream.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
> > > ---
> > 
> > Patch looks mostly fine. A few remarks below.
> > 
> > >  hw/ppc/spapr.c            |  7 ++++++
> > >  hw/ppc/spapr_caps.c       | 49 +++++++++++++++++++++++++++++++++++++++
> > >  include/hw/ppc/spapr.h    |  8 +++++--
> > >  linux-headers/linux/kvm.h |  1 +
> > >  target/ppc/kvm.c          | 12 ++++++++++
> > >  target/ppc/kvm_ppc.h      | 11 +++++++++
> > >  6 files changed, 86 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 489cefcb81..0228083800 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -890,6 +890,11 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, 
> > > void *fdt)
> > >      add_str(hypertas, "hcall-copy");
> > >      add_str(hypertas, "hcall-debug");
> > >      add_str(hypertas, "hcall-vphn");
> > > +    if (kvm_enabled() &&
> > 
> > You shouldn't check KVM here. The capability is enough to decide if we
> > should expose "hcall-rpt-invalidate" or not. FWIW we won't even reach
> > this code when running with anything but KVM.
> 
> Correct, the capability itself can be only for KVM case.

Hrm.. that's kind of a problem in itself.  Enabling KVM should not
change the guest visible environment.

> 
> > 
> > > +        (spapr_get_cap(spapr, SPAPR_CAP_RPT_INVALIDATE) == 
> > > SPAPR_CAP_ON)) {
> > > +        add_str(hypertas, "hcall-rpt-invalidate");
> > > +    }
> > > +
> > >      add_str(qemu_hypertas, "hcall-memop1");
> > >  
> > >      if (!kvm_enabled() || kvmppc_spapr_use_multitce()) {
> > > @@ -2021,6 +2026,7 @@ static const VMStateDescription vmstate_spapr = {
> > >          &vmstate_spapr_cap_ccf_assist,
> > >          &vmstate_spapr_cap_fwnmi,
> > >          &vmstate_spapr_fwnmi,
> > > +        &vmstate_spapr_cap_rpt_invalidate,
> > >          NULL
> > >      }
> > >  };
> > > @@ -4478,6 +4484,7 @@ static void spapr_machine_class_init(ObjectClass 
> > > *oc, void *data)
> > >      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
> > >      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
> > >      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
> > > +    smc->default_caps.caps[SPAPR_CAP_RPT_INVALIDATE] = SPAPR_CAP_OFF;
> > 
> > Any reason for not enabling this for the default machine type and
> > disabling it for existing machine types only ?
> 
> If this capability is enabled, then
> 
> 1. First level guest (L1) can off-load the TLB invalidations to the
> new hcall if the platform has disabled LPCR[GTSE].
> 
> 2. Nested guest (L2) will switch to this new hcall rather than using
> the old H_TLB_INVALIDATE hcall.
> 
> Case 2 is optional and case 1 makes sense only if LPCR[GTSE]=off.
> Hence I thought keeping it off by default and expecting the
> user to turn it on only if required would be correct.
> 
> Please note that turning this capability ON will result in the
> new hcall being exposed to the guest. I hope this is the right
> usage of spapr-caps?
> 
> > > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> > > index 73ce2bc951..8e27f8421f 100644
> > > --- a/target/ppc/kvm_ppc.h
> > > +++ b/target/ppc/kvm_ppc.h
> > > @@ -24,6 +24,7 @@ void kvmppc_enable_logical_ci_hcalls(void);
> > >  void kvmppc_enable_set_mode_hcall(void);
> > >  void kvmppc_enable_clear_ref_mod_hcalls(void);
> > >  void kvmppc_enable_h_page_init(void);
> > > +void kvmppc_enable_h_rpt_invalidate(void);
> > >  void kvmppc_set_papr(PowerPCCPU *cpu);
> > >  int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
> > >  void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
> > > @@ -72,6 +73,7 @@ bool kvmppc_has_cap_nested_kvm_hv(void);
> > >  int kvmppc_set_cap_nested_kvm_hv(int enable);
> > >  int kvmppc_get_cap_large_decr(void);
> > >  int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable);
> > > +int kvmppc_has_cap_rpt_invalidate(void);
> > >  int kvmppc_enable_hwrng(void);
> > >  int kvmppc_put_books_sregs(PowerPCCPU *cpu);
> > >  PowerPCCPUClass *kvm_ppc_get_host_cpu_class(void);
> > > @@ -151,6 +153,10 @@ static inline void kvmppc_enable_h_page_init(void)
> > >  {
> > >  }
> > >  
> > > +static inline void kvmppc_enable_h_rpt_invalidate(void)
> > > +{
> > 
> > g_assert_not_reached() ?
> 
> Don't see many others doing that, is that a new preferred
> way?

Pretty much, yes.

> 
> Regards,
> Bharata.
> 

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