Page MenuHomeFreeBSD

simplify bhyve vlapic ESR logic
ClosedPublic

Authored by pmooney_pfmooney.com on May 22 2019, 8:29 PM.

Details

Summary

The bhyve vLAPIC uses an instance-global flag to indicate when an error LVT is being delivered. This is to prevent infinite recursion if the error LVT itself is configured with an invalid vector. Such error-handling status could be passed as a function argument, rather than more complicated logic with "global" state.

This was inspired by the bhyve save/restore work, which called into question vLAPIC state outside the raw LAPIC page itself.

SmartOS ticket: OS-7777

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

Minor nits, and a major win, good work!

sys/amd64/vmm/io/vlapic.c
83 ↗(On Diff #57720)

extra white space

696 ↗(On Diff #57720)

Wow thats a win! 6 cases collapse to 1

Fix excess whitespace and add attribution

pmooney_pfmooney.com marked an inline comment as done.May 22 2019, 10:53 PM
rgrimes accepted this revision.May 23 2019, 12:46 AM
This revision is now accepted and ready to land.May 23 2019, 12:46 AM
rgrimes resigned from this revision.May 23 2019, 12:47 AM
This revision now requires review to proceed.May 23 2019, 12:47 AM
rgrimes accepted this revision as: rgrimes.May 23 2019, 12:48 AM
This revision is now accepted and ready to land.May 23 2019, 12:48 AM
jhb added a comment.May 23 2019, 4:32 PM

I think it would actually be a smaller and simpler diff to add a 'firing_error' bool to vlapic_fire_lvt() that is normally false except in vlapic_set_error() and then have the vlapic_fire_lvt() pass that down rather through the various layers. OTOH, I'm not really sure that is a lot better than esr_firing and just ignoring the field during suspend and restore?

sys/amd64/vmm/io/vlapic.c
621 ↗(On Diff #57735)

The APIC_LVT_DM_FIXED is lost here as well.

694 ↗(On Diff #57735)

These forced FIXED delivery mode writes are getting lost now for TIMER and ERROR.

sys/amd64/vmm/io/vlapic.c
621 ↗(On Diff #57735)

See below

625 ↗(On Diff #57735)

Will eliminate the ILLEGAL_VECTOR qualification here per @jhb's feedback.

694 ↗(On Diff #57735)

APIC_LVT_DM_FIXED is defined to be zero, so those ORs were effectively no-ops. The code which minds the LVTs after writes ensures that the delivery modes are valid.

Eliminate the ILLEGAL_VECTOR qualification in vlapic_set_error per @jhb's feedback and rebase.

This revision now requires review to proceed.Jun 1 2019, 12:10 AM

Merged downstream in SmartOS as aa2898c4.

rgrimes accepted this revision as: rgrimes.Jun 10 2019, 2:28 PM
This revision is now accepted and ready to land.Jun 10 2019, 2:28 PM
markj accepted this revision as: markj.Aug 7 2019, 6:42 PM
markj added inline comments.
sys/amd64/vmm/io/vlapic.c
694 ↗(On Diff #57735)

In particular, vlapic_lvt_write_handler() does not include the delivery mode in the saved LVT entry for timer and error interrupts, forcing it to fixed mode.

jhb accepted this revision.Aug 7 2019, 7:15 PM
jhb added inline comments.
sys/amd64/vmm/io/vlapic.c
694 ↗(On Diff #57735)

Ok. I found it kind of useful in a documentation sense that if you sit down with the SDM and read the requirements of the given LVT entries, these specific ones ignore the delivery mode and always use fixed, so even if the or with 0 was a no-op, it still matched the spec, as it were.

This revision was automatically updated to reflect the committed changes.