Skip to content

Conversation

@nick-kvmhv
Copy link
Contributor

The fix that resolved the issue we discussed here: #461
The rationale behind this change is that m_gdt_reg.limit is coming directly from SGDT and therefore is size minus one.

@rianquinn
Copy link
Member

After having a chance to think about this more, I'm confused as to why this is needed. Before I merge, I want to see if I can reproduce the same issue so that I can study the root cause (which might just be my ignorance).

Also, astyle is made because it should be + 1 and not +1.

@nick-kvmhv
Copy link
Contributor Author

nick-kvmhv commented Jun 26, 2017

On why it is needed, I think it is because as OSWiki states here http://wiki.osdev.org/Global_Descriptor_Table
The offset is the linear address of the table itself, which means that paging applies. The size is the size of the table subtracted by 1. This is because the maximum value of size is 65535, while the GDT can be up to 65536 bytes (a maximum of 8192 entries). Further no GDT can have a size of 0.
So, I am getting 0x57 as the size with FS=0x53 and they both give you 0x10 after >> 3

@rianquinn
Copy link
Member

Huh, I knew about the -1 for the IDT but somehow missed that for the GDT. This being the case, this line should have a -1 similar to the IDT
https://github.com/Bareflank/hypervisor/blob/master/bfvmm/include/intrinsics/gdt_x64.h#L242

The IDT code can be seen here:
https://github.com/Bareflank/hypervisor/blob/master/bfvmm/include/intrinsics/idt_x64.h#L194

This will likely need a small fix to the unit tests as well. Can you make these changes as part of your PR, as well as fix the astyle error?

@nick-kvmhv
Copy link
Contributor Author

Not sure if I figured out the style issues, but I added the created GDT correction and also updated the unit tests.

@rianquinn
Copy link
Member

LGTM +1

@rianquinn rianquinn merged commit 9de33c3 into Bareflank:master Jun 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants