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

[David Gibson]

On Thu, Aug 14, 2014 at 06:29:50PM +1000, Alexey Kardashevskiy wrote:
> On 08/13/2014 01:27 PM, David Gibson wrote:
> > 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.
> 
> 
> It is literally "Largest contiguous block of TCEs allocated specifically
> for (that is, are reserved for) this PE". Which I understand as the maximum
> number of TCEs.

Ok, so essentially it's a property of the IOMMU.  Hrm, I guess
ram_size is good enough for now then.

[snip]
> > [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.
> 
> LIOBNs already migrate, liobn itself is an instance id of a TCE table
> object in the migration stream.

Ok, so couldn't we just add an alloc_liobn() function instead of
hardcoding how the liobns are constructed?

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

pgpLT1pRF0IHv.pgp
Description: PGP signature