Paths

Table of Contentst

Differential D19975

vmm: Add emulation for the `add` instruction
ClosedPublic
Actions

Authored by scottph on Apr 19 2019, 9:05 PM.

Details

Reviewers

jhb
tychon
rgrimes

Group Reviewers

bhyve

Commits

rS347065: Emulate the "ADD reg, r/m" instruction (opcode 03H).

Summary

Hasn't been needed thus far, apparently. OVMF's flash variable
storage is using add instructions when indexing the variable store
bootrom location that bhyve(8) emulates, so it's needed now.

Submitted by: D Scott Phillips <d.scott.phillips@intel.com>
Sponsored by: Intel Corporation
MFC After: 1 week

Test Plan

Turn on flash variable storage in OVMF. Apply the bhyve patch to support flash variable storage. See that boot does not die with "Failed to emulate instruction"...

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

scottph created this revision.Apr 19 2019, 9:05 PM

Herald added a reviewer: bhyve. · View Herald TranscriptApr 19 2019, 9:05 PM

Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript

A question to @jhb, should the order of the enum and the code updated to be in alphabetic order, in a seperate review, it would only obfuscate the change to do it here.

sys/amd64/vmm/vmm_instruction_emul.c
419	I do not find what the word Similarly refers to here. Might this comment be more descripive as: Macro creation of functions getaddflags{8,16,32,64}

This revision is now accepted and ready to land.Apr 20 2019, 12:09 AM

In D19975#429429, @rgrimes wrote:

A question to @jhb, should the order of the enum and the code updated to be in alphabetic order, in a separate review, it would only obfuscate the change to do it here.

Changing the order of the VIE_OP_TYPE_ enum values in a way that renumbers them would break binary backwards compatibility. vmm_instruction_emul.c gets built into both vmm.ko and usr.sbin/bhyve, which makes definitions that would seem file-local actually be part of the vmm device interface.

sys/amd64/vmm/vmm_instruction_emul.c
419	The comment was a call back to the comment on line 383, but it's not particularly close and I can see how it's not helpful. I'll update with the better comment you suggested.

reword comment about GETADDFLAGS macro

This revision now requires review to proceed.Apr 22 2019, 10:31 PM

scottph marked an inline comment as done.Apr 22 2019, 10:31 PM

I think this looks fine, just some minor style nits.

sys/amd64/vmm/vmm_instruction_emul.c
1242	Small style nits would be to render these as sentences, e.g. /* Get the first operand. */
1318	It would be tempting to rename 'getcc' to 'getsubflags' as a followup commit I think.
1644	It would be best to use a 4 space indent for the new code.

This revision is now accepted and ready to land.May 3 2019, 5:43 PM

jhb added inline comments.May 3 2019, 6:45 PM

sys/amd64/vmm/vmm_instruction_emul.c
1242	Nevermind, I see that this is a copy and paste of emulate_sub.

scottph added inline comments.May 3 2019, 8:04 PM

sys/amd64/vmm/vmm_instruction_emul.c
1242	Right, it looked to me like it was a small enough bit of code that duplication would be better for maintenance than folding the add and sub cases together. I suppose that probably deserves a mention in the commit message.

Closed by commit rS347065: Emulate the "ADD reg, r/m" instruction (opcode 03H). (authored by jhb). · Explain WhyMay 3 2019, 9:48 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

sys/

amd64/

vmm/

vmm_instruction_emul.c

100 lines

Diff 56515

View Options

sys/amd64/vmm/vmm_instruction_emul.c

vmm: Add emulation for the `add` instructionClosedPublicActions