On Wed, Sep 02, 2020 at 06:00:24PM +1000, Jordan Niethe wrote:
> 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/)
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.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.
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.
