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

From: Scott Wood <scottwood@freescale.com>
To: Alexander Graf <agraf@suse.de>
Cc: <kvm-ppc@vger.kernel.org>, KVM list <kvm@vger.kernel.org>
Subject: Re: [PATCH 17/38] KVM: PPC: BookE: Add support for vcpu->mode
Date: Mon, 20 Aug 2012 20:41:32 -0500	[thread overview]
Message-ID: <5032E74C.7010100@freescale.com> (raw)
In-Reply-To: <6F911453-9056-4070-8279-7ADA8C28FD47@suse.de>

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