From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50832) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCRR5-0001FR-TS for qemu-devel@nongnu.org; Tue, 07 Jul 2015 07:53:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCRR3-0006gR-5x for qemu-devel@nongnu.org; Tue, 07 Jul 2015 07:53:15 -0400 Received: from mail-pa0-f48.google.com ([209.85.220.48]:33060) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCRR2-0006g4-VX for qemu-devel@nongnu.org; Tue, 07 Jul 2015 07:53:13 -0400 Received: by pacws9 with SMTP id ws9so113700502pac.0 for ; Tue, 07 Jul 2015 04:53:10 -0700 (PDT) References: <1436148670-6592-1-git-send-email-aik@ozlabs.ru> <1436148670-6592-15-git-send-email-aik@ozlabs.ru> <20150707113335.66821a88@thh440s> <559BAD60.50701@ozlabs.ru> <20150707133512.456ee007@thh440s> From: Alexey Kardashevskiy Message-ID: <559BBD9E.9020402@ozlabs.ru> Date: Tue, 7 Jul 2015 21:53:02 +1000 MIME-Version: 1.0 In-Reply-To: <20150707133512.456ee007@thh440s> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu v10 14/14] spapr_pci/spapr_pci_vfio: Support Dynamic DMA Windows (DDW) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: Michael Roth , qemu-devel@nongnu.org, Gavin Shan , Alex Williamson , qemu-ppc@nongnu.org, David Gibson On 07/07/2015 09:35 PM, Thomas Huth wrote: > On Tue, 7 Jul 2015 20:43:44 +1000 > Alexey Kardashevskiy wrote: > >> On 07/07/2015 07:33 PM, Thomas Huth wrote: >>> On Mon, 6 Jul 2015 12:11:10 +1000 >>> Alexey Kardashevskiy wrote: > ... >>>> +static void rtas_ibm_create_pe_dma_window(PowerPCCPU *cpu, >>>> + sPAPRMachineState *spapr, >>>> + uint32_t token, uint32_t nargs, >>>> + target_ulong args, >>>> + uint32_t nret, target_ulong rets) >>>> +{ >>>> + sPAPRPHBState *sphb; >>>> + sPAPRTCETable *tcet = NULL; >>>> + uint32_t addr, page_shift, window_shift, liobn; >>>> + uint64_t buid; >>>> + long ret; >>>> + >>>> + if ((nargs != 5) || (nret != 4)) { >>> >>> Pascal bracket style again :-( >> >> >> Am I breaking any code design guideline here? > > No, but my Pascal allergy causes me to sneeze here ;-) I feel cold when I do not see braces in cases like this ;) >>>> + goto param_error_exit; >>>> + } >>>> + >>>> + buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2); >> >> But here braces are ok? :-/ > > You could remove them, too. But I did not need to sneeze here. :) >>>> + addr = rtas_ld(args, 0); >>>> + sphb = spapr_pci_find_phb(spapr, buid); >>>> + if (!sphb || !sphb->ddw_enabled) { >>>> + goto param_error_exit; >>>> + } >>>> + >>>> + page_shift = rtas_ld(args, 3); >>>> + window_shift = rtas_ld(args, 4); >>>> + liobn = spapr_phb_get_free_liobn(sphb); >>>> + >>>> + if (!liobn || !(sphb->page_size_mask & (1ULL << page_shift))) { >>>> + goto hw_error_exit; >>>> + } >>>> + >>>> + ret = spapr_phb_dma_init_window(sphb, liobn, page_shift, >>>> + 1ULL << window_shift); >>> >>> As already mentioned in a comment to another patch in this series, I >>> think it maybe might be better to do some sanity checks on the >>> window_shift value, too? >> >> >> Well, as you suggested, I added a check to spapr_phb_dma_init_window() >> which makes this code return RTAS_OUT_HW_ERROR. Or I can add this here: >> >> if (window_shift < page_shift) { >> goto param_error_exit; >> } >> >> and RTAS handler will return RTAS_OUT_PARAM_ERROR. >> SPAPR does not say what is the correct reponse in this case... > > Both error codes sound ok for me here, so do whatever you think is best. RTAS_OUT_PARAM_ERROR it is then. >>>> + >>>> + rtas_st(rets, 0, RTAS_OUT_SUCCESS); >>>> + rtas_st(rets, 1, liobn); >>>> + rtas_st(rets, 2, tcet->bus_offset >> 32); >>>> + rtas_st(rets, 3, tcet->bus_offset & ((uint32_t) -1)); >>> >>> Why don't you simply use 0xffffffff instead of ((uint32_t) -1) ? >>> That's shorter and much easier to understand at a first glance than >>> calulating the type-cast in your brain ;-) >> >> >> At a first glance I cannot tell if there are 7 or 8 or 9 "f"s in >> 0xffffffff. I may accidentally add/remove one "f" and nobody will notice. >> Such typecast of (-1) is quite typical. > > But IMHO it's ugly to use it to mask a value to the lower 32 bits this > way. At least I had to read this twice to understand what you're > trying to achieve here. So if you don't like the 0xffffffff, what about > simply using: > > rtas_st(rets, 3, (uint32_t)tcet->bus_offset); > > ? I believe there are compilers which will warn me than I am loosing upper 32bits. -- Alexey