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
pgp5vE1HD9uCb.pgp
Description: PGP signature
