From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52036) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XHqQA-0006kJ-9a for qemu-devel@nongnu.org; Thu, 14 Aug 2014 04:30:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XHqQ2-0002ru-Pz for qemu-devel@nongnu.org; Thu, 14 Aug 2014 04:30:06 -0400 Received: from mail-pd0-f182.google.com ([209.85.192.182]:45442) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XHqQ2-0002rn-I9 for qemu-devel@nongnu.org; Thu, 14 Aug 2014 04:29:58 -0400 Received: by mail-pd0-f182.google.com with SMTP id fp1so1195858pdb.13 for ; Thu, 14 Aug 2014 01:29:57 -0700 (PDT) Message-ID: <53EC737E.4080203@ozlabs.ru> Date: Thu, 14 Aug 2014 18:29:50 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 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> In-Reply-To: <20140813032751.GG7628@voom.redhat.com> Content-Type: text/plain; charset=koi8-r Content-Transfer-Encoding: 7bit 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: David Gibson Cc: Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Alexander Graf 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