| Message ID | 20210225134652.2127648-6-npiggin@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | KVM: PPC: Book3S: C-ify the P9 entry/exit code | expand |
Hi Nick, > void kvmppc_set_msr_hv(struct kvm_vcpu *vcpu, u64 msr) > { > + /* > + * Guest must always run with machine check interrupt > + * enabled. > + */ > + if (!(msr & MSR_ME)) > + msr |= MSR_ME; This 'if' is technically redundant but you mention a future patch warning on !(msr & MSR_ME) so I'm holding off on any judgement about the 'if' until I get to that patch :) The patch seems sane to me, I agree that we don't want guests running with MSR_ME=0 and kvmppc_set_msr_hv already ensures that the transactional state is sane so this is another sanity-enforcement in the same sort of vein. All up: Reviewed-by: Daniel Axtens <dja@axtens.net> Kind regards, Daniel > + > /* > * Check for illegal transactional state bit combination > * and if we find it, force the TS field to a safe state. > -- > 2.23.0
Excerpts from Daniel Axtens's message of February 26, 2021 4:06 pm: > Hi Nick, > >> void kvmppc_set_msr_hv(struct kvm_vcpu *vcpu, u64 msr) >> { >> + /* >> + * Guest must always run with machine check interrupt >> + * enabled. >> + */ >> + if (!(msr & MSR_ME)) >> + msr |= MSR_ME; > > This 'if' is technically redundant but you mention a future patch warning > on !(msr & MSR_ME) so I'm holding off on any judgement about the 'if' until > I get to that patch :) That's true. The warning is actually further down when we're setting up the msr to run in guest mode. At this point the MSR I think comes from qemu (and arguably the guest setup code shouldn't need to know about HV specific MSR bits) so a warning here wouldn't be appropriate. I could remove the if, although the compiler might already do that. > > The patch seems sane to me, I agree that we don't want guests running with > MSR_ME=0 and kvmppc_set_msr_hv already ensures that the transactional state is > sane so this is another sanity-enforcement in the same sort of vein. > > All up: > Reviewed-by: Daniel Axtens <dja@axtens.net> Thanks, Nick
>> This 'if' is technically redundant but you mention a future patch warning >> on !(msr & MSR_ME) so I'm holding off on any judgement about the 'if' until >> I get to that patch :) > > That's true. The warning is actually further down when we're setting up > the msr to run in guest mode. At this point the MSR I think comes from > qemu (and arguably the guest setup code shouldn't need to know about HV > specific MSR bits) so a warning here wouldn't be appropriate. > > I could remove the if, although the compiler might already do that. Yes, I think the compiler is almost certainly going to remove that. I'd probably not include the 'if' statement even though it probably gets removed by the compiler but I think that's more a matter of taste than anything else. Kind regards, Daniel > >> >> The patch seems sane to me, I agree that we don't want guests running with >> MSR_ME=0 and kvmppc_set_msr_hv already ensures that the transactional state is >> sane so this is another sanity-enforcement in the same sort of vein. >> >> All up: >> Reviewed-by: Daniel Axtens <dja@axtens.net> > > Thanks, > Nick
diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c b/arch/powerpc/kvm/book3s_hv_builtin.c index 158d309b42a3..1ca484160636 100644 --- a/arch/powerpc/kvm/book3s_hv_builtin.c +++ b/arch/powerpc/kvm/book3s_hv_builtin.c @@ -662,6 +662,13 @@ static void kvmppc_end_cede(struct kvm_vcpu *vcpu) void kvmppc_set_msr_hv(struct kvm_vcpu *vcpu, u64 msr) { + /* + * Guest must always run with machine check interrupt + * enabled. + */ + if (!(msr & MSR_ME)) + msr |= MSR_ME; + /* * Check for illegal transactional state bit combination * and if we find it, force the TS field to a safe state.