On Wed, Sep 2, 2020 at 4:18 PM Paul Mackerras <paulus@xxxxxxxxxx> 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@xxxxxxxxx/)
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.
