[hypervisor] Traps should not cancel other packets
Traps should only cancel port packets that are associated with the trap
on tear-down, and not cancel other packets with the same key.
ZX-4221 #done
Change-Id: I1da9b0cd051db58bb03a7d2571819f00e27b5408
diff --git a/zircon/kernel/lib/hypervisor/trap_map.cpp b/zircon/kernel/lib/hypervisor/trap_map.cpp
index b207df7..8e34b09 100644
--- a/zircon/kernel/lib/hypervisor/trap_map.cpp
+++ b/zircon/kernel/lib/hypervisor/trap_map.cpp
@@ -34,7 +34,7 @@
}
PortPacket* BlockingPortAllocator::Alloc() {
- return arena_.New(nullptr, this);
+ return arena_.New(this /* handle */, this /* allocator */);
}
void BlockingPortAllocator::Free(PortPacket* port_packet) {
@@ -44,15 +44,13 @@
Trap::Trap(uint32_t kind, zx_gpaddr_t addr, size_t len, fbl::RefPtr<PortDispatcher> port,
uint64_t key)
- : kind_(kind), addr_(addr), len_(len), port_(ktl::move(port)), key_(key) {
- (void) key_;
-}
+ : kind_(kind), addr_(addr), len_(len), port_(ktl::move(port)), key_(key) {}
Trap::~Trap() {
if (port_ == nullptr) {
return;
}
- port_->CancelQueued(nullptr /* handle */, key_);
+ port_->CancelQueued(&port_allocator_ /* handle */, key_);
}
zx_status_t Trap::Init() {
diff --git a/zircon/kernel/object/port_dispatcher.cpp b/zircon/kernel/object/port_dispatcher.cpp
index dc74218..0c0bfef 100644
--- a/zircon/kernel/object/port_dispatcher.cpp
+++ b/zircon/kernel/object/port_dispatcher.cpp
@@ -82,11 +82,6 @@
PortPacket::PortPacket(const void* handle, PortAllocator* allocator)
: packet{}, handle(handle), observer(nullptr), allocator(allocator) {
// Note that packet is initialized to zeros.
- if (handle) {
- // Currently |handle| is only valid if the packets are not ephemeral
- // which means that PortObserver always uses the kernel heap.
- DEBUG_ASSERT(allocator == nullptr);
- }
}
PortObserver::PortObserver(uint32_t type, const Handle* handle, fbl::RefPtr<PortDispatcher> port,
diff --git a/zircon/system/utest/hypervisor/guest.cpp b/zircon/system/utest/hypervisor/guest.cpp
index 160799ef..f75c2d7 100644
--- a/zircon/system/utest/hypervisor/guest.cpp
+++ b/zircon/system/utest/hypervisor/guest.cpp
@@ -77,20 +77,19 @@
typedef struct test {
bool supported = false;
bool interrupts_enabled = false;
+ uintptr_t host_addr = 0;
zx::vmo vmo;
- uintptr_t host_addr;
zx::guest guest;
zx::vmar vmar;
zx::vcpu vcpu;
-} test_t;
-static bool teardown(test_t* test) {
- BEGIN_HELPER;
- ASSERT_EQ(zx::vmar::root_self()->unmap(test->host_addr, VMO_SIZE), ZX_OK);
- return true;
- END_HELPER;
-}
+ ~test() {
+ if (host_addr != 0) {
+ zx::vmar::root_self()->unmap(host_addr, VMO_SIZE);
+ }
+ }
+} test_t;
// TODO(MAC-246): Convert to C++ FIDL interface.
static zx_status_t get_sysinfo(zx::channel* sysinfo) {
@@ -139,6 +138,8 @@
#endif // __aarch64__
static bool setup(test_t* test, const char* start, const char* end) {
+ BEGIN_HELPER;
+
ASSERT_EQ(zx::vmo::create(VMO_SIZE, 0, &test->vmo), ZX_OK);
ASSERT_EQ(
zx::vmar::root_self()->map(0, test->vmo, 0, VMO_SIZE, kHostMapFlags, &test->host_addr),
@@ -154,7 +155,7 @@
test->supported = status != ZX_ERR_NOT_SUPPORTED;
if (!test->supported) {
fprintf(stderr, "Guest creation not supported\n");
- return teardown(test);
+ return true;
}
ASSERT_EQ(status, ZX_OK);
@@ -180,11 +181,11 @@
test->supported = status != ZX_ERR_NOT_SUPPORTED;
if (!test->supported) {
fprintf(stderr, "VCPU creation not supported\n");
- return teardown(test);
+ return true;
}
ASSERT_EQ(status, ZX_OK);
- return true;
+ END_HELPER;
}
static bool setup_and_interrupt(test_t* test, const char* start, const char* end) {
@@ -261,7 +262,6 @@
}
ASSERT_TRUE(resume_and_clean_exit(&test));
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -369,7 +369,6 @@
EXPECT_EQ(vcpu_state.rflags, (1u << 0) | (1u << 18));
#endif // __x86_64__
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -399,8 +398,6 @@
EXPECT_EQ(vcpu_state.rax, kInterruptVector);
#endif
- ASSERT_TRUE(teardown(&test));
-
END_TEST;
}
@@ -424,7 +421,6 @@
EXPECT_EQ(packet.type, ZX_PKT_TYPE_GUEST_MEM);
ASSERT_TRUE(resume_and_clean_exit(&test));
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -455,8 +451,6 @@
EXPECT_EQ(packet.type, ZX_PKT_TYPE_GUEST_BELL);
EXPECT_EQ(packet.guest_bell.addr, TRAP_ADDR);
- ASSERT_TRUE(teardown(&test));
-
END_TEST;
}
@@ -483,14 +477,50 @@
EXPECT_EQ(packet.type, ZX_PKT_TYPE_GUEST_MEM);
EXPECT_EQ(packet.guest_mem.addr, EXIT_TEST_ADDR);
- ASSERT_TRUE(teardown(&test));
-
// The guest in test is destructed with one packet still queued on the
// port. This should work correctly.
END_TEST;
}
+// Test for ZX-4221.
+static bool guest_set_trap_with_bell_and_user() {
+ BEGIN_TEST;
+
+ zx::port port;
+ ASSERT_EQ(zx::port::create(0, &port), ZX_OK);
+
+ // Queue a packet with the same key as the trap.
+ zx_port_packet packet = {};
+ packet.key = kTrapKey;
+ packet.type = ZX_PKT_TYPE_USER;
+ ASSERT_EQ(port.queue(&packet), ZX_OK);
+
+ // Force guest to be released and cancel all packets associated with traps.
+ {
+ test_t test;
+ ASSERT_TRUE(setup(&test, guest_set_trap_start, guest_set_trap_end));
+ if (!test.supported) {
+ // The hypervisor isn't supported, so don't run the test.
+ return true;
+ }
+
+ // Trap on access of TRAP_ADDR.
+ ASSERT_EQ(test.guest.set_trap(ZX_GUEST_TRAP_BELL, TRAP_ADDR, PAGE_SIZE, port, kTrapKey),
+ ZX_OK);
+
+ ASSERT_EQ(test.vcpu.resume(&packet), ZX_OK);
+ EXPECT_EQ(packet.type, ZX_PKT_TYPE_GUEST_MEM);
+ EXPECT_EQ(packet.guest_mem.addr, EXIT_TEST_ADDR);
+ }
+
+ ASSERT_EQ(port.wait(zx::time::infinite(), &packet), ZX_OK);
+ EXPECT_EQ(packet.key, kTrapKey);
+ EXPECT_EQ(packet.type, ZX_PKT_TYPE_USER);
+
+ END_TEST;
+}
+
#ifdef __aarch64__
static bool vcpu_wfi() {
@@ -504,7 +534,6 @@
}
ASSERT_TRUE(resume_and_clean_exit(&test));
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -538,7 +567,6 @@
ASSERT_EQ(test.vcpu.interrupt(kInterruptVector + 1), ZX_OK);
ASSERT_TRUE(resume_and_clean_exit(&test));
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -560,8 +588,6 @@
EXPECT_EQ(packet.guest_mem.read, false);
EXPECT_EQ(packet.guest_mem.data, 0);
- ASSERT_TRUE(teardown(&test));
-
END_TEST;
}
@@ -576,7 +602,6 @@
}
ASSERT_TRUE(resume_and_clean_exit(&test));
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -598,8 +623,6 @@
EXPECT_EQ(packet.guest_mem.read, false);
EXPECT_EQ(packet.guest_mem.data, 0);
- ASSERT_TRUE(teardown(&test));
-
END_TEST;
}
@@ -629,8 +652,6 @@
// TODO(MAC-225): Check that the exception is cleared.
- ASSERT_TRUE(teardown(&test));
-
END_TEST;
}
@@ -654,7 +675,6 @@
zx_vcpu_state_t vcpu_state;
ASSERT_EQ(test.vcpu.read_state(ZX_VCPU_STATE, &vcpu_state, sizeof(vcpu_state)), ZX_OK);
EXPECT_EQ(vcpu_state.rax, kNmiVector);
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -683,8 +703,6 @@
// TODO(MAC-225): Check that the interrupt is queued.
- ASSERT_TRUE(teardown(&test));
-
END_TEST;
}
@@ -708,7 +726,6 @@
zx_vcpu_state_t vcpu_state;
ASSERT_EQ(test.vcpu.read_state(ZX_VCPU_STATE, &vcpu_state, sizeof(vcpu_state)), ZX_OK);
EXPECT_EQ(vcpu_state.rax, kExceptionVector);
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -724,7 +741,6 @@
}
ASSERT_TRUE(resume_and_clean_exit(&test));
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -740,7 +756,6 @@
}
ASSERT_TRUE(resume_and_clean_exit(&test));
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -762,8 +777,6 @@
// Check that cr0 has the NE bit set when read.
EXPECT_TRUE(vcpu_state.rax & X86_CR0_NE);
- ASSERT_TRUE(teardown(&test));
-
END_TEST;
}
@@ -786,8 +799,6 @@
EXPECT_EQ(vcpu_state.rcx, 2u);
#endif
- ASSERT_TRUE(teardown(&test));
-
END_TEST;
}
@@ -802,7 +813,6 @@
}
ASSERT_TRUE(resume_and_clean_exit(&test));
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -818,7 +828,6 @@
}
ASSERT_TRUE(resume_and_clean_exit(&test));
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -834,7 +843,6 @@
}
ASSERT_TRUE(resume_and_clean_exit(&test));
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -857,8 +865,6 @@
const uint64_t kVmCallNoSys = -1000;
EXPECT_EQ(vcpu_state.rax, kVmCallNoSys);
- ASSERT_TRUE(teardown(&test));
-
END_TEST;
}
@@ -898,8 +904,6 @@
// Guest successfully runs again
ASSERT_TRUE(resume_and_clean_exit(&test));
- ASSERT_TRUE(teardown(&test));
-
END_TEST;
}
@@ -923,7 +927,6 @@
EXPECT_EQ(packet.guest_io.port, TRAP_PORT);
ASSERT_TRUE(resume_and_clean_exit(&test));
- ASSERT_TRUE(teardown(&test));
END_TEST;
}
@@ -937,6 +940,7 @@
RUN_TEST(guest_set_trap_with_mem)
RUN_TEST(guest_set_trap_with_bell)
RUN_TEST(guest_set_trap_with_bell_drop)
+RUN_TEST(guest_set_trap_with_bell_and_user)
#if __aarch64__
RUN_TEST(vcpu_wfi)
RUN_TEST(vcpu_wfi_pending_interrupt)
