qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: Bharata B Rao <bharata@linux.ibm.com>
Cc: paulus@ozlabs.org, qemu-ppc@nongnu.org,
	Greg Kurz <groug@kaod.org>,
	qemu-devel@nongnu.org
Subject: Re: [RFC PATCH v0 1/1] target/ppc: Support for H_RPT_INVALIDATE hcall
Date: Mon, 18 Jan 2021 14:08:29 +1100	[thread overview]
Message-ID: <20210118030829.GH2089552@yekko.fritz.box> (raw)
In-Reply-To: <20210115083128.GA2457611@in.ibm.com>

[-- Attachment #1: Type: text/plain, Size: 5624 bytes --]

On Fri, Jan 15, 2021 at 02:01:28PM +0530, Bharata B Rao 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.

Hrm.. that's kind of a problem in itself.  Enabling KVM should not
change the guest visible environment.

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

Pretty much, yes.

> 
> Regards,
> Bharata.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

      parent reply	other threads:[~2021-01-18  3:09 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
2021-01-18  3:08     ` David Gibson [this message]

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=20210118030829.GH2089552@yekko.fritz.box \
    --to=david@gibson.dropbear.id.au \
    --cc=bharata@linux.ibm.com \
    --cc=groug@kaod.org \
    --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).