Cleanup low-level arch code

PATCH Superseded Alexander Gordeev 2 participants 21 messages May 4, 2016 – May 5, 2016

Subsystems (2)

KVM Library code

lore

Share & tools

lore

Alexander Gordeev

author

2016-05-04 10:31 UTC #0 lore raw

v6 cover Cleanup low-level arch code

Hi Andrew et al,

This is 6th attempt to make mainly x86 arch code consistent with other architectures.

Changes since v5:
- ioremap() PCD comment added, the Reviewed-by preserved;
- page table flag names changed from PTE_* to PT_*_MASK;

Changes since v4:
- single instead of multiple pages allocation bug fixed in x86 ioremap();
- style fixes squashed into code movements for lib/x86/asm/page.h and

    lib/x86/asm/barrier.h;

- #ifndef __ASSEMBLY__ added to lib/x86/asm/page.h;

Changes since v3:
- x86 PAGE_* constants and PTE_* flags cleaned up;
- missing Reviewed-bys restored;
- missing headers to lib/x86/asm/page.h and lib/x86/asm/barriers.h added

    (I did not remove already given Reviewed-bys from these);

Changes since v2:
- x86 ioremap() address arithmetics nonsense fixed;
- x86 virt_to_phys() and phys_to_virt() implementations left intact;
- separate overrides of virt_to_phys() and phys_to_virt() added;
- unsigned long size specifier removed from PAGE_SIZE constant;

Changes since v1:
- arm compilation error fixed;
- disabling memory re-ordering for generic memory barriers added;

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Alexander Gordeev (11):
Remove unused and unnecessary PHYS32 macro
Move phys_addr_t type definition to lib/libcflat.h
x86: Introduce lib/x86/asm/page.h
x86: Introduce lib/x86/asm/io.h
x86: Introduce lib/x86/asm/barrier.h
io: Separate overrides of virt_to_phys() and phys_to_virt()
io: Disallow memory re-ordering for generic memory barriers
x86: Cleanup PAGE_* constants
x86: Cleanup PT_*_MASK flags
io: Make ioremap() prototype conform to Linux one
x86: io: Factor out ioremap()

 arm/selftest.c          |  1 -
 lib/alloc.h             |  5 -----
 lib/arm/asm/page.h      |  2 --
 lib/asm-generic/io.h    | 13 +++++++++----
 lib/libcflat.h          |  2 ++
 lib/powerpc/asm/setup.h |  1 -
 lib/ppc64/asm/io.h      |  2 ++
 lib/x86/asm/barrier.h   | 13 +++++++++++++
 lib/x86/{ => asm}/io.h  | 18 ++++++++++++++++--
 lib/x86/asm/page.h      | 44 ++++++++++++++++++++++++++++++++++++++++++++
 lib/x86/asm/pci.h       |  2 +-
 lib/x86/io.c            | 17 ++++++++++++++++-
 lib/x86/smp.h           |  4 ----
 lib/x86/vm.c            | 37 ++++++++++---------------------------
 lib/x86/vm.h            | 25 ++-----------------------
 x86/access.c            | 12 +-----------
 x86/asyncpf.c           |  2 +-
 x86/eventinj.c          | 17 ++++++-----------
 x86/hypercall.c         |  2 +-
 x86/hyperv.c            |  1 +
 x86/hyperv.h            |  1 -
 x86/hyperv_stimer.c     |  2 +-
 x86/hyperv_synic.c      |  1 -
 x86/init.c              |  2 +-
 x86/kvmclock.c          |  1 +
 x86/pku.c               |  2 +-
 x86/rmap_chain.c        |  2 +-
 x86/smap.c              |  6 +++---
 x86/svm.c               |  1 -
 x86/vmexit.c            | 11 ++---------
 x86/vmx.c               |  1 -
 x86/vmx.h               |  2 +-
 x86/vmx_tests.c         |  1 -
 33 files changed, 136 insertions(+), 117 deletions(-)
 create mode 100644 lib/x86/asm/barrier.h
 rename lib/x86/{ => asm}/io.h (74%)
 create mode 100644 lib/x86/asm/page.h

Signature · 1 line

1.8.3.1

Alexander Gordeev

author

2016-05-04 10:31 UTC #1 lore raw

PATCH v6 1/11 Remove unused and unnecessary PHYS32 macro

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/alloc.h | 4 ----
 1 file changed, 4 deletions(-)

lib/alloc.h +0 -4

index 7a73c18..0b4b4bd 100644
@@ -58,11 +58,7 @@ static inline void *memalign(size_t alignment, size_t size)
 	return alloc_ops->memalign(alignment, size);
 }
 
-#ifdef PHYS32
-typedef u32 phys_addr_t;
-#else
 typedef u64 phys_addr_t;
-#endif
 #define INVALID_PHYS_ADDR (~(phys_addr_t)0)
 
 /*

Signature · 1 line

1.8.3.1

Alexander Gordeev

author

2016-05-04 10:31 UTC #2 lore raw

PATCH v6 2/11 Move phys_addr_t type definition to lib/libcflat.h

In reply to Alexander Gordeev

This change leads to removing '#include <alloc.h>'s from several places that only included alloc.h to get the typedef.

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 arm/selftest.c          | 1 -
 lib/alloc.h             | 1 -
 lib/arm/asm/page.h      | 2 --
 lib/libcflat.h          | 2 ++
 lib/powerpc/asm/setup.h | 1 -
 5 files changed, 2 insertions(+), 5 deletions(-)

arm/selftest.c +0 -1

index 75dc91f..a8ae191 100644
@@ -7,7 +7,6 @@
  */
 #include <libcflat.h>
 #include <util.h>
-#include <alloc.h>
 #include <devicetree.h>
 #include <asm/setup.h>
 #include <asm/ptrace.h>

lib/alloc.h +0 -1

index 0b4b4bd..c12bd15 100644
@@ -58,7 +58,6 @@ static inline void *memalign(size_t alignment, size_t size)
 	return alloc_ops->memalign(alignment, size);
 }
 
-typedef u64 phys_addr_t;
 #define INVALID_PHYS_ADDR (~(phys_addr_t)0)
 
 /*

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

index df76969..3802641 100644
@@ -16,8 +16,6 @@
 
 #define PAGE_ALIGN(addr)	ALIGN(addr, PAGE_SIZE)
 
-#include <alloc.h>
-
 typedef u64 pteval_t;
 typedef u64 pmdval_t;
 typedef u64 pgdval_t;

lib/libcflat.h +2 -0

index b58a8a1..f2ec33f 100644
@@ -60,6 +60,8 @@ typedef _Bool		bool;
 #define PRIx64  __PRI64_PREFIX	"x"
 #define PRIxPTR __PRIPTR_PREFIX	"x"
 
+typedef u64			phys_addr_t;
+
 extern void puts(const char *s);
 extern void exit(int code);
 extern void abort(void);

lib/powerpc/asm/setup.h +0 -1

index 29a6d7d..b1e1e5a 100644
@@ -6,7 +6,6 @@
  * This work is licensed under the terms of the GNU LGPL, version 2.
  */
 #include <libcflat.h>
-#include <alloc.h>	/* phys_addr_t */
 
 #define NR_CPUS			8	/* arbitrarily set for now */
 extern u32 cpus[NR_CPUS];

Signature · 1 line

1.8.3.1

Alexander Gordeev

author

2016-05-04 10:31 UTC #3 lore raw

PATCH v6 3/11 x86: Introduce lib/x86/asm/page.h

In reply to Alexander Gordeev

Make x86 consistent with other architectures and put memory page specific defines to lib/x86/asm/page.h

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/x86/asm/page.h | 33 +++++++++++++++++++++++++++++++++
 lib/x86/vm.c       | 17 -----------------
 lib/x86/vm.h       | 14 +-------------
 3 files changed, 34 insertions(+), 30 deletions(-)
 create mode 100644 lib/x86/asm/page.h

lib/x86/asm/page.h new file +33 -0

