From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51925) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XIANM-0001gO-SF for qemu-devel@nongnu.org; Fri, 15 Aug 2014 01:48:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XIANG-0008F0-I7 for qemu-devel@nongnu.org; Fri, 15 Aug 2014 01:48:32 -0400 Date: Fri, 15 Aug 2014 15:30:45 +1000 From: David Gibson Message-ID: <20140815053045.GO7628@voom.redhat.com> References: <1406799254-25223-1-git-send-email-aik@ozlabs.ru> <1406799254-25223-7-git-send-email-aik@ozlabs.ru> <20140812014533.GE7628@voom.redhat.com> <53E9C169.5060809@ozlabs.ru> <20140813032751.GG7628@voom.redhat.com> <53EC737E.4080203@ozlabs.ru> <20140815000407.GK7628@voom.redhat.com> <53ED79E0.3020203@ozlabs.ru> <20140815042052.GN7628@voom.redhat.com> <53ED9A4E.5000600@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="0lsrIB+s628ok5gC" Content-Disposition: inline In-Reply-To: <53ED9A4E.5000600@ozlabs.ru> Subject: Re: [Qemu-devel] [RFC PATCH 06/10] spapr_rtas: Add Dynamic DMA windows (DDW) RTAS calls support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Alexander Graf --0lsrIB+s628ok5gC Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 wrot= e: > >>>>>> 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 pos= sible > >>>>>>> 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 hardcod= ed > >>>>>>> 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 gu= est can > >>>>>> create smaller windows if it wants so I do not really follow you h= ere. > >>>>> > >>>>> 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 specific= ally > >>>> 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 *spa= pr, > >>>>>>>> + uint32_t token, uint3= 2_t nargs, > >>>>>>>> + target_ulong args, > >>>>>>>> + uint32_t nret, target= _ulong rets) > >>>>>>>> +{ > >>>>>>>> + sPAPRPHBState *sphb; > >>>>>>>> + sPAPRPHBClass *spc; > >>>>>>>> + sPAPRTCETable *tcet =3D NULL; > >>>>>>>> + uint32_t addr, page_shift, window_shift, liobn; > >>>>>>>> + uint64_t buid; > >>>>>>>> + long ret; > >>>>>>>> + > >>>>>>>> + if ((nargs !=3D 5) || (nret !=3D 4)) { > >>>>>>>> + goto param_error_exit; > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> + buid =3D ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args,= 2); > >>>>>>>> + addr =3D rtas_ld(args, 0); > >>>>>>>> + sphb =3D spapr_pci_find_phb(spapr, buid); > >>>>>>>> + if (!sphb) { > >>>>>>>> + goto param_error_exit; > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> + spc =3D SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb); > >>>>>>>> + if (!spc->ddw_create) { > >>>>>>>> + goto hw_error_exit; > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> + page_shift =3D rtas_ld(args, 3); > >>>>>>>> + window_shift =3D rtas_ld(args, 4); > >>>>>>>> + liobn =3D sphb->dma_liobn + 0x10000; > >>>>>>> > >>>>>>> Isn't using a fixed LIOBN here assuming you can only have a singl= e DDW > >>>>>>> per PHB? That's true for now, but in theory shouldn't it be repo= rted > >>>>>>> by the PHB code itself? > >>>>>> > >>>>>> > >>>>>> This should be a unique LIOBN so it is not up to PHB to choose. An= d we > >>>>>> cannot make it completely random for migration purposes. I'll make= it > >>>>>> something like > >>>>>> > >>>>>> #define SPAPR_DDW_LIOBN(sphb, windownum) ((sphb)->dma_liobn | wind= ownum) > >>>>> > >>>>> Ok. > >>>>> > >>>>> Really, the assigned liobns should be included in the migration str= eam > >>>>> 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 handmad= e 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. > >=20 > > Ah, duh. Clearly I'm still swapping back in my knowledge of qemu > > migration. > >=20 > > 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. > >=20 > > 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). > >=20 >=20 > Yes, this is my new queue: >=20 > ------------------------------ hw/ppc/spapr_pci.c > ------------------------------ > index 5c46c0d..17eb0d8 100644 > @@ -527,11 +527,11 @@ static void spapr_phb_realize(DeviceState *dev, Err= or > **errp) > " be specified for PAPR PHB, not both"); > return; > } >=20 > sphb->buid =3D SPAPR_PCI_BASE_BUID + sphb->index; > - sphb->dma_liobn =3D SPAPR_PCI_BASE_LIOBN + sphb->index; > + sphb->dma_liobn =3D SPAPR_PCI_LIOBN(sphb->index, 0); >=20 > windows_base =3D SPAPR_PCI_WINDOW_BASE > + sphb->index * SPAPR_PCI_WINDOW_SPACING; > sphb->mem_win_addr =3D windows_base + SPAPR_PCI_MMIO_WIN_OFF; > sphb->io_win_addr =3D windows_base + SPAPR_PCI_IO_WIN_OFF; >=20 > ---------------------------- 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) >=20 > #define SPAPR_VIO_BASE_LIOBN 0x00000000 > -#define SPAPR_PCI_BASE_LIOBN 0x80000000 > +#define SPAPR_PCI_LIOBN(i, n) (0x80000000 | ((i) << 8) | (n)) >=20 > #define RTAS_ERROR_LOG_MAX 2048 Looks good. --=20 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 --0lsrIB+s628ok5gC Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJT7ZsFAAoJEGw4ysog2bOSfFYQAJjTrtxBeLkOdP1jKeKom/dK r5ZNwP0YvloSGdy0TI/5m7k8ih1o09t/StjU4q7Sh+fJTfB1cBb3KtbT7+V9zprk bZKq6Kj3KKAWE4RHjdt8ORZxR8t/9EJVzWPbrScPcDGKRwCbXhzAocmLztcolYqh 2h7OX6ADLiMVlhe/GuEQOkkWcQChWCIfA55F+g/R0rA8jrOzT3fk50zsw9Oo9ISo L3YZ/y8HxAvzHo6oxErbR5ducRapjvFAWUK5/XWnMKkwqNWkhw+BRgDrMz0j8ewL J5lRXzok3HCCF5tmzPHzAaux+gG5/FDMF/ZuMkLZ0vh8amnmT2QzbPcX8CYePrwl 73FZTbQolN6O2OJaL2YLUoQHN27FMUwCSQyZ4lCebqxFWCdFwUefsXk6OuBzbPeJ T4v0obNU5QNZpXMxkG9hhHA2l3JdAfB0f/U5soovgHwPO6a8R6i0j8NgZPv3/MY8 xEsEXbG9rBwKgQoWHTPdDzfr2v2N5x4jD5954LLtdgVhU81qzJd0ZoL/YVgTEtUh vujrZ2gVZSInClYRvLulgiaHHsekTuxYpCKJMSIAJHqI3tGYXsVxQMZiAWXPk46z 7JJqhI6UDteI23XV+tvrA9KJeOv8cg9Bcrl0MvI9t3FLoGr0iT0rO69MFhsN9fOo 7zH3AZe4EgoSoc8FkH8Q =B83X -----END PGP SIGNATURE----- --0lsrIB+s628ok5gC--