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

[Alexey Kardashevskiy]

On 08/15/2014 02:20 PM, David Gibson wrote:
> On Fri, Aug 15, 2014 at 01:09:20PM +1000, Alexey Kardashevskiy wrote:
>> On 08/15/2014 10:04 AM, David Gibson wrote:
>>> 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?
>>
>>
>> No. If we did so, exact numbers would depend on the device order in the
>> QEMU command line - QEMU command line produced by libvirt from handmade XML
>> and QEMU command line produced by libvirt from XML printed by "dumpxml" can
>> have devices in different order, so interrupt numbers, LIOBNs - all of this
>> gets broken.
> 
> Ah, duh.  Clearly I'm still swapping back in my knowledge of qemu
> migration.
> 
> What I was meaning before is that qemu should reconstruct the TCE
> tables / liobns based on the info in the migration stream, which
> remains true, but it can't really be done without more broadly
> addressing the fact that qemu doesn't transmit the hardware
> configuration in the migration stream.
> 
> Ok, so what should probably be done here is to explicitly assign
> fields in the LIOBN to "device type", "device id" (vio reg or PHB
> buid), and per-device instance (DDW number for PCI).
> 

Yes, this is my new queue:

------------------------------ hw/ppc/spapr_pci.c
------------------------------
index 5c46c0d..17eb0d8 100644
@@ -527,11 +527,11 @@ static void spapr_phb_realize(DeviceState *dev, Error
**errp)
                        " be specified for PAPR PHB, not both");
             return;
         }

         sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
-        sphb->dma_liobn = SPAPR_PCI_BASE_LIOBN + sphb->index;
+        sphb->dma_liobn = SPAPR_PCI_LIOBN(sphb->index, 0);

         windows_base = SPAPR_PCI_WINDOW_BASE
             + sphb->index * SPAPR_PCI_WINDOW_SPACING;
         sphb->mem_win_addr = windows_base + SPAPR_PCI_MMIO_WIN_OFF;
         sphb->io_win_addr = windows_base + SPAPR_PCI_IO_WIN_OFF;

---------------------------- include/hw/ppc/spapr.h
----------------------------
index c9d6c6c..92c6e2c 100644
@@ -441,11 +441,11 @@ int spapr_rtas_device_tree_setup(void *fdt, hwaddr
rtas_addr,
 #define SPAPR_TCE_PAGE_SHIFT   12
 #define SPAPR_TCE_PAGE_SIZE    (1ULL << SPAPR_TCE_PAGE_SHIFT)
 #define SPAPR_TCE_PAGE_MASK    (SPAPR_TCE_PAGE_SIZE - 1)

 #define SPAPR_VIO_BASE_LIOBN    0x00000000
-#define SPAPR_PCI_BASE_LIOBN    0x80000000
+#define SPAPR_PCI_LIOBN(i, n)   (0x80000000 | ((i) << 8) | (n))

 #define RTAS_ERROR_LOG_MAX      2048




-- 
Alexey