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

[Alexey Kardashevskiy]

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.



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


>  Relying them on them being set consistently
> at startup is going to be really fragile.


-- 
Alexey