Patchwork [17/38] KVM: PPC: BookE: Add support for vcpu->mode

login
register
mail settings
Submitter Alexander Graf
Date Aug. 14, 2012, 11:04 p.m.
Message ID <1344985483-7440-18-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/177499/
State New
Headers show

Comments

Alexander Graf - Aug. 14, 2012, 11:04 p.m.
Generic KVM code might want to know whether we are inside guest context
or outside. It also wants to be able to push us out of guest context.

Add support to the BookE code for the generic vcpu->mode field that describes
the above states.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/booke.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
Scott Wood - Aug. 15, 2012, 12:17 a.m.
On 08/14/2012 06:04 PM, Alexander Graf wrote:
> Generic KVM code might want to know whether we are inside guest context
> or outside. It also wants to be able to push us out of guest context.
> 
> Add support to the BookE code for the generic vcpu->mode field that describes
> the above states.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/powerpc/kvm/booke.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index bcf87fe..70a86c0 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -501,6 +501,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>  			continue;
>  		}
>  
> +		if (vcpu->mode == EXITING_GUEST_MODE) {
> +			r = 1;
> +			break;
> +		}
> +
> +		/* Going into guest context! Yay! */
> +		vcpu->mode = IN_GUEST_MODE;
> +		smp_wmb();
> +
>  		break;
>  	}

Normally on entry to this function mode should be OUTSIDE_GUEST_MODE,
right?  How could it possibly be EXITING_GUEST_MODE then, since that
only replaces IN_GUEST_MODE?

This doesn't match what x86 does with mode on entry.  Mode is supposed
to be set to IN_GUEST_MODE before requests are checked.

I'm not sure what the point of EXITING_GUEST_MODE is at all, compared to
just waiting until after interrupts are disabled before setting
IN_GUEST_MODE (which we do on ppc, but not on x86 even though it seems
like a trivial change), plus the existing ordering between mode and
requests.

-Scott


--
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
Alexander Graf - Aug. 15, 2012, 12:26 a.m.
On 15.08.2012, at 02:17, Scott Wood wrote:

> On 08/14/2012 06:04 PM, Alexander Graf wrote:
>> Generic KVM code might want to know whether we are inside guest context
>> or outside. It also wants to be able to push us out of guest context.
>> 
>> Add support to the BookE code for the generic vcpu->mode field that describes
>> the above states.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> arch/powerpc/kvm/booke.c |   11 +++++++++++
>> 1 files changed, 11 insertions(+), 0 deletions(-)
>> 
>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>> index bcf87fe..70a86c0 100644
>> --- a/arch/powerpc/kvm/booke.c
>> +++ b/arch/powerpc/kvm/booke.c
>> @@ -501,6 +501,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>> 			continue;
>> 		}
>> 
>> +		if (vcpu->mode == EXITING_GUEST_MODE) {
>> +			r = 1;
>> +			break;
>> +		}
>> +
>> +		/* Going into guest context! Yay! */
>> +		vcpu->mode = IN_GUEST_MODE;
>> +		smp_wmb();
>> +
>> 		break;
>> 	}
> 
> Normally on entry to this function mode should be OUTSIDE_GUEST_MODE,
> right?  How could it possibly be EXITING_GUEST_MODE then, since that
> only replaces IN_GUEST_MODE?
> 
> This doesn't match what x86 does with mode on entry.  Mode is supposed
> to be set to IN_GUEST_MODE before requests are checked.
> 
> I'm not sure what the point of EXITING_GUEST_MODE is at all, compared to
> just waiting until after interrupts are disabled before setting
> IN_GUEST_MODE (which we do on ppc, but not on x86 even though it seems
> like a trivial change), plus the existing ordering between mode and
> requests.

Well, the only real use case I could find for the mode was the remote vcpu kick. If we're not outside of guest mode, we get an IPI to notify us that requests are outstanding.

So I only get us into OUTSIDE_GUEST_MODE when we really exit __vcpu_run, thus are in user space. That doesn't reflect what x86 does, right, but so doesn't our whole loop concept.

