Page MenuHomeFreeBSD

vmm: do not call mtx_lock in icr_low write handler
ClosedPublic

Authored by corvink on Mon, Nov 21, 2:16 PM.

Details

Summary

A x2apic access is handled by a wrmsr exit. This handler is called in a
critical section. So, we can't lock a mtx in the icr_low handler.

Reported by: kp
Fixes: c0f35dbf19c3c8825bd2b321d8efd582807d1940

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Alternatively, could just add a new spin mutex lock for startup_cpus. (That's what I had planned to do). This isn't a critical performance path such that atomic ops is a big win over just having a small lock for this.

@pho had reported this to me via e-mail as well.

D37452.id113378.diff fixed the issue for me.

In D37452#851606, @jhb wrote:

Alternatively, could just add a new spin mutex lock for startup_cpus. (That's what I had planned to do). This isn't a critical performance path such that atomic ops is a big win over just having a small lock for this.

I think that makes more sense. When CPU scalability isn't a major concern, it's almost always better to rely on mutexes for synchronization.

As an alternative to using a separate lock, I wonder if it can make sense to have vlapic_icrlo_write_handler() trigger a vmexit that gets handled in vm_run() instead? We can lock a regular mutex there.

  • handle setting of startup_cpus in a vm_run handler
  • use CPU_COPY to set ipimask in SIPI case
sys/amd64/vmm/io/vlapic.c
1189

This wasn't done in the !vlapic->ipi_exit case before. I don't think that resetting the vlapic is an issue for old bhyve binaries. So, I didn't masked it to keep the code cleaner.

If you complain about it, I'm going to mask it.

LGTM with the suggested fix.

sys/amd64/vmm/io/vlapic.c
1215
  • set correct cpuid for STARTUP_AP exits
This revision was not accepted when it landed; it landed in state Needs Review.Wed, Nov 23, 8:01 AM
This revision was automatically updated to reflect the committed changes.