Zero allocated pages

PATCH In review Nadav Amit 3 participants 9 messages May 10, 2019 – May 15, 2019

Subsystems (2)

KVM Library code

lore

Share & tools

lore

Nadav Amit

author

2019-05-10 03:26 UTC #0 lore raw

v3 cover Zero allocated pages

For reproducibility, it is best to zero pages before they are used. There are hidden assumptions on memory being zeroed (by BIOS/KVM), which might be broken at any given moment. The full argument appears in the first patch commit log.

Following the first patch that zeros the memory, the rest of the patch-set removes redundant zeroing do to the additional zeroing.

This patch-set is only tested on x86.

v2->v3:
* Typos [Alexandru]

v1->v2:
* Change alloc_pages() as well
* Remove redundant page zeroing [Andrew]

Nadav Amit (4):
lib/alloc_page: Zero allocated pages
x86: Remove redundant page zeroing
lib: Remove redundant page zeroing
arm: Remove redundant page zeroing

 lib/alloc_page.c         |  4 ++++
 lib/arm/asm/pgtable.h    |  2 --
 lib/arm/mmu.c            |  1 -
 lib/arm64/asm/pgtable.h  |  1 -
 lib/virtio-mmio.c        |  1 -
 lib/x86/intel-iommu.c    |  5 -----
 x86/eventinj.c           |  1 -
 x86/hyperv_connections.c |  4 ----
 x86/vmx.c                | 10 ----------
 x86/vmx_tests.c          | 11 -----------
 10 files changed, 4 insertions(+), 36 deletions(-)

Signature · 1 line

2.17.1

Nadav Amit

author

2019-05-10 03:26 UTC #1 lore raw

PATCH v3 1/4 lib/alloc_page: Zero allocated pages

One of the most important properties of tests is reproducibility. For tests to be reproducible, the same environment should be set on each test invocation.

When it comes to memory content, this is not exactly the case in kvm-unit-tests. The tests might, mistakenly or intentionally, assume that memory is zeroed (by the BIOS or KVM). However, failures might not be reproducible if this assumption is broken.

As an example, consider x86 do_iret(), which mistakenly does not push SS:RSP onto the stack on 64-bit mode, although they are popped unconditionally on iret.

Do not assume that memory is zeroed. Clear it once it is allocated to allow tests to easily be reproduced.

Reviewed-by Krish Sadhukhan <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
 lib/alloc_page.c | 4 ++++
 1 file changed, 4 insertions(+)

lib/alloc_page.c +4 -0

index 730f2b5..97d1339 100644
@@ -65,6 +65,8 @@ void *alloc_page()
 	freelist = *(void **)freelist;
 	spin_unlock(&lock);
 
+	if (p)
+		memset(p, 0, PAGE_SIZE);
 	return p;
 }

@@ -126,6 +128,8 @@ void *alloc_pages(unsigned long order)
 		}
 	}
 	spin_unlock(&lock);
+	if (run_start)
+		memset(run_start, 0, n * PAGE_SIZE);
 	return run_start;
 }

Signature · 1 line

2.17.1

Andrew Jones

Reviewed-by on v3 1/4

2019-05-15 09:18 UTC #2 lore raw

Re: [kvm-unit-tests PATCH v3 1/4] lib/alloc_page: Zero allocated pages

Quoted 47 lines · Nadav Amit

One of the most important properties of tests is reproducibility. For tests to be reproducible, the same environment should be set on each test invocation.
When it comes to memory content, this is not exactly the case in kvm-unit-tests. The tests might, mistakenly or intentionally, assume that memory is zeroed (by the BIOS or KVM). However, failures might not be reproducible if this assumption is broken.
As an example, consider x86 do_iret(), which mistakenly does not push SS:RSP onto the stack on 64-bit mode, although they are popped unconditionally on iret.
Do not assume that memory is zeroed. Clear it once it is allocated to allow tests to easily be reproduced.
Reviewed-by Krish Sadhukhan <[email protected]>
Signed-off-by Nadav Amit <[email protected]>
---
 lib/alloc_page.c | 4 ++++
 1 file changed, 4 insertions(+)
