[RFC PATCH 1/2] KVM: PPC: Use the ppc_inst type

[Jordan Niethe]

On Wed, Sep 2, 2020 at 4:18 PM Paul Mackerras <paulus at ozlabs.org> wrote:
>
> On Thu, Aug 20, 2020 at 01:39:21PM +1000, Jordan Niethe wrote:
> > The ppc_inst type was added to help cope with the addition of prefixed
> > instructions to the ISA. Convert KVM to use this new type for dealing
> > wiht instructions. For now do not try to add further support for
> > prefixed instructions.
>
> This change does seem to splatter itself across a lot of code that
> mostly or exclusively runs on machines which are not POWER10 and will
> never need to handle prefixed instructions, unfortunately.  I wonder
> if there is a less invasive way to approach this.
Something less invasive would be good.
>
> In particular we are inflicting this 64-bit struct on 32-bit platforms
> unnecessarily (I assume, correct me if I am wrong here).
No, that is something that I wanted to to avoid, on 32 bit platforms
it is a 32bit struct:

struct ppc_inst {
        u32 val;
#ifdef CONFIG_PPC64
        u32 suffix;
#endif
} __packed;
>
> How would it be to do something like:
>
> typedef unsigned long ppc_inst_t;
>
> so it is 32 bits on 32-bit platforms and 64 bits on 64-bit platforms,
> and then use that instead of 'struct ppc_inst'?  You would still need
> to change the function declarations but I think most of the function
> bodies would not need to be changed.  In particular you would avoid a
> lot of the churn related to having to add ppc_inst_val() and suchlike.

Would the idea be to get rid of `struct ppc_inst` entirely or just not
use it in kvm?
In an earlier series I did something similar (at least code shared
between 32bit and 64bit would need helpers, but 32bit only code need
not change):

#ifdef __powerpc64__

typedef struct ppc_inst {
    union {
        struct {
            u32 word;
            u32 pad;
        } __packed;
        struct {
            u32 prefix;
            u32 suffix;
        } __packed;
    };
} ppc_inst;

#else /* !__powerpc64__ */

typedef u32 ppc_inst;
#endif

However mpe wanted to avoid using a typedef
(https://patchwork.ozlabs.org/comment/2391845/)

We did also talk about just using a u64 for instructions
(https://lore.kernel.org/linuxppc-dev/1585028462.t27rstc2uf.astroid@bobo.none/)
but the concern was that as prefixed instructions act as two separate
u32s (prefix is always before the suffix regardless of endianess)
keeping it as a u64 would lead to lot of macros and potential
confusion.
But it does seem if that can avoid a lot of needless churn it might
worth the trade off.
>
> > -static inline unsigned make_dsisr(unsigned instr)
> > +static inline unsigned make_dsisr(struct ppc_inst instr)
> >  {
> >       unsigned dsisr;
> > +     u32 word = ppc_inst_val(instr);
> >
> >
> >       /* bits  6:15 --> 22:31 */
> > -     dsisr = (instr & 0x03ff0000) >> 16;
> > +     dsisr = (word & 0x03ff0000) >> 16;
> >
> >       if (IS_XFORM(instr)) {
> >               /* bits 29:30 --> 15:16 */
> > -             dsisr |= (instr & 0x00000006) << 14;
> > +             dsisr |= (word & 0x00000006) << 14;
> >               /* bit     25 -->    17 */
> > -             dsisr |= (instr & 0x00000040) << 8;
> > +             dsisr |= (word & 0x00000040) << 8;
> >               /* bits 21:24 --> 18:21 */
> > -             dsisr |= (instr & 0x00000780) << 3;
> > +             dsisr |= (word & 0x00000780) << 3;
> >       } else {
> >               /* bit      5 -->    17 */
> > -             dsisr |= (instr & 0x04000000) >> 12;
> > +             dsisr |= (word & 0x04000000) >> 12;
> >               /* bits  1: 4 --> 18:21 */
> > -             dsisr |= (instr & 0x78000000) >> 17;
> > +             dsisr |= (word & 0x78000000) >> 17;
> >               /* bits 30:31 --> 12:13 */
> >               if (IS_DSFORM(instr))
> > -                     dsisr |= (instr & 0x00000003) << 18;
> > +                     dsisr |= (word & 0x00000003) << 18;
>
> Here I would have done something like:
>
> > -static inline unsigned make_dsisr(unsigned instr)
> > +static inline unsigned make_dsisr(struct ppc_inst pi)
> >  {
> >       unsigned dsisr;
> > +     u32 instr = ppc_inst_val(pi);
>
> and left the rest of the function unchanged.
That is better.
>
> At first I wondered why we still had that function, since IBM Power
> CPUs have not set DSISR on an alignment interrupt since POWER3 days.
> It turns out it this function is used by PR KVM when it is emulating
> one of the old 32-bit PowerPC CPUs (601, 603, 604, 750, 7450 etc.).
>
> > diff --git a/arch/powerpc/kernel/kvm.c b/arch/powerpc/kernel/kvm.c
>
> Despite the file name, this code is not used on IBM Power servers.
> It is for platforms which run under an ePAPR (not server PAPR)
> hypervisor (which would be a KVM variant, but generally book E KVM not
> book 3S).
>
> Paul.