Page MenuHomeFreeBSD

xen: fix setting vcpu id for APs
ClosedPublic

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

Details

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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

royger created this revision.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.

royger updated this revision to Diff 47688.Sep 5 2018, 7:26 AM
  • Remove xenpv_set_ids.
  • Make sure acpi_id is set when starting APs.

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?

cperciva added a comment.EditedSep 6 2018, 12:48 AM

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?

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.

royger updated this revision to Diff 47751.Sep 6 2018, 1:56 PM
  • Bind PV IPI event channels once vcpu_id is set.

Awesome, this fixes the boot on all the EC2 instances I've tried. Please commit!

This revision was not accepted when it landed; it landed in state Needs Review.Sep 13 2018, 7:07 AM
Closed by commit rS338625: xen: fix PV IPI setup (authored by royger). · Explain Why
This revision was automatically updated to reflect the committed changes.