Re: [RFC PATCH 1/2] KVM: PPC: Use the ppc_inst type
On Wed, Sep 02, 2020 at 06:00:24PM +1000, Jordan Niethe wrote:
> On Wed, Sep 2, 2020 at 4:18 PM Paul Mackerras 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/)
Well it doesn't have to be typedef'd, it could just be "unsigned
long", which is used in other places for things that want to be 32-bit
on 32-bit machines and 64-bit on 64-bit machines.
I do however think that it should be a numeric type so that we can
mask, shift and compare it more easily. I know that's less "abstract"
but it's also a lot less obfuscated and I think that will lead to
clearer code. If you got the opposite advice from Michael Ellerman or
Nick Piggin then I will discuss it with them.
> We did also talk about just using a u64 for instructions
> (https://lore.kernel.org/linuxppc-dev/1585028462.t27rstc2uf.astr...@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.
u32 *ip;
instr = *ip++;
if (is_prefix(instr) && is_suitably_aligned(ip))
instr = (instr << 32) | *ip++;
would avoid the endian issues pretty cleanly I think. In other words
the prefix would always be the high half of the 64-bit value, so you
can't just do a single 64-bit of the instruction on little-endian
platforms; but you can't do a single 64-bit load for other reasons as
well, such as alignment.
Paul.
Re: [RFC PATCH 1/2] KVM: PPC: Use the ppc_inst type
On Wed, Sep 2, 2020 at 4:18 PM Paul Mackerras 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.astr...@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 & 0x03ff) >> 16;
> > + dsisr = (word & 0x03ff) >> 16;
> >
> > if (IS_XFORM(instr)) {
> > /* bits 29:30 --> 15:16 */
> > - dsisr |= (instr & 0x0006) << 14;
> > + dsisr |= (word & 0x0006) << 14;
> > /* bit 25 -->17 */
> > - dsisr |= (instr & 0x0040) << 8;
> > + dsisr |= (word & 0x0040) << 8;
> > /* bits 21:24 --> 18:21 */
> > - dsisr |= (instr & 0x0780) << 3;
> > + dsisr |= (word & 0x0780) << 3;
> > } else {
> > /* bit 5 -->17 */
> > - dsisr |= (instr & 0x0400) >> 12;
> > + dsisr |= (word & 0x0400) >> 12;
> > /* bits 1: 4 --> 18:21 */
> > - dsisr |= (instr & 0x7800) >> 17;
> > + dsisr |= (word & 0x7800) >> 17;
> > /* bits 30:31 --> 12:13 */
> > if (IS_DSFORM(instr))
> > - dsisr |= (instr & 0x0003) << 18;
> > + dsisr |= (word & 0x0003) << 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.
Re: [RFC PATCH 1/2] KVM: PPC: Use the ppc_inst type
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.
In particular we are inflicting this 64-bit struct on 32-bit platforms
unnecessarily (I assume, correct me if I am wrong here).
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.
> -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 & 0x03ff) >> 16;
> + dsisr = (word & 0x03ff) >> 16;
>
> if (IS_XFORM(instr)) {
> /* bits 29:30 --> 15:16 */
> - dsisr |= (instr & 0x0006) << 14;
> + dsisr |= (word & 0x0006) << 14;
> /* bit 25 -->17 */
> - dsisr |= (instr & 0x0040) << 8;
> + dsisr |= (word & 0x0040) << 8;
> /* bits 21:24 --> 18:21 */
> - dsisr |= (instr & 0x0780) << 3;
> + dsisr |= (word & 0x0780) << 3;
> } else {
> /* bit 5 -->17 */
> - dsisr |= (instr & 0x0400) >> 12;
> + dsisr |= (word & 0x0400) >> 12;
> /* bits 1: 4 --> 18:21 */
> - dsisr |= (instr & 0x7800) >> 17;
> + dsisr |= (word & 0x7800) >> 17;
> /* bits 30:31 --> 12:13 */
> if (IS_DSFORM(instr))
> - dsisr |= (instr & 0x0003) << 18;
> + dsisr |= (word & 0x0003) << 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.
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.
[RFC PATCH 1/2] KVM: PPC: Use the ppc_inst type
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.
Signed-off-by: Jordan Niethe
---
arch/powerpc/include/asm/disassemble.h| 80 +-
arch/powerpc/include/asm/kvm_book3s.h | 4 +-
arch/powerpc/include/asm/kvm_book3s_asm.h | 3 +-
arch/powerpc/include/asm/kvm_host.h | 5 +-
arch/powerpc/include/asm/kvm_ppc.h| 14 ++--
arch/powerpc/kernel/asm-offsets.c | 2 +
arch/powerpc/kernel/kvm.c | 99 +++
arch/powerpc/kvm/book3s.c | 6 +-
arch/powerpc/kvm/book3s.h | 2 +-
arch/powerpc/kvm/book3s_64_mmu_hv.c | 8 +-
arch/powerpc/kvm/book3s_emulate.c | 30 +++
arch/powerpc/kvm/book3s_hv.c | 19 ++---
arch/powerpc/kvm/book3s_hv_nested.c | 4 +-
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 5 ++
arch/powerpc/kvm/book3s_hv_tm.c | 17 ++--
arch/powerpc/kvm/book3s_hv_tm_builtin.c | 12 +--
arch/powerpc/kvm/book3s_paired_singles.c | 15 ++--
arch/powerpc/kvm/book3s_pr.c | 20 ++---
arch/powerpc/kvm/booke.c | 18 ++---
arch/powerpc/kvm/booke.h | 4 +-
arch/powerpc/kvm/booke_emulate.c | 4 +-
arch/powerpc/kvm/e500_emulate.c | 6 +-
arch/powerpc/kvm/e500_mmu_host.c | 6 +-
arch/powerpc/kvm/emulate.c| 15 ++--
arch/powerpc/kvm/emulate_loadstore.c | 8 +-
arch/powerpc/kvm/powerpc.c| 4 +-
arch/powerpc/kvm/trace.h | 9 ++-
arch/powerpc/kvm/trace_booke.h| 8 +-
arch/powerpc/kvm/trace_pr.h | 8 +-
arch/powerpc/lib/inst.c | 4 +-
arch/powerpc/lib/sstep.c | 4 +-
arch/powerpc/sysdev/fsl_pci.c | 4 +-
32 files changed, 237 insertions(+), 210 deletions(-)
diff --git a/arch/powerpc/include/asm/disassemble.h
b/arch/powerpc/include/asm/disassemble.h
index 8d2ebc36d5e3..91dbe8e5cd13 100644
--- a/arch/powerpc/include/asm/disassemble.h
+++ b/arch/powerpc/include/asm/disassemble.h
@@ -10,75 +10,82 @@
#define __ASM_PPC_DISASSEMBLE_H__
#include
+#include
-static inline unsigned int get_op(u32 inst)
+static inline unsigned int get_op(struct ppc_inst inst)
{
- return inst >> 26;
+ return ppc_inst_val(inst) >> 26;
}
-static inline unsigned int get_xop(u32 inst)
+static inline unsigned int get_xop(struct ppc_inst inst)
{
- return (inst >> 1) & 0x3ff;
+ return (ppc_inst_val(inst) >> 1) & 0x3ff;
}
-static inline unsigned int get_sprn(u32 inst)
+static inline unsigned int get_sprn(struct ppc_inst inst)
{
- return ((inst >> 16) & 0x1f) | ((inst >> 6) & 0x3e0);
+ u32 word = ppc_inst_val(inst);
+
+ return ((word >> 16) & 0x1f) | ((word >> 6) & 0x3e0);
}
-static inline unsigned int get_dcrn(u32 inst)
+static inline unsigned int get_dcrn(struct ppc_inst inst)
{
- return ((inst >> 16) & 0x1f) | ((inst >> 6) & 0x3e0);
+ u32 word = ppc_inst_val(inst);
+
+ return ((word >> 16) & 0x1f) | ((word >> 6) & 0x3e0);
}
-static inline unsigned int get_tmrn(u32 inst)
+static inline unsigned int get_tmrn(struct ppc_inst inst)
{
- return ((inst >> 16) & 0x1f) | ((inst >> 6) & 0x3e0);
+ u32 word = ppc_inst_val(inst);
+
+ return ((word >> 16) & 0x1f) | ((word >> 6) & 0x3e0);
}
-static inline unsigned int get_rt(u32 inst)
+static inline unsigned int get_rt(struct ppc_inst inst)
{
- return (inst >> 21) & 0x1f;
+ return (ppc_inst_val(inst) >> 21) & 0x1f;
}
-static inline unsigned int get_rs(u32 inst)
+static inline unsigned int get_rs(struct ppc_inst inst)
{
- return (inst >> 21) & 0x1f;
+ return (ppc_inst_val(inst) >> 21) & 0x1f;
}
-static inline unsigned int get_ra(u32 inst)
+static inline unsigned int get_ra(struct ppc_inst inst)
{
- return (inst >> 16) & 0x1f;
+ return (ppc_inst_val(inst) >> 16) & 0x1f;
}
-static inline unsigned int get_rb(u32 inst)
+static inline unsigned int get_rb(struct ppc_inst inst)
{
- return (inst >> 11) & 0x1f;
+ return (ppc_inst_val(inst) >> 11) & 0x1f;
}
-static inline unsigned int get_rc(u32 inst)
+static inline unsigned int get_rc(struct ppc_inst inst)
{
- return inst & 0x1;
+ return ppc_inst_val(inst) & 0x1;
}
-static inline unsigned int get_ws(u32 inst)
+static inline unsigned int get_ws(struct ppc_inst inst)
{
- return (inst >> 11) & 0x1f;
+ return (ppc_inst_val(inst) >> 11) & 0x1f;
}
-static inline unsigned int get_d(u32 inst)
+static inline unsigned int get_d(struct ppc_inst inst)
{
- return inst & 0x;
+ return ppc_inst_val(inst) & 0x;
}
-static inline unsigned int get_oc(u32 inst)
+static inline unsigned int get_oc(struct ppc_inst inst)
{
- return (inst >>
