Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host

[Alexander Graf <agraf@xxxxxxx>]

On 09.01.2012, at 17:04, Yoder Stuart-B08248 wrote:

> 
> 
>> -----Original Message-----
>> From: kvm-ppc-owner@xxxxxxxxxxxxxxx [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On Behalf Of
>> Alexander Graf
>> Sent: Monday, January 09, 2012 8:15 AM
>> To: Liu Yu-B13201
>> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; galak@xxxxxxxxxxxxxxxxxxx; Wood Scott-
>> B07421; Tabi Timur-B04825
>> Subject: Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
>> 
>> 
>> On 05.01.2012, at 10:07, Liu Yu wrote:
>> 
>>> And add a new flag definition in kvm_ppc_pvinfo to indicate whether
>>> host support EV_IDLE hcall.
>>> 
>>> Signed-off-by: Liu Yu <yu.liu@xxxxxxxxxxxxx>
>>> ---
>>> v2:
>>> 1. instead of adding new field in kvm_ppc_pvinfo, use flags.
>>> 2. expose hcall definitions to userspace
>>> 
>>> arch/powerpc/include/asm/kvm_para.h |   14 ++++++++++++--
>>> arch/powerpc/kvm/powerpc.c          |    8 ++++++++
>>> include/linux/kvm.h                 |    2 ++
>>> 3 files changed, 22 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/arch/powerpc/include/asm/kvm_para.h
>>> b/arch/powerpc/include/asm/kvm_para.h
>>> index 50533f9..e8632b6 100644
>>> --- a/arch/powerpc/include/asm/kvm_para.h
>>> +++ b/arch/powerpc/include/asm/kvm_para.h
>>> @@ -41,9 +41,19 @@ struct kvm_vcpu_arch_shared { };
>>> 
>>> #define KVM_SC_MAGIC_R0		0x4b564d21 /* "KVM!" */
>>> -#define HC_VENDOR_KVM		(42 << 16)
>>> +
>>> +#include <asm/epapr_hcalls.h>
>>> +
>>> +/* ePAPR Hypercall Vendor ID */
>>> +#define HC_VENDOR_EPAPR		(EV_EPAPR_VENDOR_ID << 16)
>>> +#define HC_VENDOR_KVM		(EV_KVM_VENDOR_ID << 16)
>>> +
>>> +/* ePAPR Hypercall Token */
>>> +#define HC_EV_IDLE		EV_IDLE
>>> +
>>> +/* ePAPR Hypercall Return Codes */
>>> #define HC_EV_SUCCESS		0
>>> -#define HC_EV_UNIMPLEMENTED	12
>>> +#define HC_EV_UNIMPLEMENTED	EV_UNIMPLEMENTED
>>> 
>>> #define KVM_FEATURE_MAGIC_PAGE	1
>>> 
>>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>>> index c33f6a7..1242ee1 100644
>>> --- a/arch/powerpc/kvm/powerpc.c
>>> +++ b/arch/powerpc/kvm/powerpc.c
>>> @@ -81,6 +81,10 @@ int kvmppc_kvm_pv(struct kvm_vcpu *vcpu)
>>> 
>>> 		/* Second return value is in r4 */
>>> 		break;
>>> +	case HC_VENDOR_EPAPR | HC_EV_IDLE:
>>> +		r = HC_EV_SUCCESS;
>>> +		kvm_vcpu_block(vcpu);
>> 
>> Hrm. This will return on signal. So if the guest sends an idle hcall, then user space gets a
>> random signal, we'll continue executing the guest CPU, getting us out of idle even though the
>> guest didn't expect it, since the guest really wants to get an interrupt after the idle hcall.
> 
> Is that a problem though?   Won't it be just like a spurious interrupt where
> the guest would wake up, see that there is nothing to do, and then
> go idle again?   What is your concern here?

We would have changed state by enabling interrupts in the asm code. Also, it's not how hardware would work when setting MSR_WE, which if I understand Scott correctly, is the purpose of this hypercall.

> 
>> Maybe we should just set the MSR_WE bit, so we'll get back into wait state on next vcpu_run?
>> But then again that's guest visible, so I suppose a separate flag would be better.
> 
> Put a separate flag where?

In the vcpu struct. Just a bit somewhere to check in kvmppc_core_prepare_to_enter() in parallel to MSR_WE.


Alex

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