Re: [PATCH 3/4] xen/arm: set vpidr on the pcpu where the vcpu will run

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 16/02/2018 20:59, Stefano Stabellini wrote:
> On Fri, 16 Feb 2018, Julien Grall wrote:
>> On 16/02/2018 20:31, Stefano Stabellini wrote:
>>> On Fri, 16 Feb 2018, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 15/02/18 23:16, Stefano Stabellini wrote:
>>>>> On big.LITTLE systems not all cores have the same midr. Instead of
>>>>> initializing the vpidr to the boot cpu midr, set it to the value of the
>>>>> midr of the pcpu where the vcpu will run.
>>>>>
>>>>> This way, assuming that the vcpu has been created with the right pcpu
>>>>> affinity, the guest will be able to read the right vpidr value, matching
>>>>> the one of the physical cpu.
>>>>>
>>>>> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>> ---
>>>>>     xen/arch/arm/domain.c | 19 ++++++++++++++++---
>>>>>     1 file changed, 16 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>>>> index 532e824..280125f 100644
>>>>> --- a/xen/arch/arm/domain.c
>>>>> +++ b/xen/arch/arm/domain.c
>>>>> @@ -315,6 +315,22 @@ static void schedule_tail(struct vcpu *prev)
>>>>>     static void continue_new_vcpu(struct vcpu *prev)
>>>>>     {
>>>>>         current->arch.actlr = READ_SYSREG32(ACTLR_EL1);
>>>>> +    /*
>>>>> +     * Default the virtual ID to match the physical.
>>>>> +     *
>>>>> +     * In case the big.LITTLE systems, a guest should be created with
>>>>> +     * cpu affinity set so that all vcpus run on the same kind of
>>>>> pcpus.
>>>>> +     * Warn if it is not the case.
>>>>
>>>> continue_new_vcpu is only called once at domain creation. So this looks
>>>> pointless to check that here and probably in ctxt_switch_to.
>>>>
>>>> But I don't want to see such check at every context switch. This is
>>>> expensive
>>>> and we should not impact all the platforms for the benefits of an unsafe
>>>> configuration.
>>>>
>>>> If you really want to do that, then it should only be done when the vCPU
>>>> is
>>>> migrating. That will reduce a lot the performance impact.
>>>
>>> I don't want a check for every context switch either. I added it here
>>> because continue_new_vcpu is only called once per vcpu at domain
>>> creation -- it is a one time check. vcpus are supposed to be pinned (or
>>> cpu affinity specified) anyway, so I thought I wouldn't add the check in
>>> vcpu_migrate too. In any case, I am also happy to remove the check
>>> completely, as we have already warned the user enough.
>> If you agree that continue_new_vcpu is only called once per vCPU. Then I am
>> not sure to understand the purpose of the check. What are you trying to warn
>> the user with that?
> 
> The intention was to warn the user if she made a mistake with vcpu
> pinning and/or cpu affinity.

Oh that is current->domain->arch.vpidr and not vCPU. Sorry for that.

vpidr should be per vCPU. It is very dangerous to recommend the user to 
pin there all vCPUs of a domain to either only big or LITTLE. This is 
even worst than what we have today.

> 
> Even with this series and vcpu pinning, I assumed that only scenarios
> with vcpus assigned to pcpus of the same kind are allowed (see my other
> reply). Thus, I added this check to test once at boot that all vcpus
> in a domain have the same actlr.

That's plain wrong. You really can't assume that same actlr means same 
type of CPUs. Imagine they are RES0 on both.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel