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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> +     */
> +    if ( !current->domain->arch.vpidr )
> +        current->domain->arch.vpidr = current_cpu_data.midr.bits;
> +    else if ( current->domain->arch.vpidr != current_cpu_data.midr.bits )
> +    {
> +        gdprintk(XENLOG_WARNING,
> +                 "WARNING: possible corruptions! d%uv%u is running on a pcpu with 
> different midr (%x) from the others (%x)\n",
NIT: I would prefer if you use uppercase for pcpu and midr. This is 
easier to read.
Also, gdprintk already print the domain vCPU. So it is not necessary to 
have it again in the message.
> +                 current->domain->domain_id, current->vcpu_id,
> +                 current_cpu_data.midr.bits, current->domain->arch.vpidr);
> +    }
>   
>       schedule_tail(prev);
>   
> @@ -596,9 +612,6 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>       if ( (d->shared_info = alloc_xenheap_pages(0, 0)) == NULL )
>           goto fail;
>   
> -    /* Default the virtual ID to match the physical */
> -    d->arch.vpidr = boot_cpu_data.midr.bits;
> -
>       clear_page(d->shared_info);
>       share_xen_page_with_guest(
>           virt_to_page(d->shared_info), d, XENSHARE_writable);
Cheers,

--
Julien Grall

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