new file mode 100644
index 0000000..d4580b4
@@ -0,0 +1,33 @@
+#ifndef _ASM_X86_PAGE_H_
+#define _ASM_X86_PAGE_H_
+/*
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+
+
+#define PAGE_SIZE	4096ul
+#ifdef __x86_64__
+#define LARGE_PAGE_SIZE	(512 * PAGE_SIZE)
+#else
+#define LARGE_PAGE_SIZE	(1024 * PAGE_SIZE)
+#endif
+
+#define PTE_PRESENT	(1ull << 0)
+#define PTE_WRITE	(1ull << 1)
+#define PTE_USER	(1ull << 2)
+#define PTE_PSE		(1ull << 7)
+#define PTE_ADDR	(0xffffffffff000ull)
+
+#ifdef __x86_64__
+#define	PAGE_LEVEL	4
+#define	PGDIR_WIDTH	9
+#define	PGDIR_MASK	511
+#else
+#define	PAGE_LEVEL	2
+#define	PGDIR_WIDTH	10
+#define	PGDIR_MASK	1023
+#endif
+
+#endif

lib/x86/vm.c +0 ~~-17~~

index 7ce7bbc..9c94ca5 100644
@@ -2,13 +2,6 @@
 #include "vm.h"
 #include "libcflat.h"
 
-#define PAGE_SIZE 4096ul
-#ifdef __x86_64__
-#define LARGE_PAGE_SIZE (512 * PAGE_SIZE)
-#else
-#define LARGE_PAGE_SIZE (1024 * PAGE_SIZE)
-#endif
-
 static void *free = 0;
 static void *vfree_top = 0;

@@ -44,16 +37,6 @@ void free_page(void *page)
 extern char edata;
 static unsigned long end_of_memory;
 
-#ifdef __x86_64__
-#define	PAGE_LEVEL	4
-#define	PGDIR_WIDTH	9
-#define	PGDIR_MASK	511
-#else
-#define	PAGE_LEVEL	2
-#define	PGDIR_WIDTH	10
-#define	PGDIR_MASK	1023
-#endif
-
 unsigned long *install_pte(unsigned long *cr3,
 			   int pte_level,
 			   void *virt,

lib/x86/vm.h +1 ~~-13~~

index 28794d7..72f84e6 100644
@@ -2,19 +2,7 @@
 #define VM_H
 
 #include "processor.h"
-
-#define PAGE_SIZE 4096ul
-#ifdef __x86_64__
-#define LARGE_PAGE_SIZE (512 * PAGE_SIZE)
-#else
-#define LARGE_PAGE_SIZE (1024 * PAGE_SIZE)
-#endif
-
-#define PTE_PRESENT (1ull << 0)
-#define PTE_PSE     (1ull << 7)
-#define PTE_WRITE   (1ull << 1)
-#define PTE_USER    (1ull << 2)
-#define PTE_ADDR    (0xffffffffff000ull)
+#include "asm/page.h"
 
 void setup_vm();

Signature · 1 line

1.8.3.1

Alexander Gordeev

author

2016-05-04 10:31 UTC #4 lore raw

PATCH v6 4/11 x86: Introduce lib/x86/asm/io.h

In reply to Alexander Gordeev

Make x86 consistent with other architectures and put IO specific defines to lib/x86/asm/io.h

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/x86/{ => asm}/io.h | 14 ++++++++++++--
 lib/x86/asm/pci.h      |  2 +-
 lib/x86/io.c           |  2 +-
 lib/x86/vm.h           | 11 +----------
 x86/eventinj.c         |  7 +------
 x86/hyperv.c           |  1 +
 x86/hyperv.h           |  1 -
 x86/hyperv_stimer.c    |  1 -
 x86/hyperv_synic.c     |  1 -
 x86/init.c             |  2 +-
 x86/svm.c              |  1 -
 x86/vmexit.c           |  1 -
 x86/vmx.c              |  1 -
 x86/vmx_tests.c        |  1 -
 14 files changed, 18 insertions(+), 28 deletions(-)
 rename lib/x86/{ => asm}/io.h (79%)

lib/x86/asm/io.h renamed from lib/x86/io.h +12 -2

similarity index 79%
rename from lib/x86/io.h
rename to lib/x86/asm/io.h
index bd6341c..c944df4 100644
@@ -1,5 +1,5 @@
-#ifndef IO_H
-#define IO_H
+#ifndef _ASM_X86_IO_H_
+#define _ASM_X86_IO_H_
 
 static inline unsigned char inb(unsigned short port)
 {

@@ -37,4 +37,14 @@ static inline void outl(unsigned int value, unsigned short port)
     asm volatile("outl %0, %w1" : : "a"(value), "Nd"(port));
 }
 
+static inline unsigned long virt_to_phys(const void *virt)
+{
+    return (unsigned long)virt;
+}
+
+static inline void *phys_to_virt(unsigned long phys)
+{
+    return (void *)phys;
+}
+
 #endif

lib/x86/asm/pci.h +1 -1

index 4ec20e1..cddde41 100644
@@ -7,7 +7,7 @@
  */
 #include "libcflat.h"
 #include "pci.h"
