Subject: Re: [PATCH 2/6] KVM: PPC: Book3E: Refactor SPE/FP exit handling

From: Scott Wood <[email-masked]>
Date: Wed, 3 Jul 2013 13:44:33 -0500

On 07/03/2013 01:42:12 PM, Alexander Graf wrote:
>=20
> On 03.07.2013, at 20:28, Scott Wood wrote:
>=20
> > On 07/03/2013 10:13:57 AM, Alexander Graf wrote:
> >> There is no chip that supports SPE and HV at the same time. So =20
> we'll never hit this anyway, since kvmppc_supports_spe() always =20
> returns false on HV capable systems.
> >> Just add a comment saying so and remove the ifdef :).
> >
> > kvmppc_vcpu_enable_spe isn't defined unless CONFIG_SPE is defined.  =20
> More seriously, MSR_SPE is the same as MSR_VEC, so we shouldn't =20
> interpret it as SPE unless CONFIG_SPE is defined.  And we can't rely =20
> on the "if (kvmppc_supports_spe())" here because a later patch =20
> changes it to "if (kvmppc_supports_altivec() || =20
> kvmppc_supports_spe())".  So I think we still need the ifdef =20
> CONFIG_SPE here.
> >
> > As for the HV ifndef, we should try not to confuse HV/PR with =20
> e500mc/e500v2, even if we happen to only run HV on e500mc and PR on =20
> e500v2.  We would not want to call kvmppc_vcpu_enable_spe() here on a =20
> hypothetical HV target with SPE.  And we *would* want to call =20
> kvmppc_vcpu_enable_fp() here on a hypothetical PR target with normal =20
> FP.  It's one thing to leave out the latter, since it would involve =20
> writing actual code that we have no way to test at this point, but =20
> quite another to leave out the proper conditions for when we want to =20
> run code that we do have.
>=20
> So we should make this an #ifdef CONFIG_SPE rather than #ifndef =20
> CONFIG_KVM_BOOKE_HV?

I think it should be "#if !defined(CONFIG_KVM_BOOKE_HV) && =20
defined(CONFIG_SPE)" for the reasons I described in my second paragraph.

-Scott=