On 03.07.2013, at 20:28, Scott Wood wrote:
> On 07/03/2013 10:13:57 AM, Alexander Graf wrote:
>> On 03.07.2013, at 15:53, Caraman Mihai Claudiu-B02008 wrote:
>> >>> -#ifdef CONFIG_SPE
>> >>> case BOOKE_INTERRUPT_SPE_ALTIVEC_UNAVAIL: {
>> >>> - if (vcpu->arch.shared->msr & MSR_SPE)
>> >>> - kvmppc_vcpu_enable_spe(vcpu);
>> >>> - else
>> >>> - kvmppc_booke_queue_irqprio(vcpu,
>> >>> -
>> >> BOOKE_IRQPRIO_SPE_ALTIVEC_UNAVAIL);
>> >>> + if (kvmppc_supports_spe()) {
>> >>> + bool enabled = false;
>> >>> +
>> >>> +#ifndef CONFIG_KVM_BOOKE_HV
>> >>> + if (vcpu->arch.shared->msr & MSR_SPE) {
>> >>> + kvmppc_vcpu_enable_spe(vcpu);
>> >>> + enabled = true;
>> >>> + }
>> >>> +#endif
>> >>
>> >> Why the #ifdef? On HV capable systems kvmppc_supports_spe() will just
>> >> always return false.
>> >
>> > AltiVec and SPE unavailable exceptions follows the same path. While
>> > kvmppc_supports_spe() will always return false kvmppc_supports_altivec()
>> > may not.
>> There is no chip that supports SPE and HV at the same time. So we'll never
>> hit this anyway, since kvmppc_supports_spe() always 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. More
> seriously, MSR_SPE is the same as MSR_VEC, so we shouldn't interpret it as
> SPE unless CONFIG_SPE is defined. And we can't rely on the "if
> (kvmppc_supports_spe())" here because a later patch changes it to "if
> (kvmppc_supports_altivec() || kvmppc_supports_spe())". So I think we still
> need the ifdef CONFIG_SPE here.
>
> As for the HV ifndef, we should try not to confuse HV/PR with e500mc/e500v2,
> even if we happen to only run HV on e500mc and PR on e500v2. We would not
> want to call kvmppc_vcpu_enable_spe() here on a hypothetical HV target with
> SPE. And we *would* want to call kvmppc_vcpu_enable_fp() here on a
> hypothetical PR target with normal FP. It's one thing to leave out the
> latter, since it would involve writing actual code that we have no way to
> test at this point, but quite another to leave out the proper conditions for
> when we want to run code that we do have.
So we should make this an #ifdef CONFIG_SPE rather than #ifndef
CONFIG_KVM_BOOKE_HV?
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