However, since we might do the vcpu_block in this loop, we will never really get into OUTSIDE_GUEST_MODE, right? Hrm. So what would you suggest? Do all the handle_exit in OUTSIDE_GUEST_MODE scope and then reenter IN_GUEST_MODE in prepare_to_enter? We still wouldn't need an EXITING_GUEST_MODE though.


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
Scott Wood - Aug. 15, 2012, 1:17 a.m.
On 08/14/2012 07:26 PM, Alexander Graf wrote:
> 
> On 15.08.2012, at 02:17, Scott Wood wrote:
> 
>> On 08/14/2012 06:04 PM, Alexander Graf wrote:
>>> Generic KVM code might want to know whether we are inside guest context
>>> or outside. It also wants to be able to push us out of guest context.
>>>
>>> Add support to the BookE code for the generic vcpu->mode field that describes
>>> the above states.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>> arch/powerpc/kvm/booke.c |   11 +++++++++++
>>> 1 files changed, 11 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>>> index bcf87fe..70a86c0 100644
>>> --- a/arch/powerpc/kvm/booke.c
>>> +++ b/arch/powerpc/kvm/booke.c
>>> @@ -501,6 +501,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>>> 			continue;
>>> 		}
>>>
>>> +		if (vcpu->mode == EXITING_GUEST_MODE) {
>>> +			r = 1;
>>> +			break;
>>> +		}
>>> +
>>> +		/* Going into guest context! Yay! */
>>> +		vcpu->mode = IN_GUEST_MODE;
>>> +		smp_wmb();
>>> +
>>> 		break;
>>> 	}
>>
>> Normally on entry to this function mode should be OUTSIDE_GUEST_MODE,
>> right?  How could it possibly be EXITING_GUEST_MODE then, since that
>> only replaces IN_GUEST_MODE?
>>
>> This doesn't match what x86 does with mode on entry.  Mode is supposed
>> to be set to IN_GUEST_MODE before requests are checked.
>>
>> I'm not sure what the point of EXITING_GUEST_MODE is at all, compared to
>> just waiting until after interrupts are disabled before setting
>> IN_GUEST_MODE (which we do on ppc, but not on x86 even though it seems
>> like a trivial change), plus the existing ordering between mode and
>> requests.
> 
> Well, the only real use case I could find for the mode was the remote
> vcpu kick. If we're not outside of guest mode, we get an IPI to
> notify us that requests are outstanding.

I'm curious why this is done so differently for broadcast requests than
for single-cpu requests.

> So I only get us into OUTSIDE_GUEST_MODE when we really exit
> __vcpu_run, thus are in user space. That doesn't reflect what x86
> does, right, but so doesn't our whole loop concept.

OK.  We still need to do ordering like x86 does, because otherwise
there's a race where we could check requests before the request bit is
set, and still have make_all_cpus_request see OUTSIDE_GUEST_MODE and not
send an IPI.

> However, since we might do the vcpu_block in this loop, we will never
> really get into OUTSIDE_GUEST_MODE, right?

Not when the guest is just idling, only when it's exited to QEMU.

> Hrm. So what would you
> suggest? Do all the handle_exit in OUTSIDE_GUEST_MODE scope and then
> reenter IN_GUEST_MODE in prepare_to_enter? 

I suggest leaving this optimization until a need is felt.

> We still wouldn't need an EXITING_GUEST_MODE though.

Right.

-Scott


--
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
Scott Wood - Aug. 15, 2012, 1:25 a.m.
On 08/14/2012 06:04 PM, Alexander Graf wrote:
> +		/* Going into guest context! Yay! */
> +		vcpu->mode = IN_GUEST_MODE;
> +		smp_wmb();
> +
>  		break;
>  	}

What is the wmb protecting against here?

-Scott


--
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
Alexander Graf - Aug. 15, 2012, 9:29 a.m.
On 15.08.2012, at 03:17, Scott Wood wrote:

> On 08/14/2012 07:26 PM, Alexander Graf wrote:
>> 
>> On 15.08.2012, at 02:17, Scott Wood wrote:
>> 
>>> On 08/14/2012 06:04 PM, Alexander Graf wrote:
>>>> Generic KVM code might want to know whether we are inside guest context
>>>> or outside. It also wants to be able to push us out of guest context.
>>>> 
>>>> Add support to the BookE code for the generic vcpu->mode field that describes
>>>> the above states.
>>>> 
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>> arch/powerpc/kvm/booke.c |   11 +++++++++++
>>>> 1 files changed, 11 insertions(+), 0 deletions(-)
>>>> 
>>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>>>> index bcf87fe..70a86c0 100644
>>>> --- a/arch/powerpc/kvm/booke.c
>>>> +++ b/arch/powerpc/kvm/booke.c
>>>> @@ -501,6 +501,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>>>> 			continue;
>>>> 		}
>>>> 
>>>> +		if (vcpu->mode == EXITING_GUEST_MODE) {
>>>> +			r = 1;
>>>> +			break;
>>>> +		}
>>>> +
>>>> +		/* Going into guest context! Yay! */
>>>> +		vcpu->mode = IN_GUEST_MODE;
>>>> +		smp_wmb();
>>>> +
>>>> 		break;
>>>> 	}
>>> 
>>> Normally on entry to this function mode should be OUTSIDE_GUEST_MODE,
>>> right?  How could it possibly be EXITING_GUEST_MODE then, since that
>>> only replaces IN_GUEST_MODE?
>>> 
>>> This doesn't match what x86 does with mode on entry.  Mode is supposed
>>> to be set to IN_GUEST_MODE before requests are checked.
>>> 
>>> I'm not sure what the point of EXITING_GUEST_MODE is at all, compared to
>>> just waiting until after interrupts are disabled before setting
>>> IN_GUEST_MODE (which we do on ppc, but not on x86 even though it seems
>>> like a trivial change), plus the existing ordering between mode and
>>> requests.
>> 
>> Well, the only real use case I could find for the mode was the remote
>> vcpu kick. If we're not outside of guest mode, we get an IPI to
>> notify us that requests are outstanding.
> 
> I'm curious why this is done so differently for broadcast requests than
> for single-cpu requests.
> 
>> So I only get us into OUTSIDE_GUEST_MODE when we really exit
>> __vcpu_run, thus are in user space. That doesn't reflect what x86
>> does, right, but so doesn't our whole loop concept.
> 
> OK.  We still need to do ordering like x86 does, because otherwise
> there's a race where we could check requests before the request bit is
> set, and still have make_all_cpus_request see OUTSIDE_GUEST_MODE and not
> send an IPI.