-#include "x86/io.h"
+#include "x86/asm/io.h"
 
 static inline uint32_t pci_config_read(pcidevaddr_t dev, uint8_t reg)
 {

lib/x86/io.c +1 -1

index d3b971e..d396d42 100644
@@ -1,6 +1,6 @@
 #include "libcflat.h"
 #include "smp.h"
-#include "io.h"
+#include "asm/io.h"
 #ifndef USE_SERIAL
 #define USE_SERIAL
 #endif

lib/x86/vm.h +1 ~~-10~~

index 72f84e6..6a4384f 100644
@@ -3,6 +3,7 @@
 
 #include "processor.h"
 #include "asm/page.h"
+#include "asm/io.h"
 
 void setup_vm();

@@ -27,14 +28,4 @@ unsigned long *install_large_page(unsigned long *cr3,unsigned long phys,
                                   void *virt);
 unsigned long *install_page(unsigned long *cr3, unsigned long phys, void *virt);
 
-static inline unsigned long virt_to_phys(const void *virt)
-{
-    return (unsigned long)virt;
-}
-
-static inline void *phys_to_virt(unsigned long phys)
-{
-    return (void *)phys;
-}
-
 #endif

x86/eventinj.c +1 -6

index 57c2a2d..84dfe71 100644
@@ -16,11 +16,6 @@ static inline void io_delay(void)
 {
 }
 
-static inline void outl(int addr, int val)
-{
-        asm volatile ("outl %1, %w0" : : "d" (addr), "a" (val));
-}
-
 void apic_self_ipi(u8 v)
 {
 	apic_icr_write(APIC_DEST_SELF | APIC_DEST_PHYSICAL | APIC_DM_FIXED |

@@ -32,7 +27,7 @@ void apic_self_nmi(void)
 	apic_icr_write(APIC_DEST_PHYSICAL | APIC_DM_NMI | APIC_INT_ASSERT, 0);
 }
 
-#define flush_phys_addr(__s) outl(0xe4, __s)
+#define flush_phys_addr(__s) outl(__s, 0xe4)
 #define flush_stack() do {						\
 		int __l;						\
 		flush_phys_addr(virt_to_phys(&__l));			\

x86/hyperv.c +1 -0

index 824773d..2511aa2 100644
@@ -1,4 +1,5 @@
 #include "hyperv.h"
+#include "asm/io.h"
 
 static void synic_ctl(u8 ctl, u8 vcpu_id, u8 sint)
 {

x86/hyperv.h +0 -1

index faf931b..434a933 100644
@@ -3,7 +3,6 @@
 
 #include "libcflat.h"
 #include "processor.h"
-#include "io.h"
 
 #define HYPERV_CPUID_FEATURES                   0x40000003

x86/hyperv_stimer.c +0 -1

index bf2e429..9a971ef 100644
@@ -5,7 +5,6 @@
 #include "vm.h"
 #include "apic.h"
 #include "desc.h"
-#include "io.h"
 #include "smp.h"
 #include "atomic.h"
 #include "hyperv.h"

x86/hyperv_synic.c +0 -1

index 6e08894..4bd07c3 100644
@@ -5,7 +5,6 @@
 #include "vm.h"
 #include "apic.h"
 #include "desc.h"
-#include "io.h"
 #include "smp.h"
 #include "atomic.h"
 #include "hyperv.h"

x86/init.c +1 -1

index 344dc1c..f47d671 100644
@@ -1,6 +1,6 @@
 #include "libcflat.h"
 #include "apic.h"
-#include "io.h"
+#include "asm/io.h"
 
 #define KBD_CCMD_READ_OUTPORT   0xD0    /* read output port */
 #define KBD_CCMD_WRITE_OUTPORT  0xD1    /* write output port */

x86/svm.c +0 -1

index 934b2ae..401ff6c 100644
@@ -6,7 +6,6 @@
 #include "vm.h"
 #include "smp.h"
 #include "types.h"
-#include "io.h"
 
 /* for the nested page table*/
 u64 *pml4e;

x86/vmexit.c +0 -1

index 9e04975..db7dbd8 100644
@@ -6,7 +6,6 @@
 #include "x86/vm.h"
 #include "x86/desc.h"
 #include "x86/acpi.h"
-#include "x86/io.h"
 
 struct test {
 	void (*func)(void);

x86/vmx.c +0 -1

index 6618008..411ed32 100644
@@ -35,7 +35,6 @@
 #include "vmx.h"
 #include "msr.h"
 #include "smp.h"
-#include "io.h"
 
 u64 *vmxon_region;
 struct vmcs *vmcs_root;

x86/vmx_tests.c +0 -1

index 71c571c..e83c8a2 100644
@@ -7,7 +7,6 @@
 #include "msr.h"
 #include "processor.h"
 #include "vm.h"
-#include "io.h"
 #include "fwcfg.h"
 #include "isr.h"
 #include "apic.h"

Signature · 1 line

1.8.3.1

Alexander Gordeev

author

2016-05-04 10:31 UTC #5 lore raw

PATCH v6 5/11 x86: Introduce lib/x86/asm/barrier.h

In reply to Alexander Gordeev

Make x86 consistent with other architectures and put memory barrier defines to lib/x86/asm/barrier.h

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/x86/asm/barrier.h | 13 +++++++++++++
 lib/x86/smp.h         |  4 ----
 x86/hyperv_stimer.c   |  1 +
 x86/kvmclock.c        |  1 +
 4 files changed, 15 insertions(+), 4 deletions(-)
 create mode 100644 lib/x86/asm/barrier.h

lib/x86/asm/barrier.h new file +13 -0

new file mode 100644
index 0000000..d602a2c
@@ -0,0 +1,13 @@
+#ifndef _ASM_X86_BARRIER_H_
+#define _ASM_X86_BARRIER_H_
+/*
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+
+#define mb()	asm volatile("mfence":::"memory")
+#define rmb()	asm volatile("lfence":::"memory")
+#define wmb()	asm volatile("sfence":::"memory")
+
+#endif

lib/x86/smp.h +0 -4

index 566018f..afabac8 100644
@@ -2,10 +2,6 @@
 #define __SMP_H
 #include <asm/spinlock.h>
 
-#define mb() 	asm volatile("mfence":::"memory")
-#define rmb()	asm volatile("lfence":::"memory")
-#define wmb()	asm volatile("sfence" ::: "memory")
-
 void smp_init(void);
 
 int cpu_count(void);

x86/hyperv_stimer.c +1 -0

index 9a971ef..6382938 100644
@@ -8,6 +8,7 @@
 #include "smp.h"
 #include "atomic.h"
 #include "hyperv.h"
+#include "asm/barrier.h"
 
 #define MAX_CPUS 4

x86/kvmclock.c +1 -0

index 327e60d..208d43c 100644
@@ -3,6 +3,7 @@
 #include "atomic.h"
 #include "processor.h"
 #include "kvmclock.h"
+#include "asm/barrier.h"
 
 #define unlikely(x)	__builtin_expect(!!(x), 0)
 #define likely(x)	__builtin_expect(!!(x), 1)

Signature · 1 line

1.8.3.1

Alexander Gordeev

author

2016-05-04 10:31 UTC #6 lore raw

PATCH v6 6/11 io: Separate overrides of virt_to_phys() and phys_to_virt()

In reply to Alexander Gordeev

Generic implementations of virt_to_phys() and phys_to_virt() are currently covered by a single "virt_to_phys" macro. Introduce additional macro "phys_to_virt" to allow separate overrides.

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/asm-generic/io.h | 2 ++
 1 file changed, 2 insertions(+)

lib/asm-generic/io.h +2 -0

index 931415a..3585ac0 100644
@@ -165,7 +165,9 @@ static inline unsigned long virt_to_phys(volatile void *address)
 {
 	return __pa((unsigned long)address);
 }
+#endif
 
+#ifndef phys_to_virt
 static inline void *phys_to_virt(unsigned long address)
 {
 	return __va(address);

Signature · 1 line

1.8.3.1

Alexander Gordeev

author

2016-05-04 10:31 UTC #7 lore raw

PATCH v6 7/11 io: Disallow memory re-ordering for generic memory barriers

In reply to Alexander Gordeev

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/asm-generic/io.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

lib/asm-generic/io.h +5 -2

index 3585ac0..1834720 100644
@@ -127,11 +127,14 @@ static inline u64 __bswap64(u64 x)
 	({ u64 __r = !__cpu_is_be() ? __bswap64(x) : ((u64)x); __r; })
 #define cpu_to_be64 be64_to_cpu
 
+#ifndef mb
+#define mb()	asm volatile("":::"memory")
+#endif
 #ifndef rmb
-#define rmb() do { } while (0)
+#define rmb()	asm volatile("":::"memory")
 #endif
 #ifndef wmb
-#define wmb() do { } while (0)
+#define wmb()	asm volatile("":::"memory")
 #endif
 
 #define readb(addr) \

Signature · 1 line

1.8.3.1

Alexander Gordeev

author

2016-05-04 10:31 UTC #8 lore raw

PATCH v6 8/11 x86: Cleanup PAGE_* constants

In reply to Alexander Gordeev

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/x86/asm/page.h | 10 +++++++++-
 x86/access.c       |  4 +---
 x86/vmx.h          |  2 +-
 3 files changed, 11 insertions(+), 5 deletions(-)

lib/x86/asm/page.h +9 -1

index d4580b4..916462d 100644
@@ -7,7 +7,14 @@
  */
 
 
-#define PAGE_SIZE	4096ul
+#include <linux/const.h>
+
+#define PAGE_SHIFT	12
+#define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
+#define PAGE_MASK	(~(PAGE_SIZE-1))
+
+#ifndef __ASSEMBLY__
+
 #ifdef __x86_64__
 #define LARGE_PAGE_SIZE	(512 * PAGE_SIZE)
 #else

@@ -30,4 +37,5 @@
 #define	PGDIR_MASK	1023
 #endif
 
+#endif /* !__ASSEMBLY__ */
 #endif

x86/access.c +1 -3

index 4acaee5..aaf805d 100644
@@ -2,6 +2,7 @@
 #include "libcflat.h"
 #include "desc.h"
 #include "processor.h"
+#include "asm/page.h"
 
 #define smp_id() 0

@@ -15,9 +16,6 @@ static int cpuid_7_ebx;
 static int cpuid_7_ecx;
 static int invalid_mask;
 
-#define PAGE_SIZE ((pt_element_t)4096)
-#define PAGE_MASK (~(PAGE_SIZE-1))
-
 #define PT_BASE_ADDR_MASK ((pt_element_t)((((pt_element_t)1 << 40) - 1) & PAGE_MASK))
 #define PT_PSE_BASE_ADDR_MASK (PT_BASE_ADDR_MASK & ~(1ull << 21))

x86/vmx.h +1 -1

index d6bc217..27f3451 100644
@@ -4,6 +4,7 @@
 #include "libcflat.h"
 #include "processor.h"
 #include "bitops.h"
+#include "asm/page.h"
 
 struct vmcs {
 	u32 revision_id; /* vmcs revision identifier */

@@ -476,7 +477,6 @@ enum Ctrl1 {
 #define EPT_PGDIR_ENTRIES	(1 << EPT_PGDIR_WIDTH)
 #define EPT_LEVEL_SHIFT(level)	(((level)-1) * EPT_PGDIR_WIDTH + 12)
 #define EPT_ADDR_MASK		GENMASK_ULL(51, 12)
-#define PAGE_MASK		(~(PAGE_SIZE-1))
 #define PAGE_MASK_2M		(~(PAGE_SIZE_2M-1))
 
 #define EPT_VLT_RD		1

Signature · 1 line

1.8.3.1

Alexander Gordeev

author

2016-05-04 10:31 UTC #9 lore raw

PATCH v6 9/11 x86: Cleanup PT_*_MASK flags

In reply to Alexander Gordeev

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/x86/asm/page.h | 13 ++++++++-----
 lib/x86/vm.c       | 20 ++++++++++----------
 x86/access.c       |  8 --------
 x86/asyncpf.c      |  2 +-
 x86/eventinj.c     | 10 +++++-----
 x86/hypercall.c    |  2 +-
 x86/pku.c          |  2 +-
 x86/rmap_chain.c   |  2 +-
 x86/smap.c         |  6 +++---
 9 files changed, 30 insertions(+), 35 deletions(-)

lib/x86/asm/page.h +8 -5

index 916462d..8a30212 100644
@@ -21,11 +21,14 @@
 #define LARGE_PAGE_SIZE	(1024 * PAGE_SIZE)
 #endif
 
-#define PTE_PRESENT	(1ull << 0)
-#define PTE_WRITE	(1ull << 1)
-#define PTE_USER	(1ull << 2)
-#define PTE_PSE		(1ull << 7)
-#define PTE_ADDR	(0xffffffffff000ull)
+#define PT_PRESENT_MASK		(1ull << 0)
+#define PT_WRITABLE_MASK	(1ull << 1)
+#define PT_USER_MASK		(1ull << 2)
+#define PT_ACCESSED_MASK	(1ull << 5)
+#define PT_DIRTY_MASK		(1ull << 6)
+#define PT_PSE_MASK		(1ull << 7)
+#define PT_NX_MASK		(1ull << 63)
+#define PT_ADDR_MASK		(0xffffffffff000ull)
 
 #ifdef __x86_64__
 #define	PAGE_LEVEL	4

lib/x86/vm.c +10 ~~-10~~

index 9c94ca5..fbf4825 100644
@@ -49,16 +49,16 @@ unsigned long *install_pte(unsigned long *cr3,
 
     for (level = PAGE_LEVEL; level > pte_level; --level) {
 	offset = ((unsigned long)virt >> ((level-1) * PGDIR_WIDTH + 12)) & PGDIR_MASK;
-	if (!(pt[offset] & PTE_PRESENT)) {
+	if (!(pt[offset] & PT_PRESENT_MASK)) {
 	    unsigned long *new_pt = pt_page;
             if (!new_pt)
                 new_pt = alloc_page();
             else
                 pt_page = 0;
 	    memset(new_pt, 0, PAGE_SIZE);
-	    pt[offset] = virt_to_phys(new_pt) | PTE_PRESENT | PTE_WRITE | PTE_USER;
+	    pt[offset] = virt_to_phys(new_pt) | PT_PRESENT_MASK | PT_WRITABLE_MASK | PT_USER_MASK;
 	}
-	pt = phys_to_virt(pt[offset] & 0xffffffffff000ull);
+	pt = phys_to_virt(pt[offset] & PT_ADDR_MASK);
     }
     offset = ((unsigned long)virt >> ((level-1) * PGDIR_WIDTH + 12)) & PGDIR_MASK;
     pt[offset] = pte;

@@ -74,11 +74,11 @@ unsigned long *get_pte(unsigned long *cr3, void *virt)
     for (level = PAGE_LEVEL; level > 1; --level) {
 	offset = ((unsigned long)virt >> (((level-1) * PGDIR_WIDTH) + 12)) & PGDIR_MASK;
 	pte = pt[offset];
-	if (!(pte & PTE_PRESENT))
+	if (!(pte & PT_PRESENT_MASK))
 	    return NULL;
-	if (level == 2 && (pte & PTE_PSE))
+	if (level == 2 && (pte & PT_PSE_MASK))
 	    return &pt[offset];
-	pt = phys_to_virt(pte & 0xffffffffff000ull);
+	pt = phys_to_virt(pte & PT_ADDR_MASK);
     }
     offset = ((unsigned long)virt >> (((level-1) * PGDIR_WIDTH) + 12)) & PGDIR_MASK;
     return &pt[offset];

@@ -89,14 +89,14 @@ unsigned long *install_large_page(unsigned long *cr3,
 				  void *virt)
 {
     return install_pte(cr3, 2, virt,
-		       phys | PTE_PRESENT | PTE_WRITE | PTE_USER | PTE_PSE, 0);
+		       phys | PT_PRESENT_MASK | PT_WRITABLE_MASK | PT_USER_MASK | PT_PSE_MASK, 0);
 }
 
 unsigned long *install_page(unsigned long *cr3,
 			    unsigned long phys,
 			    void *virt)
 {
-    return install_pte(cr3, 1, virt, phys | PTE_PRESENT | PTE_WRITE | PTE_USER, 0);
+    return install_pte(cr3, 1, virt, phys | PT_PRESENT_MASK | PT_WRITABLE_MASK | PT_USER_MASK, 0);
 }

@@ -178,7 +178,7 @@ void *vmalloc(unsigned long size)
 
 uint64_t virt_to_phys_cr3(void *mem)
 {
-    return (*get_pte(phys_to_virt(read_cr3()), mem) & PTE_ADDR) + ((ulong)mem & (PAGE_SIZE - 1));
+    return (*get_pte(phys_to_virt(read_cr3()), mem) & PT_ADDR_MASK) + ((ulong)mem & (PAGE_SIZE - 1));
 }
 
 void vfree(void *mem)

@@ -186,7 +186,7 @@ void vfree(void *mem)
     unsigned long size = ((unsigned long *)mem)[-1];
 
     while (size) {
-	free_page(phys_to_virt(*get_pte(phys_to_virt(read_cr3()), mem) & PTE_ADDR));
+	free_page(phys_to_virt(*get_pte(phys_to_virt(read_cr3()), mem) & PT_ADDR_MASK));
 	mem += PAGE_SIZE;
 	size -= PAGE_SIZE;
     }

x86/access.c +0 -8

index aaf805d..1f331db 100644
@@ -19,14 +19,6 @@ static int invalid_mask;
 #define PT_BASE_ADDR_MASK ((pt_element_t)((((pt_element_t)1 << 40) - 1) & PAGE_MASK))
 #define PT_PSE_BASE_ADDR_MASK (PT_BASE_ADDR_MASK & ~(1ull << 21))
 
-#define PT_PRESENT_MASK    ((pt_element_t)1 << 0)
-#define PT_WRITABLE_MASK   ((pt_element_t)1 << 1)
-#define PT_USER_MASK       ((pt_element_t)1 << 2)
-#define PT_ACCESSED_MASK   ((pt_element_t)1 << 5)
-#define PT_DIRTY_MASK      ((pt_element_t)1 << 6)
-#define PT_PSE_MASK        ((pt_element_t)1 << 7)
-#define PT_NX_MASK         ((pt_element_t)1 << 63)
-
 #define CR0_WP_MASK (1UL << 16)
 #define CR4_SMEP_MASK (1UL << 20)

x86/asyncpf.c +1 -1

index 304d015..a6051ab 100644
@@ -69,7 +69,7 @@ static void pf_isr(struct ex_regs *r)
 			report("Got present #PF token %x\n", true, read_cr2());
 			if ((uint32_t)read_cr2() == ~0)
 				break;
-			install_pte(phys_to_virt(read_cr3()), 1, virt, phys | PTE_PRESENT | PTE_WRITE, 0);
+			install_pte(phys_to_virt(read_cr3()), 1, virt, phys | PT_PRESENT_MASK | PT_WRITABLE_MASK, 0);
 			write_cr3(read_cr3());
 			phys = 0;
 			break;

x86/eventinj.c +5 -5

index 84dfe71..9ee557b 100644
@@ -52,7 +52,7 @@ void do_pf_tss(void)
 {
 	printf("PF running\n");
 	install_pte(phys_to_virt(read_cr3()), 1, stack_va,
-		    stack_phys | PTE_PRESENT | PTE_WRITE, 0);
+		    stack_phys | PT_PRESENT_MASK | PT_WRITABLE_MASK, 0);
 	invlpg(stack_va);
 }

@@ -363,7 +363,7 @@ int main()
 	printf("Try to divide by 0\n");
 	/* install read only pte */
 	install_pte(phys_to_virt(read_cr3()), 1, stack_va,
-		    stack_phys | PTE_PRESENT, 0);
+		    stack_phys | PT_PRESENT_MASK, 0);
 	invlpg(stack_va);
 	flush_phys_addr(stack_phys);
 	switch_stack(stack_va + 4095);

@@ -382,7 +382,7 @@ int main()
 	set_idt_sel(33, NP_SEL);
 	/* install read only pte */
 	install_pte(phys_to_virt(read_cr3()), 1, stack_va,
-		    stack_phys | PTE_PRESENT, 0);
+		    stack_phys | PT_PRESENT_MASK, 0);
 	invlpg(stack_va);
 	flush_idt_page();
 	flush_phys_addr(stack_phys);

@@ -398,8 +398,8 @@ int main()
 	/* use shadowed stack during interrupt delivery */
 	for (i = 0; i < 4096/sizeof(ulong); i++) {
 		if (!cr3[i]) {
-			cr3[i] = virt_to_phys(pt) | PTE_PRESENT | PTE_WRITE;
-			pt[0] = virt_to_phys(pt) | PTE_PRESENT | PTE_WRITE;
+			cr3[i] = virt_to_phys(pt) | PT_PRESENT_MASK | PT_WRITABLE_MASK;
+			pt[0] = virt_to_phys(pt) | PT_PRESENT_MASK | PT_WRITABLE_MASK;
 #ifndef __x86_64__
 			((ulong*)(i<<22))[1] = 0;
 #else

x86/hypercall.c +1 -1

index 75179a1..9380f78 100644
@@ -65,7 +65,7 @@ int main(int ac, char **av)
 	u8 *topmost = (void *) ((1ul << 47) - PAGE_SIZE);
 
 	install_pte(phys_to_virt(read_cr3()), 1, topmost,
-		    virt_to_phys(data1) | PTE_PRESENT | PTE_WRITE, 0);
+		    virt_to_phys(data1) | PT_PRESENT_MASK | PT_WRITABLE_MASK, 0);
 	memset(topmost, 0xcc, PAGE_SIZE);
 	topmost[4093] = 0x0f;
 	topmost[4094] = 0x01;

x86/pku.c +1 -1

index df51d1b..e742735 100644
@@ -77,7 +77,7 @@ int main(int ac, char **av)
     wrmsr(MSR_EFER, rdmsr(MSR_EFER) | EFER_LMA);
 
     for (i = 0; i < USER_BASE; i += PAGE_SIZE) {
-        *get_pte(phys_to_virt(read_cr3()), phys_to_virt(i)) &= ~PTE_USER;
+        *get_pte(phys_to_virt(read_cr3()), phys_to_virt(i)) &= ~PT_USER_MASK;
         *get_pte(phys_to_virt(read_cr3()), phys_to_virt(i)) |= ((unsigned long)pkey << PTE_PKEY_BIT);
         invlpg((void *)i);
     }

x86/rmap_chain.c +1 -1

index b356a37..7bf6275 100644
@@ -36,7 +36,7 @@ int main (void)
 
     virt_addr += PAGE_SIZE;
     install_pte(phys_to_virt(read_cr3()), 1, virt_addr,
-                0 | PTE_PRESENT | PTE_WRITE, target_page);
+                0 | PT_PRESENT_MASK | PT_WRITABLE_MASK, target_page);
 
     *(unsigned long *)virt_addr = 0;
     printf("PASS\n");

x86/smap.c +3 -3

index 69e7186..ace9dc0 100644
@@ -62,7 +62,7 @@ static void check_smap_nowp(void)
 {
 	test = 0x99;
 
-	*get_pte(phys_to_virt(read_cr3()), USER_ADDR(test)) &= ~PTE_WRITE;
+	*get_pte(phys_to_virt(read_cr3()), USER_ADDR(test)) &= ~PT_WRITABLE_MASK;
 
 	write_cr4(read_cr4() & ~X86_CR4_SMAP);
 	write_cr0(read_cr0() & ~X86_CR0_WP);

@@ -81,7 +81,7 @@ static void check_smap_nowp(void)
 	report("read from user page with SMAP=1, AC=0, WP=0, PTE.U=1 && PTE.W=0", pf_count == 1 && save == 0x99);
 
 	/* Undo changes */
-	*get_pte(phys_to_virt(read_cr3()), USER_ADDR(test)) |= PTE_WRITE;
+	*get_pte(phys_to_virt(read_cr3()), USER_ADDR(test)) |= PT_WRITABLE_MASK;
 
 	write_cr0(read_cr0() | X86_CR0_WP);
 	write_cr3(read_cr3());

@@ -102,7 +102,7 @@ int main(int ac, char **av)
 
 	// Map first 16MB as supervisor pages
 	for (i = 0; i < USER_BASE; i += PAGE_SIZE) {
-		*get_pte(phys_to_virt(read_cr3()), phys_to_virt(i)) &= ~PTE_USER;
+		*get_pte(phys_to_virt(read_cr3()), phys_to_virt(i)) &= ~PT_USER_MASK;
 		invlpg((void *)i);
 	}

Signature · 1 line

1.8.3.1

Andrew Jones

Reviewed-by on v6 9/11

2016-05-04 12:19 UTC #10 lore raw

Re: [kvm-unit-tests PATCH v6 09/11] x86: Cleanup PT_*_MASK flags

Quoted 24 lines · Alexander Gordeev

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/x86/asm/page.h | 13 ++++++++-----
 lib/x86/vm.c       | 20 ++++++++++----------
 x86/access.c       |  8 --------
 x86/asyncpf.c      |  2 +-
 x86/eventinj.c     | 10 +++++-----
 x86/hypercall.c    |  2 +-
 x86/pku.c          |  2 +-
 x86/rmap_chain.c   |  2 +-
 x86/smap.c         |  6 +++---
 9 files changed, 30 insertions(+), 35 deletions(-)

lib/x86/asm/page.h +0 -0

index 916462d..8a30212 100644
@@ -21,11 +21,14 @@
 #define LARGE_PAGE_SIZE	(1024 * PAGE_SIZE)
 #endif

lib/x86/asm/page.h line 28

Quoted 10 lines

lib/x86/asm/page.h +5 -5

-#define PTE_PRESENT	(1ull << 0)
-#define PTE_WRITE	(1ull << 1)
-#define PTE_USER	(1ull << 2)
-#define PTE_PSE		(1ull << 7)
-#define PTE_ADDR	(0xffffffffff000ull)
+#define PT_PRESENT_MASK		(1ull << 0)
+#define PT_WRITABLE_MASK	(1ull << 1)
+#define PT_USER_MASK		(1ull << 2)
+#define PT_ACCESSED_MASK	(1ull << 5)
+#define PT_DIRTY_MASK		(1ull << 6)

To be 100% consistent with arch/x86/kvm/mmu.h, I'd change

lib/x86/asm/page.h line 29

Quoted 1 line

lib/x86/asm/page.h +1 -0
+#define PT_PSE_MASK		(1ull << 7)

to PT_PAT_MASK and

lib/x86/asm/page.h line 30

Quoted 1 line

lib/x86/asm/page.h +1 -0
+#define PT_NX_MASK		(1ull << 63)

to PT64_NX_MASK

lib/x86/asm/page.h line 31

Quoted 1 line

lib/x86/asm/page.h +1 -0

+#define PT_ADDR_MASK		(0xffffffffff000ull)

And, this one (PT_ADDR_MASK) should just be deleted, as it's not used. Or, if somebody objects to deleting it, then it should be defined as GENMASK_ULL(52, 12) for better clarity.

These are just nits though, so

Reviewed-by Andrew Jones <[email protected]>

Alexander Gordeev

author

2016-05-04 14:06 UTC #11 lore raw

Re: [kvm-unit-tests PATCH v6 09/11] x86: Cleanup PT_*_MASK flags

Quoted 27 lines · Andrew Jones

Quoted 8 lines
lib/x86/asm/page.h +0 -0
index 916462d..8a30212 100644
@@ -21,11 +21,14 @@
 #define LARGE_PAGE_SIZE	(1024 * PAGE_SIZE)
 #endif
 
lib/x86/asm/page.h line 28
Quoted 10 lines
lib/x86/asm/page.h +5 -5
-#define PTE_PRESENT	(1ull << 0)
-#define PTE_WRITE	(1ull << 1)
-#define PTE_USER	(1ull << 2)
-#define PTE_PSE		(1ull << 7)
-#define PTE_ADDR	(0xffffffffff000ull)
+#define PT_PRESENT_MASK		(1ull << 0)
+#define PT_WRITABLE_MASK	(1ull << 1)
+#define PT_USER_MASK		(1ull << 2)
+#define PT_ACCESSED_MASK	(1ull << 5)
+#define PT_DIRTY_MASK		(1ull << 6)
To be 100% consistent with arch/x86/kvm/mmu.h, I'd change
lib/x86/asm/page.h line 29
Quoted 1 line
lib/x86/asm/page.h +1 -0
+#define PT_PSE_MASK		(1ull << 7)
to PT_PAT_MASK and
lib/x86/asm/page.h line 30
Quoted 1 line
lib/x86/asm/page.h +1 -0
+#define PT_NX_MASK		(1ull << 63)

Quoted 7 lines

to PT64_NX_MASK
lib/x86/asm/page.h line 31
Quoted 1 line
lib/x86/asm/page.h +1 -0
+#define PT_ADDR_MASK		(0xffffffffff000ull)
And, this one (PT_ADDR_MASK) should just be deleted, as it's not used. Or, if somebody objects to deleting it, then it should be defined as GENMASK_ULL(52, 12) for better clarity.

Will redefine since lib/x86/vm.c does use it

Quoted 4 lines

These are just nits though, so
Reviewed-by Andrew Jones <[email protected]>

Andrew Jones

2016-05-04 14:15 UTC #12 lore raw

Re: [kvm-unit-tests PATCH v6 09/11] x86: Cleanup PT_*_MASK flags

Quoted 30 lines · Alexander Gordeev

Quoted 29 lines · Andrew Jones
Quoted 8 lines
lib/x86/asm/page.h +0 -0
index 916462d..8a30212 100644
@@ -21,11 +21,14 @@
 #define LARGE_PAGE_SIZE	(1024 * PAGE_SIZE)
 #endif
 
lib/x86/asm/page.h line 28
Quoted 10 lines
lib/x86/asm/page.h +5 -5
-#define PTE_PRESENT	(1ull << 0)
-#define PTE_WRITE	(1ull << 1)
-#define PTE_USER	(1ull << 2)
-#define PTE_PSE		(1ull << 7)
-#define PTE_ADDR	(0xffffffffff000ull)
+#define PT_PRESENT_MASK		(1ull << 0)
+#define PT_WRITABLE_MASK	(1ull << 1)
+#define PT_USER_MASK		(1ull << 2)
+#define PT_ACCESSED_MASK	(1ull << 5)
+#define PT_DIRTY_MASK		(1ull << 6)
To be 100% consistent with arch/x86/kvm/mmu.h, I'd change
lib/x86/asm/page.h line 29
Quoted 1 line
lib/x86/asm/page.h +1 -0
+#define PT_PSE_MASK		(1ull << 7)
to PT_PAT_MASK and
lib/x86/asm/page.h line 30
Quoted 1 line
lib/x86/asm/page.h +1 -0
+#define PT_NX_MASK		(1ull << 63)
to PT64_NX_MASK

Quoted 7 lines

Quoted 5 lines
lib/x86/asm/page.h line 31
Quoted 1 line
lib/x86/asm/page.h +1 -0
+#define PT_ADDR_MASK		(0xffffffffff000ull)
And, this one (PT_ADDR_MASK) should just be deleted, as it's not used. Or, if somebody objects to deleting it, then it should be defined as GENMASK_ULL(52, 12) for better clarity.
Will redefine since lib/x86/vm.c does use it

Ah, you're right. Bad grepping on my part.

Quoted 8 lines

Quoted 4 lines
These are just nits though, so
Reviewed-by Andrew Jones <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alexander Gordeev

author

2016-05-05 09:22 UTC #13 lore raw

Re: [kvm-unit-tests PATCH v6 09/11] x86: Cleanup PT_*_MASK flags

In reply to Andrew Jones

Quoted 38 lines · Andrew Jones

Quoted 24 lines · Alexander Gordeev

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/x86/asm/page.h | 13 ++++++++-----
 lib/x86/vm.c       | 20 ++++++++++----------
 x86/access.c       |  8 --------
 x86/asyncpf.c      |  2 +-
 x86/eventinj.c     | 10 +++++-----
 x86/hypercall.c    |  2 +-
 x86/pku.c          |  2 +-
 x86/rmap_chain.c   |  2 +-
 x86/smap.c         |  6 +++---
 9 files changed, 30 insertions(+), 35 deletions(-)

lib/x86/asm/page.h +0 -0

index 916462d..8a30212 100644
@@ -21,11 +21,14 @@
 #define LARGE_PAGE_SIZE	(1024 * PAGE_SIZE)
 #endif

lib/x86/asm/page.h line 28

Quoted 10 lines

lib/x86/asm/page.h +5 -5

-#define PTE_PRESENT	(1ull << 0)
-#define PTE_WRITE	(1ull << 1)
-#define PTE_USER	(1ull << 2)
-#define PTE_PSE		(1ull << 7)
-#define PTE_ADDR	(0xffffffffff000ull)
+#define PT_PRESENT_MASK		(1ull << 0)
+#define PT_WRITABLE_MASK	(1ull << 1)
+#define PT_USER_MASK		(1ull << 2)
+#define PT_ACCESSED_MASK	(1ull << 5)
+#define PT_DIRTY_MASK		(1ull << 6)

To be 100% consistent with arch/x86/kvm/mmu.h, I'd change

Quoted 3 lines

lib/x86/asm/page.h line 29
Quoted 1 line
lib/x86/asm/page.h +1 -0
+#define PT_PSE_MASK		(1ull << 7)
to PT_PAT_MASK and

Hmm.. PT_PAT_MASK and PT_PAT_SHIFT seems superseded by PT_PAGE_SIZE_MASK and PT_PAGE_SIZE_SHIFT and actually redundant.

Quoted 13 lines

lib/x86/asm/page.h line 30
Quoted 1 line
lib/x86/asm/page.h +1 -0
+#define PT_NX_MASK		(1ull << 63)
to PT64_NX_MASK
lib/x86/asm/page.h line 31
Quoted 1 line
lib/x86/asm/page.h +1 -0
+#define PT_ADDR_MASK		(0xffffffffff000ull)
And, this one (PT_ADDR_MASK) should just be deleted, as it's not used. Or, if somebody objects to deleting it, then it should be defined as GENMASK_ULL(52, 12) for better clarity.
These are just nits though, so
Reviewed-by Andrew Jones <[email protected]>

Alexander Gordeev

author

2016-05-04 10:31 UTC #14 lore raw

PATCH v6 10/11 io: Make ioremap() prototype conform to Linux one

In reply to Alexander Gordeev

This update also adds missing __iomem specificator which has been used by existing IO accessors.

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/asm-generic/io.h | 4 ++--
 lib/ppc64/asm/io.h   | 2 ++
 lib/x86/asm/io.h     | 2 ++
 3 files changed, 6 insertions(+), 2 deletions(-)

lib/asm-generic/io.h +2 -2

index 1834720..41756db 100644
@@ -156,10 +156,10 @@ static inline u64 __bswap64(u64 x)
 	({ wmb(); __raw_writeq(cpu_to_le64(b), addr); })
 
 #ifndef ioremap
-static inline void *ioremap(u64 phys_addr, size_t size __unused)
+static inline void __iomem *ioremap(phys_addr_t phys_addr, size_t size __unused)
 {
 	assert(sizeof(long) == 8 || !(phys_addr >> 32));
-	return (void *)(unsigned long)phys_addr;
+	return (void __iomem *)(unsigned long)phys_addr;
 }
 #endif

lib/ppc64/asm/io.h +2 -0

index 4f2c31b..2b4dd2b 100644
@@ -9,5 +9,7 @@
 #error Undefined byte order
 #endif
 
+#define __iomem
+
 #include <asm-generic/io.h>
 #endif

lib/x86/asm/io.h +2 -0

index c944df4..83387b5 100644
@@ -1,6 +1,8 @@
 #ifndef _ASM_X86_IO_H_
 #define _ASM_X86_IO_H_
 
+#define __iomem
+
 static inline unsigned char inb(unsigned short port)
 {
     unsigned char value;

Signature · 1 line

1.8.3.1

Alexander Gordeev

author

2016-05-04 10:31 UTC #15 lore raw

PATCH v6 11/11 x86: io: Factor out ioremap()

In reply to Alexander Gordeev

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/x86/asm/io.h |  2 ++
 lib/x86/io.c     | 15 +++++++++++++++
 x86/vmexit.c     | 10 ++--------
 3 files changed, 19 insertions(+), 8 deletions(-)

lib/x86/asm/io.h +2 -0

index 83387b5..2436822 100644
@@ -49,4 +49,6 @@ static inline void *phys_to_virt(unsigned long phys)
     return (void *)phys;
 }
 
+void __iomem *ioremap(phys_addr_t phys_addr, size_t size);
+
 #endif

lib/x86/io.c +15 -0

index d396d42..c49926a 100644
@@ -1,6 +1,8 @@
 #include "libcflat.h"
+#include "vm.h"
 #include "smp.h"
 #include "asm/io.h"
+#include "asm/page.h"
 #ifndef USE_SERIAL
 #define USE_SERIAL
 #endif

@@ -81,3 +83,16 @@ void exit(int code)
         asm volatile("out %0, %1" : : "a"(code), "d"((short)0xf4));
 #endif
 }
+
+void __iomem *ioremap(phys_addr_t phys_addr, size_t size)
+{
+	phys_addr_t base = phys_addr & PAGE_MASK;
+	phys_addr_t offset = phys_addr - base;
+
+	/*
+	 * The kernel sets PTEs for an ioremap() with page cache disabled.
+	 * It would make sense that I/O mappings would be uncached -
+	 * and may help us find bugs when we properly map that way.
+	 */
+	return vmap(phys_addr, size) + offset;
+}

x86/vmexit.c +2 -8

index db7dbd8..c2e1e49 100644
@@ -371,8 +371,7 @@ int main(int ac, char **av)
 {
 	struct fadt_descriptor_rev1 *fadt;
 	int i;
-	unsigned long membar = 0, base, offset;
-	void *m;
+	unsigned long membar = 0;
 	pcidevaddr_t pcidev;
 
 	smp_init();

@@ -394,12 +393,7 @@ int main(int ac, char **av)
 			}
 			if (pci_bar_is_memory(pcidev, i)) {
 				membar = pci_bar_addr(pcidev, i);
-				base = membar & ~4095;
-				offset = membar - base;
-				m = alloc_vpages(1);
-				
-				install_page((void *)read_cr3(), base, m);
-				pci_test.memaddr = m + offset;
+				pci_test.memaddr = ioremap(membar, PAGE_SIZE);
 			} else {
 				pci_test.iobar = pci_bar_addr(pcidev, i);
 			}

Signature · 1 line

1.8.3.1

Andrew Jones

2016-05-04 12:24 UTC #16 lore raw

Re: [kvm-unit-tests PATCH v6 11/11] x86: io: Factor out ioremap()

Quoted 39 lines · Alexander Gordeev

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/x86/asm/io.h |  2 ++
 lib/x86/io.c     | 15 +++++++++++++++
 x86/vmexit.c     | 10 ++--------
 3 files changed, 19 insertions(+), 8 deletions(-)

lib/x86/asm/io.h +2 -0

index 83387b5..2436822 100644
@@ -49,4 +49,6 @@ static inline void *phys_to_virt(unsigned long phys)
     return (void *)phys;
 }
 
+void __iomem *ioremap(phys_addr_t phys_addr, size_t size);
+
 #endif

lib/x86/io.c +2 -0

index d396d42..c49926a 100644
@@ -1,6 +1,8 @@
 #include "libcflat.h"
+#include "vm.h"
 #include "smp.h"
 #include "asm/io.h"
+#include "asm/page.h"
 #ifndef USE_SERIAL
 #define USE_SERIAL
 #endif

@@ -81,3 +83,16 @@ void exit(int code)
         asm volatile("out %0, %1" : : "a"(code), "d"((short)0xf4));
 #endif
 }

lib/x86/io.c void exit(int code) · line 95

Quoted 10 lines

lib/x86/io.c +10 -0

+
+void __iomem *ioremap(phys_addr_t phys_addr, size_t size)
+{
+	phys_addr_t base = phys_addr & PAGE_MASK;
+	phys_addr_t offset = phys_addr - base;
+
+	/*
+	 * The kernel sets PTEs for an ioremap() with page cache disabled.
+	 * It would make sense that I/O mappings would be uncached -
+	 * and may help us find bugs when we properly map that way.

You left out the 'but' ("...that way, but we don't currently. Maybe we should add a vmap_uncached?"), or something like that.

Quoted 38 lines

lib/x86/io.c +3 -0

+	 */
+	return vmap(phys_addr, size) + offset;
+}

x86/vmexit.c +2 -8

index db7dbd8..c2e1e49 100644
@@ -371,8 +371,7 @@ int main(int ac, char **av)
 {
 	struct fadt_descriptor_rev1 *fadt;
 	int i;
-	unsigned long membar = 0, base, offset;
-	void *m;
+	unsigned long membar = 0;
 	pcidevaddr_t pcidev;
 
 	smp_init();

@@ -394,12 +393,7 @@ int main(int ac, char **av)
 			}
 			if (pci_bar_is_memory(pcidev, i)) {
 				membar = pci_bar_addr(pcidev, i);
-				base = membar & ~4095;
-				offset = membar - base;
-				m = alloc_vpages(1);
-				
-				install_page((void *)read_cr3(), base, m);
-				pci_test.memaddr = m + offset;
+				pci_test.memaddr = ioremap(membar, PAGE_SIZE);
 			} else {
 				pci_test.iobar = pci_bar_addr(pcidev, i);
 			}

Signature · 6 lines

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alexander Gordeev

author

2016-05-04 14:01 UTC #17 lore raw

Re: [kvm-unit-tests PATCH v6 11/11] x86: io: Factor out ioremap()

Quoted 45 lines · Andrew Jones

Quoted 44 lines · Alexander Gordeev

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/x86/asm/io.h |  2 ++
 lib/x86/io.c     | 15 +++++++++++++++
 x86/vmexit.c     | 10 ++--------
 3 files changed, 19 insertions(+), 8 deletions(-)

lib/x86/asm/io.h +2 -0

index 83387b5..2436822 100644
@@ -49,4 +49,6 @@ static inline void *phys_to_virt(unsigned long phys)
     return (void *)phys;
 }
 
+void __iomem *ioremap(phys_addr_t phys_addr, size_t size);
+
 #endif

lib/x86/io.c +7 -0

index d396d42..c49926a 100644
@@ -1,6 +1,8 @@
 #include "libcflat.h"
+#include "vm.h"
 #include "smp.h"
 #include "asm/io.h"
+#include "asm/page.h"
 #ifndef USE_SERIAL
 #define USE_SERIAL
 #endif

@@ -81,3 +83,16 @@ void exit(int code)
         asm volatile("out %0, %1" : : "a"(code), "d"((short)0xf4));
 #endif
 }
