simplify bhyve vlapic ESR logic
ClosedPublic
Actions

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

Details

Reviewers

jhb
rgrimes
markj

Group Reviewers

bhyve

Commits

rS351609: Simplify bhyve vlapic ESR logic.

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

pmooney_pfmooney.com created this revision.May 22 2019, 8:29 PM

Herald added subscribers: bcran, imp. · View Herald TranscriptMay 22 2019, 8:29 PM

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

Herald added a subscriber: rgrimes. · View Herald TranscriptMay 23 2019, 12:47 AM

rgrimes removed reviewers: rgrimes, bhyve.May 23 2019, 12:48 AM

Herald added a reviewer: bhyve. · View Herald TranscriptMay 23 2019, 12:48 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

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.

pmooney_pfmooney.com added inline comments.May 23 2019, 5:29 PM

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

mgrooms_shrew.net added a subscriber: mgrooms_shrew.net.Jun 3 2019, 8:13 PM

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

pmooney_pfmooney.com added a reviewer: markj.Jul 24 2019, 3:20 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.