Re: [Qemu-ppc] [RFC NO-MERGE 11/12] target/ppc: Implement ISA V3.00 radix page fault handler

[Suraj Jitindar Singh]

On Thu, 2017-03-02 at 12:29 +1100, David Gibson wrote:
> On Wed, Mar 01, 2017 at 05:15:12PM +1100, Suraj Jitindar Singh wrote:
> > 
> > On Tue, 2017-02-28 at 13:29 +1100, David Gibson wrote:
> > > 
> > > On Fri, Feb 24, 2017 at 06:18:50PM +1100, Suraj Jitindar Singh
> > > wrote:
> > > > 
> > > > 
> > > > On Mon, 2017-02-20 at 14:16 +1100, David Gibson wrote:
> > > > > 
> > > > > 
> > > > > On Fri, Feb 17, 2017 at 04:08:11PM +1100, Suraj Jitindar
> > > > > Singh
> > > > wrote:
> > > [snip]
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > +int ppc_radix64_handle_mmu_fault(PowerPCCPU *cpu, vaddr
> > > > > > eaddr,
> > > > > > int
> > > > > > rwx,
> > > > > > +                                 int mmu_idx)
> > > > > > +{
> > > > > > +    CPUState *cs = CPU(cpu);
> > > > > > +    CPUPPCState *env = &cpu->env;
> > > > > > +    PPCVirtualHypervisorClass *vhc =
> > > > > > +        PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> > > > > > +    hwaddr raddr;
> > > > > > +    uint64_t lpid = 0, pid = 0, offset, size, patbe,
> > > > > > prtbe0;
> > > > > > +    int page_size, prot, fault_cause = 0;
> > > > > > +
> > > > > > +    assert((rwx == 0) || (rwx == 1) || (rwx == 2));
> > > > > > +
> > > > > > +    /* Real Mode Access */
> > > > > > +    if (((rwx == 2) && (msr_ir == 0)) || ((rwx != 2) &&
> > > > > > (msr_dr ==
> > > > > > 0))) {
> > > > > > +        /* In real mode top 4 effective addr bits (mostly)
> > > > > > ignored
> > > > > > */
> > > > > > +        raddr = eaddr & 0x0FFFFFFFFFFFFFFFULL;
> > > > > > +
> > > > > > +        if (msr_hv) {
> > > > > > +            /* if EA0 == 0, raddr = eaddr4:63 | HRMOR */
> > > > > > +            if (!(eaddr >> 63)) {
> > > > > > +                raddr |= env->spr[SPR_HRMOR];
> > > > > > +            }
> > > > > > +        } else {
> > > > > > +            /* TODO */
> > > > > > +            /* PowerNV ONLY */
> > > > > Uh.. isn't this the wrong way around?  msr_hv is set in
> > > > > hypervisor
> > > > > mode, isn't it?
> > > > Um, so on powernv we will have to do partition-scoped
> > > > translation
> > > > which
> > > > we don't have to worry about yet but this is where it would go.
> > > > Technically the HRMOR stuff is powernv as well and will never
> > > > be
> > > > reached. It just seemed so easy to add now...
> > > Sure, but that doesn't actually address the point I'm making -
> > > for
> > > pseries msr_hv will be always false, so you'll go down the
> > > PowerNV
> > > partition translation path, which isn't right.
> > So on that path we'll need a PowerNV check. If PowerNV we'll do
> > partition scoped translation, otherwise do nothing.
> More the other way around, I think.  The "normal" path would go and
> do
> partition level translation, but you skip that when vhyp is set.

Yes, but that's just the two sides of an if statement :)

> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > In fact, there are multiple places further down this function
> > > > > which
> > > > > won't work for powernv anyway (e.g. getting the patbe, not
> > > > > translating
> > > > > guest-guest to guest-host addresses for reading the process
> > > > > table).
> > > > > So you might as well just assert (cpu->vhyp) up the top and
> > > > > be
> > > > > done
> > > > True - I'll do that