+
+void __iomem *ioremap(phys_addr_t phys_addr, size_t size)
+{
+	phys_addr_t base = phys_addr & PAGE_MASK;
+	phys_addr_t offset = phys_addr - base;

Quoted 8 lines

lib/x86/io.c void exit(int code) · line 95
Quoted 5 lines
lib/x86/io.c +5 -0
+
+	/*
+	 * The kernel sets PTEs for an ioremap() with page cache disabled.
+	 * It would make sense that I/O mappings would be uncached -
+	 * and may help us find bugs when we properly map that way.
You left out the 'but' ("...that way, but we don't currently. Maybe we should add a vmap_uncached?"), or something like that.

If this one is better?

"
Andrew Jones noted: "The kernel sets the PTEs for an ioremap with PCD, page cache disabled. I guess we can live without that, but it would make sense that I/O mappings would be uncached - and may help us find bugs when we properly map that way."
"

As of vmap_uncached - I guess since vmap() has a broader "scope" than just IO it would make sense to introduce vmap_prot() instead. Or add an extra page protection parameter to the existing vmap(). I could do that with a follow-up patch.

Quoted 38 lines

lib/x86/io.c +3 -0

+	 */
+	return vmap(phys_addr, size) + offset;
+}

x86/vmexit.c +2 -8

