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

[Bharata B Rao]

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.

Regards,
Bharata.