Re: [PATCH] KVM: PPC: Book3S HV: Treat TM-related invalid form instructions on P9 like the valid ones

[Gustavo Romero <gromero@linux.vnet.ibm.com>]

Hi Leonardo,

Thanks a lot for the review.

On 02/20/2020 02:51 PM, Leonardo Bras wrote:
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
> 
> Could not see where is this used.

This is used by pr_warn_ratelimited() below so the module name is printed before
the message, for instance:

[531454.670909] kvm_hv: Unrecognized TM-related instruction 0x7c00075c for emulation


>>   #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?

oh! you're right, I forgot that one. I'll send a v3.


>>   	/* 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?

I think it's sufficiently self-contained and trivial to be in the same file and
to be in a single commit.


Best regards,
Gustavo