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

[Bharata B Rao]

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.

> 
> > +        (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?

Regards,
Bharata.