qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Greg Kurz <groug@kaod.org>
To: Bharata B Rao <bharata@linux.ibm.com>
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 09:01:38 +0100	[thread overview]
Message-ID: <20210119090138.05ebf18a@bahia.lan> (raw)
In-Reply-To: <20210119044455.GA2587010@in.ibm.com>

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.



  reply	other threads:[~2021-01-19  8:03 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-06  8:59 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
2021-01-19  8:01         ` Greg Kurz [this message]
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=20210119090138.05ebf18a@bahia.lan \
    --to=groug@kaod.org \
    --cc=bharata@linux.ibm.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=paulus@ozlabs.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    --subject='Re: [RFC PATCH v0 1/1] target/ppc: Support for H_RPT_INVALIDATE hcall' \
    /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

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