Re: [Qemu-devel] [PATCH 4/6] target/ppc: Cleanup HPTE accessors for 64-bit hash MMU

[David Gibson]

On Thu, Feb 23, 2017 at 04:02:54PM +1100, Alexey Kardashevskiy wrote:
> On 23/02/17 13:09, David Gibson wrote:
> > Accesses to the hashed page table (HPT) are complicated by the fact that
> > the HPT could be in one of three places:
> >    1) Within guest memory - when we're emulating a full guest CPU at the
> >       hardware level (e.g. powernv, mac99, g3beige)
> >    2) Within qemu, but outside guest memory - when we're emulating user and
> >       supervisor instructions within TCG, but instead of emulating
> >       the CPU's hypervisor mode, we just emulate a hypervisor's behaviour
> >       (pseries in TCG)
> >    3) Within KVM - a pseries machine using KVM acceleration.  Mostly
> >       accesses to the HPT are handled by KVM, but there are a few cases
> >       where qemu needs to access it via a special fd for the purpose.
> > 
> > In order to batch accesses to the fd in case (3), we use a somewhat awkward
> > ppc_hash64_start_access() / ppc_hash64_stop_access() pair, which for case
> > (3) reads / releases a whole PTEG from the kernel.  For cases (1) & (2)
> > it just returns an address value.  The actual HPTE load helpers then need
> > to interpret the returned token differently in the 3 cases.
> > 
> > This patch keeps the same basic structure, but simplfiies the details.
> > First start_access() / stop_access() are renamed to get_pteg() and
> > put_pteg() to make their operation more obvious.

[snip]
> >  
> > /*****************************************************************************/
> > -/* Types used to describe some PowerPC registers */
> > +/* Types used to describe some PowerPC registers etc. */
> >  typedef struct DisasContext DisasContext;
> >  typedef struct ppc_spr_t ppc_spr_t;
> >  typedef union ppc_avr_t ppc_avr_t;
> >  typedef union ppc_tlb_t ppc_tlb_t;
> > +typedef struct ppc_hash_pte64 ppc_hash_pte64_t;
> >  
> >  /* SPR access micro-ops generations callbacks */
> >  struct ppc_spr_t {
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 52bbea5..9d3e57e 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -2601,17 +2601,17 @@ struct kvm_get_htab_buf {
> >      /*
> >       * We require one extra byte for read
> >       */
> > -    target_ulong hpte[(HPTES_PER_GROUP * 2) + 1];
> > +    ppc_hash_pte64_t hpte[HPTES_PER_GROUP];
> 
> 
> The "one extra byte" (which was ulong) is not needed any more why?
> 
> 
> >  };
> >  
> > -uint64_t kvmppc_hash64_read_pteg(PowerPCCPU *cpu, target_ulong pte_index)
> > +const ppc_hash_pte64_t *kvmppc_map_hptes(hwaddr ptex, int n)
> 
> 
> This "int n" is ignored here by a reason?

So looking at these comments, I realized the current code for reading
the HPTEs from KVM is just broken.  So, for v2, I've prepended a patch
to fix that first, after which I don't need to touch the KVM side for
the rest of the series.

[snip]
> > +const ppc_hash_pte64_t *ppc_hash64_map_hptes(PowerPCCPU *cpu,\
> 
> 
> You do not need the trailing '\'.

Missed this comment on my first pass, fixed now, thanks.

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