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

[Greg Kurz <groug@kaod.org>]

On Tue, 19 Jan 2021 10:14:55 +0530
Bharata B Rao <bharata@linux.ibm.com> wrote:

> On Fri, Jan 15, 2021 at 06:30:05PM +0100, Greg Kurz wrote:
> > On Fri, 15 Jan 2021 14:01:28 +0530
> > Bharata B Rao <bharata@linux.ibm.com> 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.
> > > 
> > > > 
> > > > > +        (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.
> > 
> > I don't think this is relevant, as the importance of each case can change,
> > e.g. nested is gaining momentum.
> > 
> > > Hence I thought keeping it off by default and expecting the
> > > user to turn it on only if required would be correct.
> > > 
> > 
> > If the feature is an improvement, even for what is considered a corner
> > case now, and it doesn't do harm to setups that won't use it, then it
> > should be enabled IMHO.
> > 
> > > 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?
> > > 
> > 
> > That's perfectly fine and this is why we should set it to ON
> > for the default machine type only.
> 
> The property can be turned ON only when the hypervisor supports
> the hcall. So if it set to ON for default machine type, then
> it may fail if the host doesn't have this hcall. Hence I thought
> it should be OFF by default and turning ON should be left to the
> user.
> 

Ok. This can be changed later when H_RPT_INVALIDATE support is
more widely available. BTW, if users are expected to manually
set this, I think you should add some documentation so that
they know how/when to use it.

> Regards,
> Bharata.