index db7dbd8..c2e1e49 100644
@@ -371,8 +371,7 @@ int main(int ac, char **av)
 {
 	struct fadt_descriptor_rev1 *fadt;
 	int i;
-	unsigned long membar = 0, base, offset;
-	void *m;
+	unsigned long membar = 0;
 	pcidevaddr_t pcidev;
 
 	smp_init();

@@ -394,12 +393,7 @@ int main(int ac, char **av)
 			}
 			if (pci_bar_is_memory(pcidev, i)) {
 				membar = pci_bar_addr(pcidev, i);
-				base = membar & ~4095;
-				offset = membar - base;
-				m = alloc_vpages(1);
-				
-				install_page((void *)read_cr3(), base, m);
-				pci_test.memaddr = m + offset;
+				pci_test.memaddr = ioremap(membar, PAGE_SIZE);
 			} else {
 				pci_test.iobar = pci_bar_addr(pcidev, i);
 			}

Signature · 6 lines

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrew Jones

2016-05-04 14:12 UTC #18 lore raw

Re: [kvm-unit-tests PATCH v6 11/11] x86: io: Factor out ioremap()

Quoted 55 lines · Alexander Gordeev

Quoted 45 lines · Andrew Jones

Quoted 44 lines · Alexander Gordeev

Cc Andrew Jones <[email protected]>

