Re: [Qemu-devel] [PATCH qemu v10 14/14] spapr_pci/spapr_pci_vfio: Support Dynamic DMA Windows (DDW)

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 07/07/2015 09:35 PM, Thomas Huth wrote:
> On Tue, 7 Jul 2015 20:43:44 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>> On 07/07/2015 07:33 PM, Thomas Huth wrote:
>>> On Mon,  6 Jul 2015 12:11:10 +1000
>>> Alexey Kardashevskiy <aik@ozlabs.ru> 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