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