Skip to content

refactor to remove/clarify arch-specific parts of ukvm unikernel#157

Closed
djwillia wants to merge 4 commits intoSolo5:masterfrom
djwillia:asm-refactor
Closed

refactor to remove/clarify arch-specific parts of ukvm unikernel#157
djwillia wants to merge 4 commits intoSolo5:masterfrom
djwillia:asm-refactor

Conversation

@djwillia
Copy link
Copy Markdown
Member

The purpose of these changes is to move all asm and x86-specific code for the ukvm path into one place. This should help with the ARM port (#151) and hopefully the Hypervisor.framework patch too (#150).

The main changes are:

  • rely on ukvm to set up hardware state (gdt, sse)
  • interrupt handling is completely removed from ukvm
  • all assembly (inline or otherwise) for the ukvm target is in kernel/cpu.S, kernel/cpu.h, or kernel/ukvm/ukvm.h

Practically, this means some code moved from kernel/ to kernel/virtio.

Some of the inline assembly in kernel/ was in malloc.c, but it had to do with locks, which are unnecessary and targeted for removal in PR #155, which I have also merged here.

@mato
Copy link
Copy Markdown
Member

mato commented Feb 20, 2017

AFAICT this change is incomplete -- you remove the arch/cpu-specific code from kernel/ukvm but I don't see any code in the monitor that replaces it?

@djwillia
Copy link
Copy Markdown
Member Author

You are right, there is more being removed than added back in. If we don't support guest handling of interrupts (which we don't; we had interrupt injection code in ukvm at some point but removed it), then it turns out that we do not need to enable interrupts in the guest. This in turn means that we do not need an IDT to be set up in the guest (BTW, this is all related to one of your comments in #150).

A GDT and page table mapping were already being done in ukvm and do not need to be redone in the guest. The enabling of SSE for floating point is now in ukvm https://github.com/Solo5/solo5/pull/157/files#diff-2db47873a953eeb11019d73e0e2d16bfR302, but I believe that is the only addition to ukvm from what was already there.

Copy link
Copy Markdown
Collaborator

@ricarkol ricarkol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits.

sregs->efer |= EFER_LME;
uint64_t cr0 = (X86_CR0_NE | X86_CR0_PE | X86_CR0_PG)
& ~(X86_CR0_NW | X86_CR0_CD);
uint64_t cr4 = X86_CR4_PAE;
Copy link
Copy Markdown
Collaborator

@ricarkol ricarkol Feb 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cr4:X86_CR4_PAE and cr0:X86_CR0_PG are already set in setup_system_page_tables. You might want to remove them from setup_system_page_tables or from here.

/* The maximum possible size_t value has all bits set */
#define MAX_SIZE_T (~(size_t)0)

#ifndef USE_LOCKS /* ensure true if spin or recursive locks set */
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a reminder that you should not delete this code anymore. I guess git will tell you that.

#define X86_CR4_PAE _BITUL(X86_CR4_PAE_BIT)
#define X86_CR0_PE 0x00000001 /* Protected mode Enable */
#define X86_CR0_NE 0x00000020 /* Numeric Error enable (EX16 vs IRQ13) */
#define X86_CR0_PG 0x80000000 /* PaGing enable */
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a bit of a pain to change (and it's up to you), but using _BITUL made it easier to check for the flag number directly. For example, mapping 0x80000000 to 31 is not as "straightforward" as before.

@mato
Copy link
Copy Markdown
Member

mato commented May 11, 2017

Superseded by #192.

@mato mato closed this May 11, 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.

3 participants