Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fpu and MSR dumping in vcpu_state_dump #251

Merged
merged 1 commit into from Jan 7, 2020

Conversation

@nevilad
Copy link
Contributor

nevilad commented Dec 10, 2019

Added fpu and MSR dumping to vcpu_state_dump since these are part ofthe guest state (these are switched from host to guest values before each vmentry and back after each vmexit).

@nevilad nevilad force-pushed the nevilad:extended_state_dump branch from 6c83be0 to 43e5f99 Dec 10, 2019
@nevilad

This comment has been minimized.

Copy link
Contributor Author

nevilad commented Dec 19, 2019

What is with this PR? Are there any questions?

Copy link
Contributor

wcwang left a comment

Thanks for your pull requests. Currently, I am busy on HAXM new APIs development and sorry for late response. Could you review my comments inline? And you are welcome to give your opinions and discuss later. For other pull requests, I will review them meanwhile. Thanks for understanding.

core/vcpu.c Outdated
// Dump FPU state
hax_log(HAX_LOGW,
"FX layout:\n"
"fcw: %08x fsw: %08x ftw: %08x res1: %08x fop: %08x\n"

This comment has been minimized.

Copy link
@wcwang

wcwang Dec 23, 2019

Contributor
  1. Uppercase each state as vcpu state format.
  2. Pay attention to the space usage. Use one space after the colon and use 2 spaces between states.

This comment has been minimized.

Copy link
@nevilad

nevilad Dec 23, 2019

Author Contributor
  1. OK
  2. I use 2 spaces between states. Field names are of different length, so I use different number of spaces after the colon, so the output of different lines will be aligned by field names start and values, and that makes the output easier to understand. Are you sure to use one space? See how the output looks like, I think it is better.

This comment has been minimized.

Copy link
@wcwang

wcwang Dec 23, 2019

Contributor

Great, using column alignment is better. I agree with you.

core/vcpu.c Outdated
"FX layout:\n"
"fcw: %08x fsw: %08x ftw: %08x res1: %08x fop: %08x\n"
"fpu_ip: %08llx fpu_dp: %08llx\n"
"mxcsr: %08x mxcsr_mask: %08x\n",

This comment has been minimized.

Copy link
@wcwang

wcwang Dec 23, 2019

Contributor

As both states are uint32_t, should they use 'lx' instead of 'x'?
MXCSR: %08lx MXCSR_MASK: %08lx

This comment has been minimized.

Copy link
@nevilad

nevilad Dec 23, 2019

Author Contributor

I thought under Windows 'x' is formatting a 32 bit value. Is this not true, or different under other platforms?

This comment has been minimized.

Copy link
@wcwang

wcwang Dec 24, 2019

Contributor

You are right. I referred to the table from https://www.cs.uic.edu/~jbell/CourseNotes/C_Programming/DataTypesSummary.pdf and considered 'lx' should be more exact. It is up to you.

core/vcpu.c Outdated
gfx->fpu_dp, gfx->mxcsr, gfx->mxcsr_mask);

