qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Bharata B Rao <bharata@linux.ibm.com>
To: Greg Kurz <groug@kaod.org>
Cc: paulus@ozlabs.org, qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
	david@gibson.dropbear.id.au
Subject: Re: [RFC PATCH v0 1/1] target/ppc: Support for H_RPT_INVALIDATE hcall
Date: Tue, 19 Jan 2021 10:14:55 +0530	[thread overview]
Message-ID: <20210119044455.GA2587010@in.ibm.com> (raw)
In-Reply-To: <20210115183005.2337dae4@bahia.lan>

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.


  reply	other threads:[~2021-01-19  4:49 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-06  8:59 [RFC PATCH v0 1/1] target/ppc: Support for H_RPT_INVALIDATE hcall Bharata B Rao
2021-01-12 13:16 ` Daniel Henrique Barboza
2021-01-13 14:52   ` Bharata B Rao
2021-01-13 16:22 ` Greg Kurz
2021-01-15  8:31   ` Bharata B Rao
2021-01-15 17:30     ` Greg Kurz
2021-01-19  4:44       ` Bharata B Rao [this message]
2021-01-19  8:01         ` Greg Kurz
2021-01-18  3:08     ` David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210119044455.GA2587010@in.ibm.com \
    --to=bharata@linux.ibm.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=groug@kaod.org \
    --cc=paulus@ozlabs.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).