On 24.09.2021 21:39, Alex Olson wrote:
> Inspired by an earlier attempt by Chao Gao <chao....@intel.com>,
> 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