Cc Thomas Huth <[email protected]>

Cc Radim Krčmář <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/x86/asm/io.h |  2 ++
 lib/x86/io.c     | 15 +++++++++++++++
 x86/vmexit.c     | 10 ++--------
 3 files changed, 19 insertions(+), 8 deletions(-)

lib/x86/asm/io.h +2 -0

index 83387b5..2436822 100644
@@ -49,4 +49,6 @@ static inline void *phys_to_virt(unsigned long phys)
     return (void *)phys;
 }
 
+void __iomem *ioremap(phys_addr_t phys_addr, size_t size);
+
 #endif

lib/x86/io.c +7 -0

index d396d42..c49926a 100644
@@ -1,6 +1,8 @@
 #include "libcflat.h"
+#include "vm.h"
 #include "smp.h"
 #include "asm/io.h"
+#include "asm/page.h"
 #ifndef USE_SERIAL
 #define USE_SERIAL
 #endif

@@ -81,3 +83,16 @@ void exit(int code)
         asm volatile("out %0, %1" : : "a"(code), "d"((short)0xf4));
 #endif
 }
+
+void __iomem *ioremap(phys_addr_t phys_addr, size_t size)
+{
+	phys_addr_t base = phys_addr & PAGE_MASK;
+	phys_addr_t offset = phys_addr - base;

Quoted 8 lines

lib/x86/io.c void exit(int code) · line 95
Quoted 5 lines
lib/x86/io.c +5 -0
+
+	/*
+	 * The kernel sets PTEs for an ioremap() with page cache disabled.
+	 * It would make sense that I/O mappings would be uncached -
+	 * and may help us find bugs when we properly map that way.
You left out the 'but' ("...that way, but we don't currently. Maybe we should add a vmap_uncached?"), or something like that.

Quoted 1 line

If this one is better?

Not really :-) I'd stick with what you have above, but extending it with some sort of 'but we don't do that right now'.

Quoted 8 lines

"
Andrew Jones noted: "The kernel sets the PTEs for an ioremap with PCD, page cache disabled. I guess we can live without that, but it would make sense that I/O mappings would be uncached - and may help us find bugs when we properly map that way."
"

Quoted 4 lines

As of vmap_uncached - I guess since vmap() has a broader "scope" than just IO it would make sense to introduce vmap_prot() instead. Or add an extra page protection parameter to the existing vmap(). I could do that with a follow-up patch.

You don't really need to write vmap_uncached unless you plan to move unit tests to it too. We'd need sign-off from the unit test developers for that though. BTW, I'd still add a vmap_uncached, but it can certainly wrap a vmap_prot. I wouldn't extend vmap though. The default flags will almost always be correct, so having to pass them in (or a zero to be ORed with them) all the time wouldn't be nice.

drew

Quoted 43 lines

Quoted 35 lines
Quoted 35 lines
lib/x86/io.c +3 -0
+	 */
+	return vmap(phys_addr, size) + offset;
+}
x86/vmexit.c +2 -8
index db7dbd8..c2e1e49 100644
@@ -371,8 +371,7 @@ int main(int ac, char **av)
 {
 	struct fadt_descriptor_rev1 *fadt;
 	int i;
-	unsigned long membar = 0, base, offset;
-	void *m;
+	unsigned long membar = 0;
 	pcidevaddr_t pcidev;
 
 	smp_init();
@@ -394,12 +393,7 @@ int main(int ac, char **av)
 			}
 			if (pci_bar_is_memory(pcidev, i)) {
 				membar = pci_bar_addr(pcidev, i);
-				base = membar & ~4095;
-				offset = membar - base;
-				m = alloc_vpages(1);
-				
-				install_page((void *)read_cr3(), base, m);
-				pci_test.memaddr = m + offset;
+				pci_test.memaddr = ioremap(membar, PAGE_SIZE);
 			} else {
 				pci_test.iobar = pci_bar_addr(pcidev, i);
 			}
