Re: [Qemu-ppc] [PATCH V2 10/10] target/ppc: Enable RADIX for pseries TCG guest

[David Gibson]

On Wed, Mar 29, 2017 at 02:14:22PM +1100, Suraj Jitindar Singh wrote:
> On Fri, 2017-03-03 at 16:03 +1100, David Gibson wrote:
> > On Wed, Mar 01, 2017 at 06:13:01PM +1100, Suraj Jitindar Singh wrote:
> > > 
> > > Now that we have added all the infrastructure we can enable a
> > > pseries TCG
> > > guest to use radix.
> > > 
> > > We make the assumption that since we can support radix, a guest
> > > will choose
> > > radix, so set the guest radix bit in the partition table entry
> > > unconditionally. This also represents that we haven't allocated an
> > > hpt.
> > > 
> > > We must set spapr->htab_shift even if we aren't going to allocate
> > > an hpt as
> > > we used this to set the pft-size device tree entry which the guest
> > > uses to
> > > determine the pte_index mask in the event it decides to use a hash
> > > mmu.
> > > 
> > > During machine reset if we're on kvm and the hypervisor doesn't
> > > have radix
> > > capability then we have to allocate a hpt, under TCG we can delay
> > > this
> > > until CAS time.
> > > NOTE: Allocating an hpt clears the guest radix bit in patb_entry.
> > > 
> > > During machine init we unconditionally allow GTSE on radix in TCG
> > > and
> > > on KVM if the hypervisor supports radix.
> > > 
> > > When creating the device tree ibm,arch-vec-5-platform-support node
> > > we set
> > > both hash and radix supported if we're on a ISAV3.00 capable
> > > processor,
> > > otherwise we set hash only.
> > > 
> > > During CAS we check if we previously assumed radix (Guest Radix bit
> > > set in
> > > patb_entry) and the guest decided to do hash at which point we have
> > > to
> > > allocate a hpt.
> > > 
> > > Finally we call the radix fault handler if we're a radix guest
> > > instead of
> > > exiting enabling address translation for a radix guest.
> > Not sure about the patch itself, but looks as if these comments at
> > least cover some of the same ground as the CAS stuff in Sam's KVM
> > series - so looks like a little more polish / comment updating is
> > necessary to make the series completely work together.
> 
> Alright, I'll clean it up a bit.
> 
> > 
> > > 
> > > 
> > > Signed-off-by: Suraj Jitindar Singh <address@hidden>
> > > 
> > > ---
> > > 
> > > V1 -> V2:
> > >  - Added patch to series
> > > ---
> > >  hw/ppc/spapr.c             | 39 +++++++++++++++++++++++++++-------
> > > -----
> > >  hw/ppc/spapr_hcall.c       | 13 +++++++------
> > >  target/ppc/mmu-book3s-v3.c |  6 ++----
> > >  3 files changed, 36 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index f6155a6..adc107e 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -850,6 +850,8 @@ static void spapr_dt_rtas(sPAPRMachineState
> > > *spapr, void *fdt)
> > >   * option vector 5: */
> > >  static void spapr_dt_ov5_platform_support(void *fdt, int chosen)
> > >  {
> > > +    PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
> > > +
> > >      char val[2 * 3] = {
> > >          24, 0x00, /* Hash/Radix, filled in below. */
> > >          25, 0x00, /* Hash options: Segment Tables == no, GTSE ==
> > > no. */
> > > @@ -866,8 +868,12 @@ static void spapr_dt_ov5_platform_support(void
> > > *fdt, int chosen)
> > >              val[1] = 0x00; /* Hash */
> > >          }
> > >      } else {
> > > -        /* TODO: TCG case, hash */
> > > -        val[1] = 0x00;
> > > +        if (first_ppc_cpu->env.mmu_model & POWERPC_MMU_V3) {
> > > +            /* Either - TCG supports both so let the guest choose
> > > */
> > > +            val[1] = 0x80;
> > > +        } else {
> > > +            val[0] = 0x00; /* Hash */
> > > +        }
> > >      }
> > >      _FDT(fdt_setprop(fdt, chosen, "ibm,arch-vec-5-platform-
> > > support",
> > >                       val, sizeof(val)));
> > > @@ -1301,15 +1307,24 @@ static void ppc_spapr_reset(void)
> > >      /* Check for unknown sysbus devices */
> > >      foreach_dynamic_sysbus_device(find_unknown_sysbus_device,
> > > NULL);
> > >  
> > > -    if (kvm_enabled()) { /* We assume Radix under KVM */
> > > -        spapr->patb_entry = PATBE1_GR;
> > > -    } else {
> > > -        spapr->patb_entry = 0;
> > > -    }
> > > +    /*
> > > +     * Assume we're radix, this is fine since in real mode we
> > > don't care.
> > > +     * Either we were right and the guest registers a radix
> > > process table,
> > > +     * the host doesn't support radix so we allocate a hpt
> > > directly below,
> > > +     * or finally the guest can choose hash at H_CAS or
> > > H_REGISTER_PROC_TBL
> > > +     * time in which case we allocate an hpt in the H_CALL handler
> > > if one
> > > +     * hasn't already been allocated previously.
> > > +     */
> > > +    spapr->patb_entry = PATBE1_GR;
> > >  
> > > -    /* If using KVM with radix mode available, VCPUs can be
> > > started
> > > -     * without a HPT because KVM will start them in radix mode. */
> > > -    if (!(kvm_enabled() && kvmppc_has_cap_mmu_radix())) {
> > > +    /*
> > > +     * If using KVM with radix mode available, VCPUs can be
> > > started
> > > +     * without a HPT because KVM will start them in radix mode.
> > > +     * In tcg we can always do radix, so don't allocate an hpt
> > > +     */
> > > +    spapr->htab_shift = spapr_hpt_shift_for_ramsize(MACHINE(
> > > +                                qdev_get_machine())->maxram_size);
> > > +    if (kvm_enabled() && !kvmppc_has_cap_mmu_radix()) {
> > >          spapr_setup_hpt_and_vrma(spapr);
> > >      }
> > >  
> > > @@ -2093,8 +2108,8 @@ static void ppc_spapr_init(MachineState
> > > *machine)
> > >      }
> > >  
> > >      spapr_ovec_set(spapr->ov5, OV5_FORM1_AFFINITY);
> > > -    if (kvmppc_has_cap_mmu_radix()) {
> > > -        /* KVM always allows GTSE with radix... */
> > > +    if (!kvm_enabled() || kvmppc_has_cap_mmu_radix()) {
> > > +        /* KVM always allows GTSE with radix, so does TCG */
> > Comment here is unclear: Does TCG *always* allow GTSE, or always
> > allow
> > GTSE when in radix mode?
> 
> Only radix mode for now, I'll be more clear.
> 
> > 
> > > 
> > >          spapr_ovec_set(spapr->ov5, OV5_MMU_RADIX_GTSE);
> > So.. where's the TCG implementation of GTSE?
> 
> GTSE in radix lets the guest use tlbie and tbliel which are already
> implemented. There may be more work to do before we let a guest use
> GTSE in hash although I haven't looked at it closely. But we don't let
> a hash guest do that yet anyway.

Hrm.  But if availability of tlbie depends on something, don't we need
some kind of tweak to the permissions of those instructions?  Seems
to me that it would either need a change for GTSE, or it was wrong
already and should be cleaned up.

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

signature.asc
Description: PGP signature