Differential D17013

xen: fix setting vcpu id for APs
ClosedPublic
Actions

Authored by royger on Sep 4 2018, 2:55 PM.

Details

Reviewers

cperciva

Commits

rS338625: xen: fix PV IPI setup

Summary

So that it's set before calling xen_setup_cpus (which happens at
SI_SUB_SMP-1).

This requires moving the setup of the vcpu id for the APs before
calling into init_secondary_tail, because that part of the AP
initialization happens at SI_SUB_SMP.

Reported by: cperciva
Sponsored by: Citrix Systems R&D

Test Plan

Can you test this on EC2? Thanks!

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 19435
Build 19032: arc lint + arc unit

Event Timeline

royger created this revision.Sep 4 2018, 2:55 PM

Harbormaster completed remote builds in B19382: Diff 47639.Sep 4 2018, 2:55 PM

Hmm, I agree that it looks like this *should* fix the problem, but I'm still seeing the instance hang waiting for TLB shootdown IPIs to complete. Maybe there's something else going on here.

Remove xenpv_set_ids.
Make sure acpi_id is set when starting APs.

Harbormaster completed remote builds in B19411: Diff 47688.Sep 5 2018, 7:26 AM

Still doesn't work. We have a chicken-and-egg problem between launching CPUs and recording their IDs, it seems...

sys/x86/acpica/madt.c
762 ↗	(On Diff #47688)	Alas, this doesn't work either. madt_set_ids uses CPU_FOREACH, which iterates through the all_cpus mask, which isn't set until after native_start_all_aps finishes launching an AP.
sys/x86/xen/pvcpu_enum.c
262 ↗	(On Diff #47688)	Is it correct to nuke this completely? I assumed that xen_hvm_cpu_init would only set vcpu_id when running in HVM and so we would still need xenpv_set_ids for PV?

I wonder, can we make this work by changing the lines

PCPU_SET(vcpu_id, (regs[0] & XEN_HVM_CPUID_VCPU_ID_PRESENT) ?
       regs[1] : PCPU_GET(acpi_id));

in xen_hvm_cpu_init to

PCPU_SET(vcpu_id, (regs[0] & XEN_HVM_CPUID_VCPU_ID_PRESENT) ?
       regs[1] : -1);

and then resurrecting xen_set_vcpu_id but having it only set vcpu_id if it is currently set to -1?

That way if we have XEN_HVM_CPUID_VCPU_ID_PRESENT we'll get vcpu_id from there (and it doesn't matter if that runs before acpi_id is set) and if we don't then we'll fill in the right value slightly later (after acpi_id is set, but we don't need to be running on the AP in question at that point).

Hmm, I guess filling in vcpu_id after xen_hvm_cpu_init returns won't work, since we use that value in the "Set the vCPU info." code at the end of that function.

Can the vCPU info be set in a CPU_FOREACH loop in a later SYSINIT instead?

In D17013#363424, @cperciva wrote:
I wonder, can we make this work by changing the lines
PCPU_SET(vcpu_id, (regs[0] & XEN_HVM_CPUID_VCPU_ID_PRESENT) ?
       regs[1] : PCPU_GET(acpi_id));
in xen_hvm_cpu_init to
PCPU_SET(vcpu_id, (regs[0] & XEN_HVM_CPUID_VCPU_ID_PRESENT) ?
       regs[1] : -1);
and then resurrecting xen_set_vcpu_id but having it only set vcpu_id if it is currently set to -1?

That way if we have XEN_HVM_CPUID_VCPU_ID_PRESENT we'll get vcpu_id from there (and it doesn't matter if that runs before acpi_id is set) and if we don't then we'll fill in the right value slightly later (after acpi_id is set, but we don't need to be running on the AP in question at that point).

xen_hvm_cpu_init is also called on resume from suspension, so we would have to add a guard in order to avoid setting vcpu_id to -1 on resume.

royger marked an inline comment as done.Sep 6 2018, 12:03 PM

royger added inline comments.

sys/x86/xen/pvcpu_enum.c
262 ↗	(On Diff #47688)	No, xen_hvm_cpu_init is called by all guest types, and PV guests (without ACPI) will get the vcpu_id from the cpuid instruction. All the Xen versions that can run FreeBSD as PV will expose the vcpu_id through the cpuid leaf.