On Fri, Aug 15, 2014 at 03:27:42PM +1000, Alexey Kardashevskiy wrote:
> 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
Looks good.
--
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
pgp9Z2XTIi8SR.pgp
Description: PGP signature
