Re: [Qemu-ppc] [Qemu-devel] [RFC PATCH 06/10] spapr_rtas: Add Dynamic DMA windows (DDW) RTAS calls support

[David Gibson]

On Tue, Aug 12, 2014 at 05:25:29PM +1000, Alexey Kardashevskiy wrote:
> On 08/12/2014 11:45 AM, David Gibson wrote:
> > On Thu, Jul 31, 2014 at 07:34:10PM +1000, Alexey Kardashevskiy
> wrote:
[snip]
> > The function of this is kind of unclear.  I'm assuming this is
> > filtering the supported page sizes reported by the PHB by the possible
> > page sizes based on host page size or other constraints.  Is that
> > right?
> > 
> > I think you'd be better off folding the whole double loop into the
> > fixmask function.
> > 
> >> +
> >> +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> >> +    rtas_st(rets, 1, windows_available);
> >> +    /* Return maximum number as all RAM was 4K pages */
> >> +    rtas_st(rets, 2, ram_size >> SPAPR_TCE_PAGE_SHIFT);
> > 
> > I'm assuming this is the allowed size of the dynamic windows.
> > Shouldn't that be reported by a PHB callback, rather than hardcoded
> > here?
> 
> Why PHB? This is DMA memory. @ram_size is the upper limit, we can make more
> only when we have memory hotplug (which we do not have) and the guest can
> create smaller windows if it wants so I do not really follow you here.

What I'm not clear on is what this RTAS return actually means.  Is it
saying the maximum size of the DMA window, or the maximum address
which can be mapped by that window?  Remember I don't have access to
PAPR documentation any more - nor do others reading these patches.

[snip]
> >> +static void rtas_ibm_create_pe_dma_window(PowerPCCPU *cpu,
> >> +                                          sPAPREnvironment *spapr,
> >> +                                          uint32_t token, uint32_t nargs,
> >> +                                          target_ulong args,
> >> +                                          uint32_t nret, target_ulong 
> >> rets)
> >> +{
> >> +    sPAPRPHBState *sphb;
> >> +    sPAPRPHBClass *spc;
> >> +    sPAPRTCETable *tcet = NULL;
> >> +    uint32_t addr, page_shift, window_shift, liobn;
> >> +    uint64_t buid;
> >> +    long ret;
> >> +
> >> +    if ((nargs != 5) || (nret != 4)) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> >> +    addr = rtas_ld(args, 0);
> >> +    sphb = spapr_pci_find_phb(spapr, buid);
> >> +    if (!sphb) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> >> +    if (!spc->ddw_create) {
> >> +        goto hw_error_exit;
> >> +    }
> >> +
> >> +    page_shift = rtas_ld(args, 3);
> >> +    window_shift = rtas_ld(args, 4);
> >> +    liobn = sphb->dma_liobn + 0x10000;
> > 
> > Isn't using a fixed LIOBN here assuming you can only have a single DDW
> > per PHB?  That's true for now, but in theory shouldn't it be reported
> > by the PHB code itself?
> 
> 
> This should be a unique LIOBN so it is not up to PHB to choose. And we
> cannot make it completely random for migration purposes. I'll make it
> something like
> 
> #define SPAPR_DDW_LIOBN(sphb, windownum) ((sphb)->dma_liobn | windownum)

Ok.

Really, the assigned liobns should be included in the migration stream
if they're not already.  Relying them on them being set consistently
at startup is going to be really fragile.

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

pgp1lxF5vUpCK.pgp
Description: PGP signature