lib/alloc_page.c +4 -0
index 730f2b5..97d1339 100644
@@ -65,6 +65,8 @@ void *alloc_page()
 	freelist = *(void **)freelist;
 	spin_unlock(&lock);
 
+	if (p)
+		memset(p, 0, PAGE_SIZE);
 	return p;
 }
 
@@ -126,6 +128,8 @@ void *alloc_pages(unsigned long order)
 		}
 	}
 	spin_unlock(&lock);
+	if (run_start)
+		memset(run_start, 0, n * PAGE_SIZE);
 	return run_start;
 }
 
Signature · 1 line
2.17.1

Reviewed-by Andrew Jones <[email protected]>

Nadav Amit

author

2019-05-10 03:26 UTC #3 lore raw

PATCH v3 2/4 x86: Remove redundant page zeroing

In reply to Nadav Amit

Now that alloc_page() zeros the page, remove the redundant page zeroing.

Reviewed-by Krish Sadhukhan <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
 lib/x86/intel-iommu.c    |  5 -----
 x86/eventinj.c           |  1 -
 x86/hyperv_connections.c |  4 ----
 x86/vmx.c                | 10 ----------
 x86/vmx_tests.c          | 11 -----------
 5 files changed, 31 deletions(-)

lib/x86/intel-iommu.c +0 -5

