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