Signature · 1 line
1.8.3.1
Quoted 4 lines
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alexander Gordeev

author

2016-05-05 06:57 UTC #19 lore raw

Re: [kvm-unit-tests PATCH v6 11/11] x86: io: Factor out ioremap()

Quoted 11 lines · Andrew Jones

Quoted 4 lines
As of vmap_uncached - I guess since vmap() has a broader "scope" than just IO it would make sense to introduce vmap_prot() instead. Or add an extra page protection parameter to the existing vmap(). I could do that with a follow-up patch.
You don't really need to write vmap_uncached unless you plan to move unit tests to it too. We'd need sign-off from the unit test developers for that though. BTW, I'd still add a vmap_uncached, but it can certainly wrap a vmap_prot. I wouldn't extend vmap though. The default flags will almost always be correct, so having to pass them in (or a zero to be ORed with them) all the time wouldn't be nice.

I re-read it few times and still do not get if PCD for ioremap() is desirable or not :) So if yes, I would go this way:

	vmap_prot( ..., prot )
	{
		... = <default prot> | prot
	}

	#define vmap( ... )	vmap_prot( ..., 0 )

And if vmap_uncached is still needed (not sure why would?) - as you said:

	#define vmap_uncached( ... )	vmap_prot( ..., <PCD prot> )

