enable kvm to support arm64 #919

lubinszARM · 2019-09-26T06:48:05Z

There are 4 jobs were finished in this package:
1, Virtual machine initialization.
2, Bluepill implementation.
3, Move ring0.Vectors() into the address with 11-bits alignment.
4, Basic support for "SwitchToUser".
Signed-off-by: Bin Lu bin.lu@arm.com

amscanne

Thanks for this contribution! I’ve love to get this merged.

My main feedback is simply that there’s a lot of duplication and copied code that should be common. I explain why this is not desirable below.

I don’t see major issues outside that, but I’d like to do another pass with those things fixed. I hope that makes sense.

amscanne · 2019-10-02T15:41:49Z

pkg/sentry/platform/kvm/bluepill.go

-	}
-
-	// Setup the trampoline.
-	dieArchSetup(c, context, &c.dieState.guestRegs)


Why move this function? I intentionally tried to split arch-specific bits into dieArchSetup already. It’s unclear to me what will need to change from the above.

Hi @amscanne,
_KVM_GET_REGS/_KVM_SET_REGS/_KVM_SET_SREGS are not supported on Arm64 platform, so getUserRegisters() & userRegs can't be used on Arm64 platform.
Currently, on Arm64 platform, we only use _KVM_SET_ONE_REG/_KVM_GET_ONE_REG to set/get registers.
Maybe I can use _KVM_GET_ONE_REG to simulate _KVM_GET_REGS on Arm platform.

amscanne · 2019-10-02T15:42:04Z

pkg/sentry/platform/kvm/bluepill.go

