* Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
@ 2012-01-09 14:15 ` Alexander Graf
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-01-09 14:15 UTC (permalink / raw)
To: Liu Yu; +Cc: kvm-ppc, kvm, galak, scottwood, timur
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@freescale.com>
> ---
> 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.
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.
Alex
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
2012-01-09 14:15 ` Alexander Graf
@ 2012-01-09 16:04 ` Yoder Stuart-B08248
-1 siblings, 0 replies; 18+ messages in thread
From: Yoder Stuart-B08248 @ 2012-01-09 16:04 UTC (permalink / raw)
To: Alexander Graf, Liu Yu-B13201
Cc: kvm-ppc@vger.kernel.org, kvm@vger.kernel.org,
galak@kernel.crashing.org, Wood Scott-B07421, Tabi Timur-B04825
> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of
> Alexander Graf
> Sent: Monday, January 09, 2012 8:15 AM
> To: Liu Yu-B13201
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; galak@kernel.crashing.org; 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@freescale.com>
> > ---
> > 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?
> 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?
Stuart
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
@ 2012-01-09 16:04 ` Yoder Stuart-B08248
0 siblings, 0 replies; 18+ messages in thread
From: Yoder Stuart-B08248 @ 2012-01-09 16:04 UTC (permalink / raw)
To: Alexander Graf, Liu Yu-B13201
Cc: kvm-ppc@vger.kernel.org, kvm@vger.kernel.org,
galak@kernel.crashing.org, Wood Scott-B07421, Tabi Timur-B04825
> -----Original Message-----
> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of
> Alexander Graf
> Sent: Monday, January 09, 2012 8:15 AM
> To: Liu Yu-B13201
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; galak@kernel.crashing.org; 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@freescale.com>
> > ---
> > 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?
> 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?
Stuart
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
2012-01-09 16:04 ` Yoder Stuart-B08248
@ 2012-01-09 16:07 ` Alexander Graf
-1 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-01-09 16:07 UTC (permalink / raw)
To: Yoder Stuart-B08248
Cc: Liu Yu-B13201, kvm-ppc@vger.kernel.org, kvm@vger.kernel.org,
galak@kernel.crashing.org, Wood Scott-B07421, Tabi Timur-B04825
On 09.01.2012, at 17:04, Yoder Stuart-B08248 wrote:
>
>
>> -----Original Message-----
>> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of
>> Alexander Graf
>> Sent: Monday, January 09, 2012 8:15 AM
>> To: Liu Yu-B13201
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; galak@kernel.crashing.org; 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@freescale.com>
>>> ---
>>> 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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
@ 2012-01-09 16:07 ` Alexander Graf
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-01-09 16:07 UTC (permalink / raw)
To: Yoder Stuart-B08248
Cc: Liu Yu-B13201, kvm-ppc@vger.kernel.org, kvm@vger.kernel.org,
galak@kernel.crashing.org, Wood Scott-B07421, Tabi Timur-B04825
On 09.01.2012, at 17:04, Yoder Stuart-B08248 wrote:
>
>
>> -----Original Message-----
>> From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-owner@vger.kernel.org] On Behalf Of
>> Alexander Graf
>> Sent: Monday, January 09, 2012 8:15 AM
>> To: Liu Yu-B13201
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; galak@kernel.crashing.org; 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@freescale.com>
>>> ---
>>> 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
^ permalink raw reply [flat|nested] 18+ messages in thread* RE: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
2012-01-09 16:07 ` Alexander Graf
@ 2012-01-09 22:41 ` Yoder Stuart-B08248
-1 siblings, 0 replies; 18+ messages in thread
From: Yoder Stuart-B08248 @ 2012-01-09 22:41 UTC (permalink / raw)
To: Alexander Graf
Cc: Liu Yu-B13201, kvm-ppc@vger.kernel.org, kvm@vger.kernel.org,
galak@kernel.crashing.org, Wood Scott-B07421, Tabi Timur-B04825
> >> 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.
What do you mean here, could you elaborate?
Stuart
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
@ 2012-01-09 22:41 ` Yoder Stuart-B08248
0 siblings, 0 replies; 18+ messages in thread
From: Yoder Stuart-B08248 @ 2012-01-09 22:41 UTC (permalink / raw)
To: Alexander Graf
Cc: Liu Yu-B13201, kvm-ppc@vger.kernel.org, kvm@vger.kernel.org,
galak@kernel.crashing.org, Wood Scott-B07421, Tabi Timur-B04825
> >> 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.
What do you mean here, could you elaborate?
Stuart
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
2012-01-09 22:41 ` Yoder Stuart-B08248
@ 2012-01-09 22:51 ` Alexander Graf
-1 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-01-09 22:51 UTC (permalink / raw)
To: Yoder Stuart-B08248
Cc: Liu Yu-B13201, kvm-ppc@vger.kernel.org, kvm@vger.kernel.org,
galak@kernel.crashing.org, Wood Scott-B07421, Tabi Timur-B04825
On 09.01.2012, at 23:41, Yoder Stuart-B08248 wrote:
>
>>>> 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.
>
> What do you mean here, could you elaborate?
>
> +#ifdef CONFIG_KVM_GUEST
> +/*
> + * r3 contains the pointer to in[8]
> + * r4 contains the pointer to out[8]
> + * r5 contains the hcall vendor and nr
> + * r6 contains the handler which send hcall
> + */
> +_GLOBAL(e500_ev_idle)
entering with interrupts disabled
> + rlwinm r7,r1,0,0,31-THREAD_SHIFT /* current thread_info */
> + lwz r8,TI_LOCAL_FLAGS(r7) /* set napping bit */
> + ori r8,r8,_TLF_NAPPING /* so when we take an exception */
> + stw r8,TI_LOCAL_FLAGS(r7) /* it will return to our caller */
> + wrteei 1
enable interrupts
> + mtctr r6
> + bctr
call hypercall, race occurs, return to caller with interrupts enabled
Alex
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
@ 2012-01-09 22:51 ` Alexander Graf
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-01-09 22:51 UTC (permalink / raw)
To: Yoder Stuart-B08248
Cc: Liu Yu-B13201, kvm-ppc@vger.kernel.org, kvm@vger.kernel.org,
galak@kernel.crashing.org, Wood Scott-B07421, Tabi Timur-B04825
On 09.01.2012, at 23:41, Yoder Stuart-B08248 wrote:
>
>>>> 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.
>
> What do you mean here, could you elaborate?
>
> +#ifdef CONFIG_KVM_GUEST
> +/*
> + * r3 contains the pointer to in[8]
> + * r4 contains the pointer to out[8]
> + * r5 contains the hcall vendor and nr
> + * r6 contains the handler which send hcall
> + */
> +_GLOBAL(e500_ev_idle)
entering with interrupts disabled
> + rlwinm r7,r1,0,0,31-THREAD_SHIFT /* current thread_info */
> + lwz r8,TI_LOCAL_FLAGS(r7) /* set napping bit */
> + ori r8,r8,_TLF_NAPPING /* so when we take an exception */
> + stw r8,TI_LOCAL_FLAGS(r7) /* it will return to our caller */
> + wrteei 1
enable interrupts
> + mtctr r6
> + bctr
call hypercall, race occurs, return to caller with interrupts enabled
Alex
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
2012-01-09 14:15 ` Alexander Graf
@ 2012-01-09 20:00 ` Scott Wood
-1 siblings, 0 replies; 18+ messages in thread
From: Scott Wood @ 2012-01-09 20:00 UTC (permalink / raw)
To: Alexander Graf; +Cc: Liu Yu, kvm-ppc, kvm, galak, timur, Stuart Yoder
On 01/09/2012 08:15 AM, Alexander Graf wrote:
>
> On 05.01.2012, at 10:07, Liu Yu wrote:
>
>> 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.
The ePAPR description of this hcall is a little vague (Stuart, put on
list to fix in next ePAPR revision?), but this is expected. It will
also be the case if a guest directly uses the wait instruction. Guests
must be able to deal with spurious wakeups.
-Scott
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
@ 2012-01-09 20:00 ` Scott Wood
0 siblings, 0 replies; 18+ messages in thread
From: Scott Wood @ 2012-01-09 20:00 UTC (permalink / raw)
To: Alexander Graf; +Cc: Liu Yu, kvm-ppc, kvm, galak, timur, Stuart Yoder
On 01/09/2012 08:15 AM, Alexander Graf wrote:
>
> On 05.01.2012, at 10:07, Liu Yu wrote:
>
>> 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.
The ePAPR description of this hcall is a little vague (Stuart, put on
list to fix in next ePAPR revision?), but this is expected. It will
also be the case if a guest directly uses the wait instruction. Guests
must be able to deal with spurious wakeups.
-Scott
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
2012-01-09 20:00 ` Scott Wood
@ 2012-01-09 20:18 ` Alexander Graf
-1 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-01-09 20:18 UTC (permalink / raw)
To: Scott Wood
Cc: Liu Yu, <kvm-ppc@vger.kernel.org>,
<kvm@vger.kernel.org>, <galak@kernel.crashing.org>,
<timur@freescale.com>, Stuart Yoder
Am 09.01.2012 um 21:00 schrieb Scott Wood <scottwood@freescale.com>:
> On 01/09/2012 08:15 AM, Alexander Graf wrote:
>>
>> On 05.01.2012, at 10:07, Liu Yu wrote:
>>
>>> 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.
>
> The ePAPR description of this hcall is a little vague (Stuart, put on
> list to fix in next ePAPR revision?), but this is expected. It will
> also be the case if a guest directly uses the wait instruction. Guests
> must be able to deal with spurious wakeups.
The wait instruction does get executed in an infinite loop though, while this hcall is only executed once.
Alex
>
> -Scott
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
@ 2012-01-09 20:18 ` Alexander Graf
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2012-01-09 20:18 UTC (permalink / raw)
To: Scott Wood
Cc: Liu Yu, <kvm-ppc@vger.kernel.org>,
<kvm@vger.kernel.org>, <galak@kernel.crashing.org>,
<timur@freescale.com>, Stuart Yoder
Am 09.01.2012 um 21:00 schrieb Scott Wood <scottwood@freescale.com>:
> On 01/09/2012 08:15 AM, Alexander Graf wrote:
>>
>> On 05.01.2012, at 10:07, Liu Yu wrote:
>>
>>> 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.
>
> The ePAPR description of this hcall is a little vague (Stuart, put on
> list to fix in next ePAPR revision?), but this is expected. It will
> also be the case if a guest directly uses the wait instruction. Guests
> must be able to deal with spurious wakeups.
The wait instruction does get executed in an infinite loop though, while this hcall is only executed once.
Alex
>
> -Scott
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
2012-01-09 20:18 ` Alexander Graf
@ 2012-01-09 22:33 ` Scott Wood
-1 siblings, 0 replies; 18+ messages in thread
From: Scott Wood @ 2012-01-09 22:33 UTC (permalink / raw)
To: Alexander Graf
Cc: Liu Yu, <kvm-ppc@vger.kernel.org>,
<kvm@vger.kernel.org>, <galak@kernel.crashing.org>,
<timur@freescale.com>, Stuart Yoder
On 01/09/2012 02:18 PM, Alexander Graf wrote:
>
>
> Am 09.01.2012 um 21:00 schrieb Scott Wood <scottwood@freescale.com>:
>
>> On 01/09/2012 08:15 AM, Alexander Graf wrote:
>>>
>>> On 05.01.2012, at 10:07, Liu Yu wrote:
>>>
>>>> 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.
>>
>> The ePAPR description of this hcall is a little vague (Stuart, put on
>> list to fix in next ePAPR revision?), but this is expected. It will
>> also be the case if a guest directly uses the wait instruction. Guests
>> must be able to deal with spurious wakeups.
>
> The wait instruction does get executed in an infinite loop though, while this hcall is only executed once.
Yes, I pointed that out as something that needs to be fixed in the
ev_idle patch.
-Scott
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] KVM: PPC: epapr: Add idle hcall support for host
@ 2012-01-09 22:33 ` Scott Wood
0 siblings, 0 replies; 18+ messages in thread
From: Scott Wood @ 2012-01-09 22:33 UTC (permalink / raw)
To: Alexander Graf
Cc: Liu Yu, <kvm-ppc@vger.kernel.org>,
<kvm@vger.kernel.org>, <galak@kernel.crashing.org>,
<timur@freescale.com>, Stuart Yoder
On 01/09/2012 02:18 PM, Alexander Graf wrote:
>
>
> Am 09.01.2012 um 21:00 schrieb Scott Wood <scottwood@freescale.com>:
>
>> On 01/09/2012 08:15 AM, Alexander Graf wrote:
>>>
>>> On 05.01.2012, at 10:07, Liu Yu wrote:
>>>
>>>> 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.
>>
>> The ePAPR description of this hcall is a little vague (Stuart, put on
>> list to fix in next ePAPR revision?), but this is expected. It will
>> also be the case if a guest directly uses the wait instruction. Guests
>> must be able to deal with spurious wakeups.
>
> The wait instruction does get executed in an infinite loop though, while this hcall is only executed once.
Yes, I pointed that out as something that needs to be fixed in the
ev_idle patch.
-Scott
^ permalink raw reply [flat|nested] 18+ messages in thread