Re: [PATCH 1/1] x86: centralize default APIC id definition

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.09.2021 21:39, Alex Olson wrote:
> Inspired by an earlier attempt by Chao Gao <chao.gao@xxxxxxxxx>,
> this revision aims to put the hypervisor in control of x86 APIC identifier
> definition instead of hard-coding a formula in multiple places
> (libxl, hvmloader, hypervisor).
> 
> This is intended as a first step toward exposing/altering CPU topology
> seen by guests.
> 
> Changes:
> 
> - Add field to vlapic for holding default ID (on reset)
> 
> - add HVMOP_get_vcpu_topology_id hypercall so libxl (for PVH domains)
>   can access APIC ids needed for ACPI table definition prior to domain start.
> 
> - For HVM guests, hvmloader now also uses the same hypercall.
> 
> - Make CPUID code use vlapic ID instead of hard-coded formula
>   for runtime reporting to guests

I'm afraid a primary question from back at the time remains: How is
migration of a guest from an old hypervisor to one with this change
in place going to work?

> --- a/tools/libs/light/libxl_x86_acpi.c
> +++ b/tools/libs/light/libxl_x86_acpi.c
> @@ -79,9 +79,13 @@ static void acpi_mem_free(struct acpi_ctxt *ctxt,
>  {
>  }
>  
> -static uint32_t acpi_lapic_id(unsigned cpu)
> +static uint32_t acpi_lapic_id(unsigned cpu, void *arg)
>  {
> -    return cpu * 2;
> +    struct xc_dom_image *dom = (struct xc_dom_image *)arg;

No need for the cast.

> +    uint32_t ret = 0xdeadbeef;
> +    int rc;
> +    rc = xc_get_vcpu_topology_id(dom->xch, dom->guest_domid, cpu, &ret);
> +    return ret;
>  }

No need for the local variable "rc" if you don't evaluate it.

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -867,8 +867,10 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>      case 0x1:
>          /* TODO: Rework topology logic. */
>          res->b &= 0x00ffffffu;
> -        if ( is_hvm_domain(d) )
> -            res->b |= (v->vcpu_id * 2) << 24;
> +
> +#ifdef CONFIG_HVM
> +        res->b |= vlapic_get_default_id(v) << 24;
> +#endif

How come you drop the is_hvm_domain() here? There also should be no
need for such an #ifdef here ...

> @@ -1049,7 +1051,13 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
>              *(uint8_t *)&res->c = subleaf;
>  
>              /* Fix the x2APIC identifier. */
> -            res->d = v->vcpu_id * 2;
> +#ifdef CONFIG_HVM
> +            res->d = vlapic_get_default_id(v);
> +#endif

... or here.

> +        }
> +        else
> +        {
> +            *res = EMPTY_LEAF;
>          }

No need for braces in such a case.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1555,7 +1555,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
>          goto fail1;
>  
>      /* NB: vlapic_init must be called before hvm_funcs.vcpu_initialise */
> -    rc = vlapic_init(v);
> +    rc = vlapic_init(v, v->vcpu_id * 2);

Now that's odd: The goal of the patch is to eliminate such, and
here's you're adding a new instance?

> @@ -5084,6 +5084,40 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          rc = current->hcall_compat ? compat_altp2m_op(arg) : 
> do_altp2m_op(arg);
>          break;
>  
> +    case HVMOP_get_vcpu_topology_id:
> +    {
> +        struct domain *d;
> +        struct xen_vcpu_topology_id tid;
> +
> +        if ( copy_from_guest(&tid, arg, 1) )
> +            return -EFAULT;
> +
> +        if (tid.domid != DOMID_SELF && !is_hardware_domain(current->domain))

This wants to be a proper XSM check, I suppose, and allow more than
just the hardware domain to access this in case the controller of
the domain doesn't run in Dom0 (see XSM_TARGET).

Also, nit: Style (see all the other if()s).

> +            return -EPERM;
> +
> +        if ( (d = rcu_lock_domain_by_any_id(tid.domid)) == NULL )
> +            return -ESRCH;
> +
> +        if ( !is_hvm_domain(d))

Nit: Style again.

> +        {
> +            rc = -EOPNOTSUPP;
> +            goto get_cpu_topology_failed;
> +        }
> +
> +        if ( tid.vcpu_id >= d->max_vcpus )

Please use domain_vcpu() ...

> +        {
> +            rc = -EINVAL;
> +            goto get_cpu_topology_failed;
> +        }
> +        tid.topology_id = vlapic_get_default_id(d->vcpu[tid.vcpu_id]);

... to guard this array access.

> @@ -1508,7 +1514,7 @@ static void lapic_load_fixup(struct vlapic *vlapic)
>           * here, but can be dropped as soon as it is found to conflict with
>           * other (future) changes.
>           */
> -        if ( GET_xAPIC_ID(id) != vlapic_vcpu(vlapic)->vcpu_id * 2 ||
> +        if ( GET_xAPIC_ID(id) != vlapic->hw.default_id ||
>               id != SET_xAPIC_ID(GET_xAPIC_ID(id)) )
>              printk(XENLOG_G_WARNING "%pv: bogus APIC ID %#x loaded\n",
>                     vlapic_vcpu(vlapic), id);

As to my initial comment - I expect this warning will trigger for
about every vCPU of a guest migrating in from an older hypervisor.

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -183,6 +183,23 @@ struct xen_hvm_get_mem_type {
>  typedef struct xen_hvm_get_mem_type xen_hvm_get_mem_type_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_get_mem_type_t);
>  
> +/*
> + * HVMOP_get_cpu_topology is used by guest to acquire vcpu topology from
> + * hypervisor.
> + */
> +#define HVMOP_get_vcpu_topology_id     16

Careful with the number choice here - 16 used to be HVMOP_inject_msi
until dm-op was introduced. Interfaces exposed to guests themselves
need to not invoke unexpected operations on older hypervisors.

> +struct xen_vcpu_topology_id {
> +    /* IN */
> +    domid_t domid;
> +    uint32_t vcpu_id;

Please make padding explict, checking it to be zero on input.

Jan