-	dieArchSetup(c, context, &c.dieState.guestRegs)
-}
-
-func init() {


Same comment. Why is this arch-specific?

Update: it seems like it’s just the signal itself. I recommend just moving that into a constant in the arch-specific files. E.g.

const bluepillSignal = SIGILL

And just having this block refer to that signal.

Hi,
Got it.
Done.

Looks like this isn't done?

Looks like this isn't done?

Done. Sorry for it. :(

amscanne · 2019-10-02T15:45:02Z

pkg/sentry/platform/kvm/bluepill_arm64.go

+)
+
+var (
+	// bounceSignal is the signal used for bouncing KVM.


These are all common? I think you could move the common bits into bluepill.go. The bounce vector itself probably makes sense to keep as arch-specific.

Hi @amscanne
The implementation for test case 'TestBounce' will be delivered in another PR.
'bounceSignal' , 'unceSignalMask' and 'bounce' are common.
And 'VirtualizationException' will be defined as a different value on Arm64.
Let me moved them into bluepill.go

pkg/sentry/platform/kvm/bluepill_arm64.go

pkg/sentry/platform/kvm/bluepill_arm64.s

pkg/sentry/platform/kvm/kvm_const_arm64.go

pkg/sentry/platform/kvm/machine_arm64.go

pkg/sentry/platform/kvm/machine_arm64_unsafe.go

lubinsz · 2019-10-22T06:56:38Z

Hi @amscanne @avagin
Any comments?

Please also review the related patches:
guest: #918
pagetable: #891

Thanks.

There are 4 jobs were finished in this package: 1, Virtual machine initialization. 2, Bluepill implementation. 3, Move ring0.Vectors() into the address with 11-bits alignment. 4, Basic support for "SwitchToUser". Signed-off-by: Bin Lu <bin.lu@arm.com> FUTURE_COPYBARA_INTEGRATE_REVIEW=#919 from lubinszARM:pr_kvm eedea52db451bf62722759009a9f14c54a69c55f PiperOrigin-RevId: 280695497

avagin · 2019-11-20T22:51:33Z

pkg/sentry/platform/kvm/bluepill_arm64.go

+//
+//go:nosplit
+func (c *vCPU) die(context *arch.SignalContext64, msg string) {
+	// TODO.


Please reference the ARM64 bug in TODOs and the same for all other TODO-s

avagin · 2019-11-20T22:52:12Z

pkg/sentry/platform/kvm/kvm_amd64.go

@@ -211,3 +212,10 @@ type cpuidEntries struct {
 	_       uint32
 	entries [_KVM_NR_CPUID_ENTRIES]cpuidEntry
 }
+
+func updateGolbalOnce(fd int) error {


avagin · 2019-11-20T22:52:39Z

pkg/sentry/platform/kvm/machine_arm64.go

+		fallthrough
+	case _ESR_SEGV_MAPERR_L2:
+		fallthrough
+	case _ESR_SEGV_MAPERR_L3:


case _ESR_SEGV_MAPERR_L0, _ESR_SEGV_MAPERR_L1, _ESR_SEGV_MAPERR_L2, _ESR_SEGV_MAPERR_L3:

avagin · 2019-11-20T22:53:14Z

pkg/sentry/platform/kvm/machine_arm64_unsafe.go

+	return (*(*[0xFFFFFF]byte)(unsafe.Pointer(p & ^uintptr(syscall.Getpagesize()-1))))[:syscall.Getpagesize()]
+}
+
+func RawMemoryAccessWithReflect(p uintptr) []byte {


Why does this exist, it is unused.

avagin · 2019-11-20T22:55:16Z

pkg/sentry/platform/kvm/machine_arm64_unsafe.go

+	return (*(*[0x800]byte)(unsafe.Pointer(p)))[:]
+}
+
+// Work around: move ring0.Vectors() into the address with 11-bits alignment.


This function needs much better explanation of what it does, why it exists, and ideally avoid rewriting the binary...

avagin · 2019-11-20T22:58:20Z

pkg/sentry/platform/kvm/machine_arm64_unsafe.go

+	}
+
+	page = getPageWithReflect(toLocation + 4096)
+	if err := syscall.Mprotect(page, syscall.PROT_READ|syscall.PROT_WRITE|syscall.PROT_EXEC); err != nil {


can we remove PROT_WRITE before exiting from this function?

There are 4 jobs were finished in this package: 1, Virtual machine initialization. 2, Bluepill implementation. 3, Move ring0.Vectors() into the address with 11-bits alignment. 4, Basic support for "SwitchToUser". Signed-off-by: Bin Lu <bin.lu@arm.com> FUTURE_COPYBARA_INTEGRATE_REVIEW=#919 from lubinszARM:pr_kvm eedea52db451bf62722759009a9f14c54a69c55f PiperOrigin-RevId: 280695497

There are 4 jobs were finished in this package: 1, Virtual machine initialization. 2, Bluepill implementation. 3, Move ring0.Vectors() into the address with 11-bits alignment. 4, Basic support for "SwitchToUser". Signed-off-by: Bin Lu <bin.lu@arm.com> FUTURE_COPYBARA_INTEGRATE_REVIEW=#919 from lubinszARM:pr_kvm 0241d74f2006b408b79a21105bd428dbeaa463a7 PiperOrigin-RevId: 280691360

There are 4 jobs were finished in this package: 1, Virtual machine initialization. 2, Bluepill implementation. 3, Move ring0.Vectors() into the address with 11-bits alignment. 4, Basic support for "SwitchToUser". Signed-off-by: Bin Lu <bin.lu@arm.com> FUTURE_COPYBARA_INTEGRATE_REVIEW=#919 from lubinszARM:pr_kvm eedea52db451bf62722759009a9f14c54a69c55f PiperOrigin-RevId: 280695497

prattmic

Hi @lubinszARM, it looks like you may have forgotten to push the latest version of this with fixes to @amscanne's comments?

prattmic · 2019-12-10T22:32:31Z

pkg/sentry/platform/kvm/bluepill.go

-	dieArchSetup(c, context, &c.dieState.guestRegs)
-}
-
-func init() {


Looks like this isn't done?

prattmic · 2019-12-10T22:33:05Z

pkg/sentry/platform/kvm/bluepill_arm64.go

+)
+
+var (
+	// bounceSignal is the signal used for bouncing KVM.


prattmic · 2019-12-10T22:33:49Z

pkg/sentry/platform/kvm/bluepill_arm64_unsafe.go

+}
+
+//go:nosplit
+func dieArchSetup(c *vCPU, context *arch.SignalContext64, guestRegs *userRegs) {


prattmic · 2019-12-10T22:34:08Z

pkg/sentry/platform/kvm/kvm_amd64.go

@@ -211,3 +212,10 @@ type cpuidEntries struct {
 	_       uint32
 	entries [_KVM_NR_CPUID_ENTRIES]cpuidEntry
 }
+
+func UpdateGolbalOnce(fd int) error {


Global is misspelled.

lubinszARM

Done. Sorry for it.

lubinszARM · 2019-12-11T06:11:38Z

pkg/sentry/platform/kvm/kvm_amd64.go

@@ -211,3 +212,10 @@ type cpuidEntries struct {
 	_       uint32
 	entries [_KVM_NR_CPUID_ENTRIES]cpuidEntry
 }
+
+func UpdateGolbalOnce(fd int) error {


lubinszARM · 2019-12-11T06:43:34Z

pkg/sentry/platform/kvm/bluepill.go

-	dieArchSetup(c, context, &c.dieState.guestRegs)
-}
-
-func init() {


Looks like this isn't done?

Done. Sorry for it. :(

lubinszARM · 2019-12-11T06:45:49Z

pkg/sentry/platform/kvm/bluepill_arm64_unsafe.go

+}
+
+//go:nosplit
+func dieArchSetup(c *vCPU, context *arch.SignalContext64, guestRegs *userRegs) {


Not done?

Done. Please check it. Thanks.

prattmic · 2019-12-12T20:24:21Z

pkg/sentry/platform/kvm/machine_arm64_unsafe.go

+		offset = 1<<11 - offset
+	}
+
+	toLocation := fromLocation + offset


How do we ensure that this function doesn't clobber some other code at toLocation? It seems like ring0.Vectors should have something like a page of dummy data to reserve the space it needs.

How do we ensure that this function doesn't clobber some other code at toLocation? It seems like ring0.Vectors should have something like a page of dummy data to reserve the space it needs.

Hi @prattmic
In fact, the total size of 'exception-vectors-table' for Arm64 is required as 2K, please see the code of 'ENTRY(vectors)' in kernel(arch/arm64/kernel/entry.s) as reference.
For gvisor, I defined the function of 'exception-vectors-table' as 4K size. Please see the code in ring0/entry_arm64.s: Vectors(SB). I filled the 2nd 2K part with NOP(no operation) instructions.

And in the function of 'updateVectorTable()', I only memmove and modify the first 2K part of the binary.

Ah, I understand now. Could you add a comment to that function explaining the purpose of the extra NOPs? (In a separate PR, this one looks good for merging).

There are 4 jobs were finished in this package: 1, Virtual machine initialization. 2, Bluepill implementation. 3, Move ring0.Vectors() into the address with 11-bits alignment. 4, Basic support for "SwitchToUser". Signed-off-by: Bin Lu <bin.lu@arm.com>

There are 4 jobs were finished in this package: 1, Virtual machine initialization. 2, Bluepill implementation. 3, Move ring0.Vectors() into the address with 11-bits alignment. 4, Basic support for "SwitchToUser". Signed-off-by: Bin Lu <bin.lu@arm.com> COPYBARA_INTEGRATE_REVIEW=#919 from lubinszARM:pr_kvm eedea52db451bf62722759009a9f14c54a69c55f PiperOrigin-RevId: 285501256

avagin · 2019-12-14T05:33:56Z

6b42453

googlebot added the cla: yes CLA has been signed label Sep 26, 2019

avagin added the kokoro:run Presubmits may be run label Sep 27, 2019

gvisor-bot removed the kokoro:run Presubmits may be run label Sep 27, 2019

amscanne reviewed Oct 2, 2019

View reviewed changes

lubinszARM force-pushed the pr_kvm branch 3 times, most recently from 9511548 to 813879f Compare October 22, 2019 06:53

lubinszARM force-pushed the pr_kvm branch from 813879f to eedea52 Compare November 11, 2019 09:41

avagin added the kokoro:run Presubmits may be run label Nov 11, 2019

gvisor-bot removed the kokoro:run Presubmits may be run label Nov 11, 2019

avagin added the ready to pull label Nov 15, 2019

avagin reviewed Nov 20, 2019

View reviewed changes

lubinszARM force-pushed the pr_kvm branch from eedea52 to 0241d74 Compare November 21, 2019 10:05

avagin added the kokoro:run Presubmits may be run label Nov 21, 2019

gvisor-bot removed the kokoro:run Presubmits may be run label Nov 21, 2019

avagin added ready to pull and removed ready to pull labels Nov 21, 2019

lubinszARM mentioned this pull request Dec 6, 2019

enable pkg/sentry/arch to support arm64 basically #773

Closed

prattmic requested changes Dec 10, 2019

View reviewed changes

lubinszARM force-pushed the pr_kvm branch 3 times, most recently from cd4f4cc to 563a492 Compare December 11, 2019 06:45

lubinszARM commented Dec 11, 2019

View reviewed changes

avagin added ready to pull and removed ready to pull labels Dec 12, 2019

gvisor-bot mentioned this pull request Dec 12, 2019

enable kvm to support arm64 #1221

Merged

prattmic reviewed Dec 12, 2019

View reviewed changes

enable kvm to support arm64

c1e033d

There are 4 jobs were finished in this package: 1, Virtual machine initialization. 2, Bluepill implementation. 3, Move ring0.Vectors() into the address with 11-bits alignment. 4, Basic support for "SwitchToUser". Signed-off-by: Bin Lu <bin.lu@arm.com>

lubinszARM force-pushed the pr_kvm branch from 563a492 to c1e033d Compare December 13, 2019 02:55

prattmic approved these changes Dec 13, 2019

View reviewed changes

avagin closed this Dec 14, 2019

prattmic added the arch: arm Issue related to the ARM architecture. label Jan 21, 2020

prattmic added this to the arm64 Support milestone Jan 22, 2020

enable kvm to support arm64 #919

enable kvm to support arm64 #919

Conversation

lubinszARM commented Sep 26, 2019

amscanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubinsz Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubinsz Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubinsz commented Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prattmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubinszARM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubinszARM Dec 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avagin commented Dec 14, 2019

lubinsz Oct 21, 2019 •

edited

Loading

lubinsz Oct 21, 2019 •

edited

Loading

lubinsz commented Oct 22, 2019 •

edited

Loading

lubinszARM Dec 13, 2019 •

edited

Loading