lkml.org 
[lkml]   [2017]   [Aug]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v7 12/12] powerpc/vas: Define copy/paste interfaces
Michael Ellerman [mpe@ellerman.id.au] wrote:
> Hi Suka,
>
> A few more things ...
>
> Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> writes:
>
> > diff --git a/arch/powerpc/platforms/powernv/copy-paste.h b/arch/powerpc/platforms/powernv/copy-paste.h
> > new file mode 100644
> > index 0000000..7783bb8
> > --- /dev/null
> > +++ b/arch/powerpc/platforms/powernv/copy-paste.h
> > @@ -0,0 +1,74 @@
> > +/*
> > + * Copyright 2016 IBM Corp.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + */
> > +
> > +/*
> > + * Macros taken from tools/testing/selftests/powerpc/context_switch/cp_abort.c
> > + */
>
> These are both out of date, they're changed in v3.0B.
>
> > +#define PASTE(RA, RB, L, RC) \
> > + .long (0x7c00070c | (RA) << (31-15) | (RB) << (31-20) \
> > + | (L) << (31-10) | (RC) << (31-31))
>
> You should define PPC_PASTE() in ppc-opcode.h
>
> We already have PPC_INST_PASTE, so use that.
>
> L and RC are gone.

Ok. I thought they would come back later, but of course we can update
these kernel-only calls then.

>
> > +
> > +#define COPY(RA, RB, L) \
> > + .long (0x7c00060c | (RA) << (31-15) | (RB) << (31-20) \
> > + | (L) << (31-10))
>
> Use PPC_COPY().
>

Ok

> > +
> > +#define CR0_FXM "0x80"
>
> I don't think a #define for this helps readability.
>
> > +#define CR0_SHIFT 28
> > +#define CR0_MASK 0xF
>
> Not used.

Will need them now to return value in cr0?
>
> > +/*
> > + * Copy/paste instructions:
> > + *
> > + * copy RA,RB,L
> > + * Copy contents of address (RA) + effective_address(RB)
> > + * to internal copy-buffer.
> > + *
> > + * L == 1 indicates this is the first copy.
> > + *
> > + * L == 0 indicates its a continuation of a prior first copy.
> > + *
> > + * paste RA,RB,L
> > + * Paste contents of internal copy-buffer to the address
> > + * (RA) + effective_address(RB)
> > + *
> > + * L == 0 indicates its a continuation of a prior paste. i.e.
> > + * don't wait for the completion or update status.
> > + *
> > + * L == 1 indicates this is the last paste in the group (i.e.
> > + * wait for the group to complete and update status in CR0).
> > + *
> > + * For Power9, the L bit must be 'true' in both copy and paste.
> > + */
> > +
> > +static inline int vas_copy(void *crb, int offset, int first)
> > +{
> > + WARN_ON_ONCE(!first);
>
> Please change the API to not require unused parameters.
>
> Same for offset.

Ok, Haren's NX patches will need to drop those parameters as well.

>
> > +
> > + __asm__ __volatile(stringify_in_c(COPY(%0, %1, %2))";"
>
> I've never seen __volatile before.
>
> Just use: asm volatile

ok
>
>
> > + :
> > + : "b" (offset), "b" (crb), "i" (1)
> > + : "memory");
> > +
> > + return 0;
> > +}
> > +
> > +static inline int vas_paste(void *paste_address, int offset, int last)
> > +{
> > + unsigned long long cr;
>
> cr is 32-bits actually.

ok
>
> > + WARN_ON_ONCE(!last);
> > +
> > + cr = 0;
> > + __asm__ __volatile(stringify_in_c(PASTE(%1, %2, 1, 1))";"
> > + "mfocrf %0," CR0_FXM ";"
> > + : "=r" (cr)
> > + : "b" (paste_address), "b" (offset)
> > + : "memory");
>
> You need cr0 in the clobbers.

ok
>
> > +
> > + return cr;
>
> I think it would be more natural if you just returned CR0, so if you did
> shift and mask with the CR0 constants you have above.
>
ok

>
> > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
> > index 70762c3..73081b4 100644
> > --- a/arch/powerpc/platforms/powernv/vas-window.c
> > +++ b/arch/powerpc/platforms/powernv/vas-window.c
> > @@ -1040,6 +1041,57 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
> > }
> > EXPORT_SYMBOL_GPL(vas_tx_win_open);
> >
> > +int vas_copy_crb(void *crb, int offset, bool first)
> > +{
> > + if (!vas_initialized())
> > + return -1;
> > +
> > + return vas_copy(crb, offset, first);
> > +}
> > +EXPORT_SYMBOL_GPL(vas_copy_crb);
> > +
> > +#define RMA_LSMP_REPORT_ENABLE PPC_BIT(53)
> > +int vas_paste_crb(struct vas_window *txwin, int offset, bool last, bool re)
> > +{
> > + int rc;
> > + uint64_t val;
> > + void *addr;
> > +
> > + if (!vas_initialized())
> > + return -1;
>
> This is in the fast path, or at least the runtime path. So I don't think
> these checks are wanted, how would we have got this far if vas wasn't
> initialised?

Yes, I have dropped vas_initialized() now.
>
>
>
> cheers

\
 
 \ /
  Last update: 2017-08-28 07:21    [W:0.061 / U:2.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site