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

Adding bootrom support #126

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Adding bootrom support #126

wants to merge 2 commits into from

Conversation

dwoz
Copy link

@dwoz dwoz commented Dec 25, 2016

No description provided.

Copy link
Member

@jeremyhu jeremyhu left a comment

Choose a reason for hiding this comment

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

While you might prefer spaces for indentation, the existing file format is using tabs, so please be consistent and fix the indentation whitespace to match existing style.

@@ -50,7 +50,7 @@
#include <xhyve/virtio.h>
#include <xhyve/block_if.h>

#define VTBLK_RINGSZ 64
#define VTBLK_RINGSZ 128
Copy link
Member

Choose a reason for hiding this comment

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

This should be broken out into a separate commit with additional justification.

goto done;
}

/*
Copy link
Member

Choose a reason for hiding this comment

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

NIT: I'd put this comment up by the definition of MAX_BOOTROM_SIZE

goto done;
}

if (fstat(fd, &sbuf) < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Fix the indentation here.

@@ -2041,6 +2042,7 @@ vmx_exit_process(struct vmx *vmx, int vcpu, struct vm_exit *vmexit)
*/
gpa = vmcs_gpa(vcpu);
if (vm_mem_allocated(vmx->vm, gpa) ||
bootrom_contains_gpa(gpa) ||
Copy link
Member

Choose a reason for hiding this comment

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

I cannot articulate why, exactly, but I'm concerned with this particular change. It feels like a layering violation.

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