Hello Gustavo, comments inline:
On Tue, 2020-02-18 at 16:13 -0500, Gustavo Romero wrote:
<SNIP>
> diff --git a/arch/powerpc/kvm/book3s_hv_tm.c b/arch/powerpc/kvm/book3s_hv_tm.c
> index 0db937497169..cc90b8b82329 100644
> --- a/arch/powerpc/kvm/book3s_hv_tm.c
> +++ b/arch/powerpc/kvm/book3s_hv_tm.c
> @@ -3,6 +3,8 @@
> * Copyright 2017 Paul Mackerras, IBM Corp. <paulus@xxxxxxxxxxx>
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
Could not see where is this used.
> #include <linux/kvm_host.h>
>
> #include <asm/kvm_ppc.h>
> @@ -44,7 +46,18 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
> u64 newmsr, bescr;
> int ra, rs;
>
> - switch (instr & 0xfc0007ff) {
> + /*
> + * rfid, rfebb, and mtmsrd encode bit 31 = 0 since it's a reserved bit
> + * in these instructions, so masking bit 31 out doesn't change these
> + * instructions. For treclaim., tsr., and trechkpt. instructions if bit
> + * 31 = 0 then they are per ISA invalid forms, however P9 UM, in section
> + * 4.6.10 Book II Invalid Forms, informs specifically that ignoring bit
> + * 31 is an acceptable way to handle these invalid forms that have
> + * bit 31 = 0. Moreover, for emulation purposes both forms (w/ and wo/
> + * bit 31 set) can generate a softpatch interrupt. Hence both forms
> + * are handled below for these instructions so they behave the same way.
> + */
> + switch (instr & PO_XOP_OPCODE_MASK) {
>
<SNIP>
> - case PPC_INST_TRECHKPT:
> + /* ignore bit 31, see comment above */
> + case (PPC_INST_TRECHKPT & PO_XOP_OPCODE_MASK):
> /* XXX do we need to check for PR=0 here? */
> /* check for TM disabled in the HFSCR or MSR */
> if (!(vcpu->arch.hfscr & HFSCR_TM)) {
> @@ -208,6 +224,8 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
> }
>
Seems good, using the same flag to mask out bit 31 of these macros.
They are used only in a few places, and I think removing the macro bit
would be ok, but I think your way is better to keep it documented.
I just noticed that there is a similar function that uses PPC_INST_TSR:
kvmhv_p9_tm_emulation_early @ arch/powerpc/kvm/book3s_hv_tm_builtin.c.
Wouldn't it need to be changed as well?
> /* What should we do here? We didn't recognize the instruction */
> - WARN_ON_ONCE(1);
> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> + pr_warn_ratelimited("Unrecognized TM-related instruction %#x for emulation", instr);
> +
> return RESUME_GUEST;
> }
I suppose this is the right thing to do, but I think it would be better
to give this change it's own patch.
What do you think?
Best regards,
Leonardo Bras
Attachment:
signature.asc
Description: This is a digitally signed message part