Then turn x86 ioremap() to the PCD variant. And all of these with a follow-up series.

Quoted 1 line

drew

Andrew Jones

2016-05-05 10:03 UTC #20 lore raw

Re: [kvm-unit-tests PATCH v6 11/11] x86: io: Factor out ioremap()

Quoted 13 lines · Alexander Gordeev

Quoted 11 lines · Andrew Jones
Quoted 4 lines
As of vmap_uncached - I guess since vmap() has a broader "scope" than just IO it would make sense to introduce vmap_prot() instead. Or add an extra page protection parameter to the existing vmap(). I could do that with a follow-up patch.
You don't really need to write vmap_uncached unless you plan to move unit tests to it too. We'd need sign-off from the unit test developers for that though. BTW, I'd still add a vmap_uncached, but it can certainly wrap a vmap_prot. I wouldn't extend vmap though. The default flags will almost always be correct, so having to pass them in (or a zero to be ORed with them) all the time wouldn't be nice.

Quoted 2 lines

I re-read it few times and still do not get if PCD for ioremap() is desirable or not :) So if yes, I would go this way:

I think it is, but you'll need sign-off from the unit test developers for any unit test you modify to use it. If you choose not to move any unit tests to it, then there's really no point in writing it.

Quoted 4 lines

	vmap_prot( ..., prot )
	{
		... = <default prot> | prot

I wouldn't OR in the default prot here (except maybe PTE_PRESENT). Otherwise there's no point in creating this function, as it won't be flexible enough for all use cases.

Quoted 5 lines

	}

	#define vmap( ... )	vmap_prot( ..., 0 )
And if vmap_uncached is still needed (not sure why would?) - as you said:

Because here you OR in the defaults, plus PCD, making it easy for users to get uncached mappings.

Basically, vmap_prot is the super flexible API, users have full control, but it's not super user friendly for users that don't need full control. Everything else can wrap around it passing in defaults plus/minus tweaks to those defaults.

Quoted 11 lines

	#define vmap_uncached( ... )	vmap_prot( ..., <PCD prot> )
Then turn x86 ioremap() to the PCD variant. And all of these with a follow-up series.
Quoted 1 line
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html