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

[Paul Mackerras]

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 & 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.

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.