|
From: | Alexey Kardashevskiy |
Subject: | Re: [Qemu-devel] [PATCH qemu v10 14/14] spapr_pci/spapr_pci_vfio: Support Dynamic DMA Windows (DDW) |
Date: | Tue, 7 Jul 2015 21:53:02 +1000 |
User-agent: | Mozilla/5.0 (X11; Linux i686 on x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.0.1 |
On 07/07/2015 09:35 PM, Thomas Huth wrote:
On Tue, 7 Jul 2015 20:43:44 +1000 Alexey Kardashevskiy <address@hidden> wrote:On 07/07/2015 07:33 PM, Thomas Huth wrote:On Mon, 6 Jul 2015 12:11:10 +1000 Alexey Kardashevskiy <address@hidden> 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
[Prev in Thread] | Current Thread | [Next in Thread] |