for (i = 0; i < 16; i++) {
hax_log(HAX_LOGW, "st_mm[%d] %02x%02x%02x%02x%02x%02x%02x%02x\n", i,

This comment has been minimized.

Copy link
@wcwang

wcwang Dec 23, 2019

Contributor

Do you mind to add the colon after the array name and add a space every 4 bytes for separation?

This comment has been minimized.

Copy link
@nevilad

nevilad Dec 23, 2019

Author Contributor

Like this:
st_mm[%d]: %02x%02x%02x%02x %02x%02x%02x%02x\n
?

This comment has been minimized.

Copy link
@wcwang

wcwang Dec 24, 2019

Contributor

Correct, 4-byte separation is just for reading convenience.

core/vcpu.c Outdated

for (i = 0; i < 16; i++) {
hax_log(HAX_LOGW, "st_mm[%d] %02x%02x%02x%02x%02x%02x%02x%02x\n", i,
gfx->st_mm[0][i], gfx->st_mm[1][i], gfx->st_mm[2][i],

This comment has been minimized.

Copy link
@wcwang

wcwang Dec 23, 2019

Contributor

Could you explain why you output the array by column? According to the 2-dimensional array in memory layout, do you think it is more reasonable as below format?

for (i = 0; i < 8; i++) {
    hax_log(HAX_LOGW, "st_mm[%d]: %02x%02x%02x%02x %02x%02x%02x%02x "
            "%02x%02x%02x%02x %02x%02x%02x%02x\n", i, gfx->st_mm[i][0],
            gfx->st_mm[i][1], gfx->st_mm[i][2], gfx->st_mm[i][3],
            gfx->st_mm[i][4], gfx->st_mm[i][5], gfx->st_mm[i][6],
            gfx->st_mm[i][7], gfx->st_mm[i][8], gfx->st_mm[i][9],
            gfx->st_mm[i][10], gfx->st_mm[i][11], gfx->st_mm[i][12],
            gfx->st_mm[i][13], gfx->st_mm[i][14], gfx->st_mm[i][15]);
}

This comment has been minimized.

Copy link
@nevilad

nevilad Dec 23, 2019

Author Contributor

I thought st_mm are registers of 8 byte size. Can you give me Intel docs link where these are described?

This comment has been minimized.

Copy link
@wcwang

wcwang Dec 24, 2019

Contributor

According to the definition in SDM Vol. 2A 3.1.1.3, st_mm are 8-byte width. I am not sure why the definition of the array here is 8 by 16. But from the existing code, see vcpu_get_fpu() in core/vcpu.c, the nested loop is raws first and then columns. Anyway, I think the 16 bytes within each raw are memory consecutive. If you are sure your implementation is reasonable, I will also accept the approach as I cannot find any exact document link to confirm that. Of course, to make the format consistent with others is okay.

SDM definition:

ST(i) - The ith element from the top of the FPU register stack (i ← 0 through 7).
mm - An MMX register. The 64-bit MMX registers are: MM0 through MM7.

This comment has been minimized.

Copy link
@nevilad

nevilad Dec 24, 2019

Author Contributor

Table 3-47 in sdm-vol-2abcd - these are saved by fxrstor as 10 bytes width, 6 bytes reserved. Rewritten the code for this and xmms.

core/vcpu.c Outdated
}

for (i = 0; i < 96; i += 8) {
hax_log(HAX_LOGW,"pad %02x%02x%02x%02x%02x%02x%02x%02x\n",

This comment has been minimized.

Copy link
@wcwang

wcwang Dec 23, 2019

Contributor

Add the colon and spaces for format.

hax_log(HAX_LOGW, "pad: %02x%02x%02x%02x %02x%02x%02x%02x\n",

This comment has been minimized.

Copy link
@nevilad

nevilad Dec 23, 2019

Author Contributor

OK


for (i = 0; i < (int)hax->apm_general_count; i++) {
uint32_t msr = (uint32_t)(IA32_PMC0 + i);
hax_log(HAX_LOGW, "MSR %08x:%08llx\n", msr, gstate->apm_pmc_msrs[i]);

This comment has been minimized.

Copy link
@wcwang

wcwang Dec 23, 2019

Contributor

Should 0x08lx be for uint32_t?

@nevilad nevilad force-pushed the nevilad:extended_state_dump branch from 43e5f99 to f29cbb0 Dec 24, 2019
core/vcpu.c Outdated
}

for (i = 0; i < 96; i += 8) {
hax_log(HAX_LOGW,"pad: %02x%02x%02x%02x %02x%02x%02x%02x\n",

This comment has been minimized.

Copy link
@wcwang

wcwang Jan 6, 2020

Contributor

Please add a space after the first comma.

This comment has been minimized.

Copy link
@nevilad

nevilad Jan 6, 2020

Author Contributor

Done.

the guest state.

Signed-off-by: Alexey Romko <nevilad@yahoo.com>
@nevilad nevilad force-pushed the nevilad:extended_state_dump branch from f29cbb0 to f6e97ed Jan 6, 2020
@wcwang
wcwang approved these changes Jan 7, 2020
@wayne-ma

This comment has been minimized.

Copy link
Contributor

wayne-ma commented Jan 7, 2020

Ok to verify

@wcwang wcwang merged commit adf4b6a into intel:master Jan 7, 2020
2 checks passed
2 checks passed
Jenkins Build Result pass
Details
Travis CI - Pull Request Build Passed
Details
@nevilad nevilad deleted the nevilad:extended_state_dump branch Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.