[2/2] KVM: PPC: booke: Add watchdog emulation

> > +
> > +/*
> > + * Return the number of jiffies until the next timeout.  If the
> > +timeout is
> > + * longer than the MAX_TIMEOUT, that we return MAX_TIMEOUT instead.
> > + */
> > +static unsigned long watchdog_next_timeout(struct kvm_vcpu *vcpu) {
> > +	unsigned long long tb, mask, nr_jiffies = 0;
> 
> u64?
> 
> > +	u32 period = TCR_GET_FSL_WP(vcpu->arch.tcr);
> 
> Doesn't sound like something booke generic to me.

the name '*FSL*' does not look good, right?

> 
> > +#ifdef CONFIG_BOOKE
> > +	ret = ret || (v->arch.tsr & TCR_WRC_MASK);
> 
> Please make this a callback. In a header if you think it's performance critical,
> but I don't want to see #ifdef CONFIG_BOOKE too often in powerpc.c.

Not sure: do you mean something like this:


Thanks
-Bharat

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 09.07.2012, at 08:43, Bhushan Bharat-R65777 wrote:

>>> +
>>> +/*
>>> + * Return the number of jiffies until the next timeout.  If the
>>> +timeout is
>>> + * longer than the MAX_TIMEOUT, that we return MAX_TIMEOUT instead.
>>> + */
>>> +static unsigned long watchdog_next_timeout(struct kvm_vcpu *vcpu) {
>>> +	unsigned long long tb, mask, nr_jiffies = 0;
>> 
>> u64?
>> 
>>> +	u32 period = TCR_GET_FSL_WP(vcpu->arch.tcr);
>> 
>> Doesn't sound like something booke generic to me.
> 
> the name '*FSL*' does not look good, right?

It usually indicates that it's not booke generic :).

> 
>> 
>>> +#ifdef CONFIG_BOOKE
>>> +	ret = ret || (v->arch.tsr & TCR_WRC_MASK);
>> 
>> Please make this a callback. In a header if you think it's performance critical,
>> but I don't want to see #ifdef CONFIG_BOOKE too often in powerpc.c.
> 
> Not sure: do you mean something like this:
> 
> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> index f0e0c6a..7bbc6cd 100644
> --- a/arch/powerpc/include/asm/kvm_book3s.h
> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> @@ -446,6 +446,11 @@ static inline bool kvmppc_critical_section(struct kvm_vcpu *vcpu)
> }
> #endif
> 
> +static inline u32 kvmppc_get_tsr_wrc(struct kvm_vcpu *vcpu)
> +{
> +       return 0;
> +}
> +
> /* Magic register values loaded into r3 and r4 before the 'sc' assembly
>  * instruction for the OSI hypercalls */
> #define OSI_SC_MAGIC_R3                        0x113724FA
> diff --git a/arch/powerpc/include/asm/kvm_booke.h b/arch/powerpc/include/asm/kvm_booke.h
> index b7cd335..e5b86c1 100644
> --- a/arch/powerpc/include/asm/kvm_booke.h
> +++ b/arch/powerpc/include/asm/kvm_booke.h
> @@ -100,4 +100,9 @@ static inline ulong kvmppc_get_msr(struct kvm_vcpu *vcpu)
> {
>        return vcpu->arch.shared->msr;
> }
> +
> +static inline u32 kvmppc_get_tsr_wrc(struct kvm_vcpu *vcpu)
> +{
> +       return vcpu->arch.tsr & TCR_WRC_MASK;
> +}

No, I was more thinking along the lines of e500/440 here. Also it should simply be a generic callback, like

ret = kvmppc_core_check_runnable(vcpu, ret);

So that if 440 wants to do something special about modifying the runnable semantics, it can easily do so. Or we just move the whole function to core specific code.


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html