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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 16 Feb 2018, Julien Grall wrote:
> 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@xxxxxxxxxx>
> > > > > > ---
> > > > > >     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.

Now your comments make sense! Yeah, I thought so too. I'll make the change.


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

Let's continue this discussion in the other thread which is more
appropriately about documentation.


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

It would be a false negative, and wouldn't trigger the warning. A false
positive would be worse: different pcpus with the same midr. Is that
possible?

In any case, at this point I am convinced that it is best to remove the
warning.

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