Could you please send a patch showing what workflow you envision? The code as is should work, just be inefficient at times, right?


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
Scott Wood - Aug. 21, 2012, 1:41 a.m.
On 08/15/2012 04:29 AM, Alexander Graf wrote:
> 
> On 15.08.2012, at 03:17, Scott Wood wrote:
> 
>> On 08/14/2012 07:26 PM, Alexander Graf wrote:
>>>
>>> On 15.08.2012, at 02:17, Scott Wood wrote:
>>>
>>>> On 08/14/2012 06:04 PM, Alexander Graf wrote:
>>>>> Generic KVM code might want to know whether we are inside guest context
>>>>> or outside. It also wants to be able to push us out of guest context.
>>>>>
>>>>> Add support to the BookE code for the generic vcpu->mode field that describes
>>>>> the above states.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>> ---
>>>>> arch/powerpc/kvm/booke.c |   11 +++++++++++
>>>>> 1 files changed, 11 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>>>>> index bcf87fe..70a86c0 100644
>>>>> --- a/arch/powerpc/kvm/booke.c
>>>>> +++ b/arch/powerpc/kvm/booke.c
>>>>> @@ -501,6 +501,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>>>>> 			continue;
>>>>> 		}
>>>>>
>>>>> +		if (vcpu->mode == EXITING_GUEST_MODE) {
>>>>> +			r = 1;
>>>>> +			break;
>>>>> +		}
>>>>> +
>>>>> +		/* Going into guest context! Yay! */
>>>>> +		vcpu->mode = IN_GUEST_MODE;
>>>>> +		smp_wmb();
>>>>> +
>>>>> 		break;
>>>>> 	}
>>>>
>>>> Normally on entry to this function mode should be OUTSIDE_GUEST_MODE,
>>>> right?  How could it possibly be EXITING_GUEST_MODE then, since that
>>>> only replaces IN_GUEST_MODE?
>>>>
>>>> This doesn't match what x86 does with mode on entry.  Mode is supposed
>>>> to be set to IN_GUEST_MODE before requests are checked.
>>>>
>>>> I'm not sure what the point of EXITING_GUEST_MODE is at all, compared to
>>>> just waiting until after interrupts are disabled before setting
>>>> IN_GUEST_MODE (which we do on ppc, but not on x86 even though it seems
>>>> like a trivial change), plus the existing ordering between mode and
>>>> requests.
>>>
>>> Well, the only real use case I could find for the mode was the remote
>>> vcpu kick. If we're not outside of guest mode, we get an IPI to
>>> notify us that requests are outstanding.
>>
>> I'm curious why this is done so differently for broadcast requests than
>> for single-cpu requests.
>>
>>> So I only get us into OUTSIDE_GUEST_MODE when we really exit
>>> __vcpu_run, thus are in user space. That doesn't reflect what x86
>>> does, right, but so doesn't our whole loop concept.
>>
>> OK.  We still need to do ordering like x86 does, because otherwise
>> there's a race where we could check requests before the request bit is
>> set, and still have make_all_cpus_request see OUTSIDE_GUEST_MODE and not
>> send an IPI.
> 
> Could you please send a patch showing what workflow you envision? 

I'll try to do this tomorrow.

> The code as is should work, just be inefficient at times, right?

No, we could fail to send the IPI -- couldn't that result in the guest
using a stale TLB entry?

-Scott


--
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

Patch

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index bcf87fe..70a86c0 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -501,6 +501,15 @@  static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
 			continue;
 		}
 
+		if (vcpu->mode == EXITING_GUEST_MODE) {
+			r = 1;
+			break;
+		}
+
+		/* Going into guest context! Yay! */
+		vcpu->mode = IN_GUEST_MODE;
+		smp_wmb();
+
 		break;
 	}
 
@@ -572,6 +581,8 @@  int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	kvm_guest_exit();
 
 out:
+	vcpu->mode = OUTSIDE_GUEST_MODE;
+	smp_wmb();
 	local_irq_enable();
 	return ret;
 }