Re: [Xen-devel] [PATCH RFC v1 49/74] x86/guest: map per-cpu vcpu_info area.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 14:06, <wei.liu2@xxxxxxxxxx> wrote:
> So that the limit of XEN_LEGACY_MAX_VCPUS can be lifted.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Should be moved earlier maybe?

Especially the changes to time.c undoing/redoing earlier changes
suggests so.

> --- a/xen/arch/x86/guest/xen.c
> +++ b/xen/arch/x86/guest/xen.c
> @@ -38,6 +38,10 @@ static struct rangeset *mem;
>  
>  DEFINE_PER_CPU(unsigned int, vcpu_id);
>  
> +static struct vcpu_info *vcpu_info;
> +unsigned long vcpu_info_mapped[BITS_TO_LONGS(NR_CPUS)];

static

> @@ -101,6 +105,38 @@ static void map_shared_info(void)
>          xchg(&XEN_shared_info->evtchn_mask[i], ~0ul);
>  }
>  
> +static int map_vcpuinfo(void)
> +{
> +    unsigned int vcpu = this_cpu(vcpu_id);
> +    struct vcpu_register_vcpu_info info = { };

I doubt you need the initializer here.

> +    long rc;
> +
> +    if ( !vcpu_info )
> +    {
> +        this_cpu(vcpu_info) = &XEN_shared_info->vcpu_info[vcpu];
> +        return 0;
> +    }
> +
> +    if ( test_bit(vcpu, vcpu_info_mapped) )
> +    {
> +        this_cpu(vcpu_info) = &vcpu_info[vcpu];
> +        return 0;
> +    }
> +
> +    info.mfn = virt_to_mfn(&vcpu_info[vcpu]);
> +    info.offset = (unsigned long)&vcpu_info[vcpu] & ~PAGE_MASK;
> +    rc = xen_hypercall_vcpu_op(VCPUOP_register_vcpu_info, vcpu, &info);
> +    if ( rc )
> +        this_cpu(vcpu_info) = &XEN_shared_info->vcpu_info[vcpu];

You need to avoid producing an out of bounds pointer here for
large vcpu values.

> @@ -176,12 +211,34 @@ void __init hypervisor_setup(void)
>      map_shared_info();
>      set_vcpu_id();
>  
> +    vcpu_info = xzalloc_array(struct vcpu_info, nr_cpu_ids);
> +    if ( map_vcpuinfo() || !vcpu_info )
> +    {
> +        if ( vcpu_info )
> +        {
> +            xfree(vcpu_info);
> +            vcpu_info = NULL;
> +        }
> +        if ( nr_cpu_ids > XEN_LEGACY_MAX_VCPUS )

How about

    if ( map_vcpuinfo() )
    {
        xfree(vcpu_info);
        vcpu_info = NULL;
    }
    if ( !vcpu_info && nr_cpu_ids > XEN_LEGACY_MAX_VCPUS )
    {
        ...

?

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -533,11 +533,11 @@ static struct platform_timesource __initdata plt_tsc =
>   * Xen clock source is a variant of TSC source.
>   */
>  
> -DECLARE_PER_CPU(unsigned int, vcpu_id);
> +DECLARE_PER_CPU(struct vcpu_info *, vcpu_info);

I didn't notice the one being removed here - both shouldn't be
declared here, but in a header.

> @@ -107,6 +108,12 @@ static inline long xen_hypercall_hvm_op(unsigned int op, 
> void *arg)
>      return _hypercall64_2(long, __HYPERVISOR_hvm_op, op, arg);
>  }
>  
> +static inline long xen_hypercall_vcpu_op(unsigned int cmd, unsigned int vcpu,

I believe "int" is sufficient here (and then also for the variable(s) into
which the return value is/are being latched).

Jan

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