index 3f3f211..c811ba5 100644
@@ -125,7 +125,6 @@ static void vtd_setup_root_table(void)
 {
 	void *root = alloc_page();
 
-	memset(root, 0, PAGE_SIZE);
 	vtd_writeq(DMAR_RTADDR_REG, virt_to_phys(root));
 	vtd_gcmd_or(VTD_GCMD_ROOT);
 	printf("DMAR table address: %#018lx\n", vtd_root_table());

@@ -135,7 +134,6 @@ static void vtd_setup_ir_table(void)
 {
 	void *root = alloc_page();
 
-	memset(root, 0, PAGE_SIZE);
 	/* 0xf stands for table size (2^(0xf+1) == 65536) */
 	vtd_writeq(DMAR_IRTA_REG, virt_to_phys(root) | 0xf);
 	vtd_gcmd_or(VTD_GCMD_IR_TABLE);

@@ -153,7 +151,6 @@ static void vtd_install_pte(vtd_pte_t *root, iova_t iova,
 		offset = PGDIR_OFFSET(iova, level);
 		if (!(root[offset] & VTD_PTE_RW)) {
 			page = alloc_page();
-			memset(page, 0, PAGE_SIZE);
 			root[offset] = virt_to_phys(page) | VTD_PTE_RW;
 		}
 		root = (uint64_t *)(phys_to_virt(root[offset] &

@@ -195,7 +192,6 @@ void vtd_map_range(uint16_t sid, iova_t iova, phys_addr_t pa, size_t size)
 
 	if (!re->present) {
 		ce = alloc_page();
-		memset(ce, 0, PAGE_SIZE);
 		memset(re, 0, sizeof(*re));
 		re->context_table_p = virt_to_phys(ce) >> VTD_PAGE_SHIFT;
 		re->present = 1;

@@ -209,7 +205,6 @@ void vtd_map_range(uint16_t sid, iova_t iova, phys_addr_t pa, size_t size)
 
 	if (!ce->present) {
 		slptptr = alloc_page();
-		memset(slptptr, 0, PAGE_SIZE);
 		memset(ce, 0, sizeof(*ce));
 		/* To make it simple, domain ID is the same as SID */
 		ce->domain_id = sid;

x86/eventinj.c +0 -1

index 8064eb9..d2dfc40 100644
@@ -398,7 +398,6 @@ int main(void)
 
 	pt = alloc_page();
 	cr3 = (void*)read_cr3();
-	memset(pt, 0, 4096);
 	/* use shadowed stack during interrupt delivery */
 	for (i = 0; i < 4096/sizeof(ulong); i++) {
 		if (!cr3[i]) {

x86/hyperv_connections.c +0 -4

index 5d541c9..8eade41 100644
@@ -47,7 +47,6 @@ static void setup_hypercall(void)
 	hypercall_page = alloc_page();
 	if (!hypercall_page)
 		report_abort("failed to allocate hypercall page");
-	memset(hypercall_page, 0, PAGE_SIZE);
 
 	wrmsr(HV_X64_MSR_GUEST_OS_ID, guestid);

@@ -105,9 +104,6 @@ static void setup_cpu(void *ctx)
 	hv->post_msg = alloc_page();
 	if (!hv->msg_page || !hv->evt_page || !hv->post_msg)
 		report_abort("failed to allocate synic pages for vcpu");
-	memset(hv->msg_page, 0, sizeof(*hv->msg_page));
-	memset(hv->evt_page, 0, sizeof(*hv->evt_page));
-	memset(hv->post_msg, 0, sizeof(*hv->post_msg));
 	hv->msg_conn = MSG_CONN_BASE + vcpu;
 	hv->evt_conn = EVT_CONN_BASE + vcpu;

x86/vmx.c +0 ~~-10~~

index be47800..962ec0f 100644
@@ -353,7 +353,6 @@ static void test_vmwrite_vmread(void)
 	struct vmcs *vmcs = alloc_page();
 	u32 vmcs_enum_max, max_index = 0;
 
-	memset(vmcs, 0, PAGE_SIZE);
 	vmcs->hdr.revision_id = basic.revision;
 	assert(!vmcs_clear(vmcs));
 	assert(!make_vmcs_current(vmcs));

@@ -373,7 +372,6 @@ static void test_vmcs_high(void)
 {
 	struct vmcs *vmcs = alloc_page();
 
-	memset(vmcs, 0, PAGE_SIZE);
 	vmcs->hdr.revision_id = basic.revision;
 	assert(!vmcs_clear(vmcs));
 	assert(!make_vmcs_current(vmcs));

@@ -400,7 +398,6 @@ static void test_vmcs_lifecycle(void)
 
 	for (i = 0; i < ARRAY_SIZE(vmcs); i++) {
 		vmcs[i] = alloc_page();
-		memset(vmcs[i], 0, PAGE_SIZE);
 		vmcs[i]->hdr.revision_id = basic.revision;
 	}

@@ -647,7 +644,6 @@ static void test_vmclear_flushing(void)
 
 	for (i = 0; i < ARRAY_SIZE(vmcs); i++) {
 		vmcs[i] = alloc_page();
-		memset(vmcs[i], 0, PAGE_SIZE);
 	}
 
 	vmcs[0]->hdr.revision_id = basic.revision;

@@ -745,7 +741,6 @@ static void split_large_ept_entry(unsigned long *ptep, int level)
 
 	new_pt = alloc_page();
 	assert(new_pt);
-	memset(new_pt, 0, PAGE_SIZE);
 
 	prototype = pte & ~EPT_ADDR_MASK;
 	if (level == 2)

@@ -1220,7 +1215,6 @@ static void init_vmcs_guest(void)
 static int init_vmcs(struct vmcs **vmcs)
 {
 	*vmcs = alloc_page();
-	memset(*vmcs, 0, PAGE_SIZE);
 	(*vmcs)->hdr.revision_id = basic.revision;
 	/* vmclear first to init vmcs */
 	if (vmcs_clear(*vmcs)) {

@@ -1259,7 +1253,6 @@ static void init_vmx(void)
 	ulong fix_cr4_set, fix_cr4_clr;
 
 	vmxon_region = alloc_page();
-	memset(vmxon_region, 0, PAGE_SIZE);
 
 	vmcs_root = alloc_page();

@@ -1291,9 +1284,7 @@ static void init_vmx(void)
 	*vmxon_region = basic.revision;
 
 	guest_stack = alloc_page();
-	memset(guest_stack, 0, PAGE_SIZE);
 	guest_syscall_stack = alloc_page();
-	memset(guest_syscall_stack, 0, PAGE_SIZE);
 }
 
 static void do_vmxon_off(void *data)

@@ -1420,7 +1411,6 @@ static void test_vmptrst(void)
 	struct vmcs *vmcs1, *vmcs2;
 
 	vmcs1 = alloc_page();
-	memset(vmcs1, 0, PAGE_SIZE);
 	init_vmcs(&vmcs1);
 	ret = vmcs_save(&vmcs2);
 	report("test vmptrst", (!ret) && (vmcs1 == vmcs2));

x86/vmx_tests.c +0 ~~-11~~

index 2d6b12d..c52ebc6 100644
@@ -253,7 +253,6 @@ static void msr_bmp_init(void)
 	u32 ctrl_cpu0;
 
 	msr_bitmap = alloc_page();
-	memset(msr_bitmap, 0x0, PAGE_SIZE);
 	ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
 	ctrl_cpu0 |= CPU_MSR_BITMAP;
 	vmcs_write(CPU_EXEC_CTRL0, ctrl_cpu0);

@@ -627,8 +626,6 @@ static int iobmp_init(struct vmcs *vmcs)
 
 	io_bitmap_a = alloc_page();
 	io_bitmap_b = alloc_page();
-	memset(io_bitmap_a, 0x0, PAGE_SIZE);
-	memset(io_bitmap_b, 0x0, PAGE_SIZE);
 	ctrl_cpu0 = vmcs_read(CPU_EXEC_CTRL0);
 	ctrl_cpu0 |= CPU_IO_BITMAP;
 	ctrl_cpu0 &= (~CPU_IO);

@@ -1062,8 +1059,6 @@ static int setup_ept(bool enable_ad)
 	if (__setup_ept(virt_to_phys(pml4), enable_ad))
 		return 1;
 
-	memset(pml4, 0, PAGE_SIZE);
-
 	end_of_memory = fwcfg_get_u64(FW_CFG_RAM_SIZE);
 	if (end_of_memory < (1ul << 32))
 		end_of_memory = (1ul << 32);

@@ -1135,8 +1130,6 @@ static int ept_init_common(bool have_ad)
 		return VMX_TEST_EXIT;
 	data_page1 = alloc_page();
 	data_page2 = alloc_page();
-	memset(data_page1, 0x0, PAGE_SIZE);
-	memset(data_page2, 0x0, PAGE_SIZE);
 	*((u32 *)data_page1) = MAGIC_VAL_1;
 	*((u32 *)data_page2) = MAGIC_VAL_2;
 	install_ept(pml4, (unsigned long)data_page1, (unsigned long)data_page2,

@@ -1483,7 +1476,6 @@ static int pml_init(struct vmcs *vmcs)
 	}
 
 	pml_log = alloc_page();
-	memset(pml_log, 0x0, PAGE_SIZE);
 	vmcs_write(PMLADDR, (u64)pml_log);
 	vmcs_write(GUEST_PML_INDEX, PML_INDEX - 1);

@@ -1908,9 +1900,6 @@ static int msr_switch_init(struct vmcs *vmcs)
 	exit_msr_store = alloc_page();
 	exit_msr_load = alloc_page();
 	entry_msr_load = alloc_page();
-	memset(exit_msr_store, 0, PAGE_SIZE);
-	memset(exit_msr_load, 0, PAGE_SIZE);
-	memset(entry_msr_load, 0, PAGE_SIZE);
 	entry_msr_load[0].index = MSR_KERNEL_GS_BASE;
 	entry_msr_load[0].value = MSR_MAGIC;

Signature · 1 line

2.17.1

Nadav Amit

author

2019-05-10 03:26 UTC #4 lore raw

PATCH v3 3/4 lib: Remove redundant page zeroing

In reply to Nadav Amit

Now that alloc_page() zeros the page, remove the redundant page zeroing.

Reviewed-by Krish Sadhukhan <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
 lib/virtio-mmio.c | 1 -
 1 file changed, 1 deletion(-)

lib/virtio-mmio.c +0 -1

index 57fe78e..e5e8f66 100644
@@ -56,7 +56,6 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev,
 	vq = calloc(1, sizeof(*vq));
 	assert(VIRTIO_MMIO_QUEUE_SIZE_MIN <= 2*PAGE_SIZE);
 	queue = alloc_pages(1);
-	memset(queue, 0, 2*PAGE_SIZE);
 	assert(vq && queue);
 
 	writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);

Signature · 1 line

2.17.1

Andrew Jones

Reviewed-by on v3 3/4

2019-05-15 09:20 UTC #5 lore raw

Re: [kvm-unit-tests PATCH v3 3/4] lib: Remove redundant page zeroing

Quoted 24 lines · Nadav Amit

Now that alloc_page() zeros the page, remove the redundant page zeroing.
Reviewed-by Krish Sadhukhan <[email protected]>
Suggested-by Andrew Jones <[email protected]>
Signed-off-by Nadav Amit <[email protected]>
---
 lib/virtio-mmio.c | 1 -
 1 file changed, 1 deletion(-)
lib/virtio-mmio.c +0 -1
index 57fe78e..e5e8f66 100644
@@ -56,7 +56,6 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev,
 	vq = calloc(1, sizeof(*vq));
 	assert(VIRTIO_MMIO_QUEUE_SIZE_MIN <= 2*PAGE_SIZE);
 	queue = alloc_pages(1);
-	memset(queue, 0, 2*PAGE_SIZE);
 	assert(vq && queue);
 
 	writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
Signature · 1 line
2.17.1

Reviewed-by Andrew Jones <[email protected]>

Nadav Amit

author

2019-05-10 03:26 UTC #6 lore raw

PATCH v3 4/4 arm: Remove redundant page zeroing

In reply to Nadav Amit

Now that alloc_page() zeros the page, remove the redundant page zeroing.

Reviewed-by Alexandru Elisei <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
 lib/arm/asm/pgtable.h   | 2 --
 lib/arm/mmu.c           | 1 -
 lib/arm64/asm/pgtable.h | 1 -
 3 files changed, 4 deletions(-)

lib/arm/asm/pgtable.h +0 -2

index b614bce..241dff6 100644
@@ -53,7 +53,6 @@ static inline pmd_t *pmd_alloc_one(void)
 {
 	assert(PTRS_PER_PMD * sizeof(pmd_t) == PAGE_SIZE);
 	pmd_t *pmd = alloc_page();
-	memset(pmd, 0, PTRS_PER_PMD * sizeof(pmd_t));
 	return pmd;
 }
 static inline pmd_t *pmd_alloc(pgd_t *pgd, unsigned long addr)

@@ -80,7 +79,6 @@ static inline pte_t *pte_alloc_one(void)
 {
 	assert(PTRS_PER_PTE * sizeof(pte_t) == PAGE_SIZE);
 	pte_t *pte = alloc_page();
-	memset(pte, 0, PTRS_PER_PTE * sizeof(pte_t));
 	return pte;
 }
 static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)

lib/arm/mmu.c +0 -1

index 03f6622..3d38c83 100644
@@ -166,7 +166,6 @@ void *setup_mmu(phys_addr_t phys_end)
 #endif
 
 	mmu_idmap = alloc_page();
-	memset(mmu_idmap, 0, PAGE_SIZE);
 
 	/*
 	 * mach-virt I/O regions:

lib/arm64/asm/pgtable.h +0 -1

index 5860abe..ee0a2c8 100644
@@ -61,7 +61,6 @@ static inline pte_t *pte_alloc_one(void)
 {
 	assert(PTRS_PER_PTE * sizeof(pte_t) == PAGE_SIZE);
 	pte_t *pte = alloc_page();
-	memset(pte, 0, PTRS_PER_PTE * sizeof(pte_t));
 	return pte;
 }
 static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)

Signature · 1 line

2.17.1

Andrew Jones

Reviewed-by on v3 4/4

2019-05-15 09:27 UTC #7 lore raw

Re: [kvm-unit-tests PATCH v3 4/4] arm: Remove redundant page zeroing

Quoted 58 lines · Nadav Amit

Now that alloc_page() zeros the page, remove the redundant page zeroing.

Reviewed-by Alexandru Elisei <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
 lib/arm/asm/pgtable.h   | 2 --
 lib/arm/mmu.c           | 1 -
 lib/arm64/asm/pgtable.h | 1 -
 3 files changed, 4 deletions(-)

lib/arm/asm/pgtable.h +0 -2

index b614bce..241dff6 100644
@@ -53,7 +53,6 @@ static inline pmd_t *pmd_alloc_one(void)
 {
 	assert(PTRS_PER_PMD * sizeof(pmd_t) == PAGE_SIZE);
 	pmd_t *pmd = alloc_page();
-	memset(pmd, 0, PTRS_PER_PMD * sizeof(pmd_t));
 	return pmd;
 }
 static inline pmd_t *pmd_alloc(pgd_t *pgd, unsigned long addr)

@@ -80,7 +79,6 @@ static inline pte_t *pte_alloc_one(void)
 {
 	assert(PTRS_PER_PTE * sizeof(pte_t) == PAGE_SIZE);
 	pte_t *pte = alloc_page();
-	memset(pte, 0, PTRS_PER_PTE * sizeof(pte_t));
 	return pte;
 }
 static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)

lib/arm/mmu.c +0 -1

index 03f6622..3d38c83 100644
@@ -166,7 +166,6 @@ void *setup_mmu(phys_addr_t phys_end)
 #endif
 
 	mmu_idmap = alloc_page();
-	memset(mmu_idmap, 0, PAGE_SIZE);
 
 	/*
 	 * mach-virt I/O regions:

lib/arm64/asm/pgtable.h +0 -1

index 5860abe..ee0a2c8 100644
@@ -61,7 +61,6 @@ static inline pte_t *pte_alloc_one(void)
 {
 	assert(PTRS_PER_PTE * sizeof(pte_t) == PAGE_SIZE);
 	pte_t *pte = alloc_page();
-	memset(pte, 0, PTRS_PER_PTE * sizeof(pte_t));
 	return pte;
 }
 static inline pte_t *pte_alloc(pmd_t *pmd, unsigned long addr)

Signature · 1 line

2.17.1

Reviewed-by Andrew Jones <[email protected]>

Konrad Rzeszutek Wilk

Reviewed-by on the v3 series

2019-05-10 12:56 UTC #8 lore raw

In reply to Nadav Amit

Quoted 9 lines · Nadav Amit

For reproducibility, it is best to zero pages before they are used. There are hidden assumptions on memory being zeroed (by BIOS/KVM), which might be broken at any given moment. The full argument appears in the first patch commit log.
Following the first patch that zeros the memory, the rest of the patch-set removes redundant zeroing do to the additional zeroing.
This patch-set is only tested on x86.

Reviewed-by Konrad Rzeszutek Wilk <[email protected]>

for all of them please.

Quoted 29 lines

v2->v3:
* Typos [Alexandru]
v1->v2:
* Change alloc_pages() as well
* Remove redundant page zeroing [Andrew]
Nadav Amit (4):
lib/alloc_page: Zero allocated pages
x86: Remove redundant page zeroing
lib: Remove redundant page zeroing
arm: Remove redundant page zeroing
 lib/alloc_page.c         |  4 ++++
 lib/arm/asm/pgtable.h    |  2 --
 lib/arm/mmu.c            |  1 -
 lib/arm64/asm/pgtable.h  |  1 -
 lib/virtio-mmio.c        |  1 -
 lib/x86/intel-iommu.c    |  5 -----
 x86/eventinj.c           |  1 -
 x86/hyperv_connections.c |  4 ----
 x86/vmx.c                | 10 ----------
 x86/vmx_tests.c          | 11 -----------
 10 files changed, 4 insertions(+), 36 deletions(-)
Signature · 1 line
2.17.1