Cleanup low-level arch code

PATCH Superseded Alexander Gordeev 2 participants 22 messages Apr 29, 2016

Subsystems (2)

KVM Library code

lore

Share & tools

lore

Alexander Gordeev

author

2016-04-29 11:38 UTC #0 lore raw

v3 cover Cleanup low-level arch code

Hi Andrew et al,

This is 3rd attempt to make mainly x86 arch code consistent with other architectures. I removed/omitted your Reviewed-bys from patches that changed as result of below updates:

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 (10):
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
io: Make ioremap() prototype conform to Linux one io/x86: Factor out ioremap()
x86: Remove size specifier from PAGE_SIZE constant

 arm/selftest.c          |  1 -
 lib/alloc.h             |  5 -----
 lib/arm/asm/page.h      |  2 --
 lib/arm/setup.c         |  1 -
 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   |  8 ++++++++
 lib/x86/{ => asm}/io.h  | 18 ++++++++++++++++--
 lib/x86/asm/page.h      | 28 ++++++++++++++++++++++++++++
 lib/x86/asm/pci.h       |  2 +-
 lib/x86/io.c            | 15 ++++++++++++++-
 lib/x86/smp.h           |  4 ----
 lib/x86/vm.c            | 17 -----------------
 lib/x86/vm.h            | 25 ++-----------------------
 x86/eventinj.c          |  7 +------
 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/svm.c               |  1 -
 x86/vmexit.c            | 11 ++---------
 x86/vmx.c               |  1 -
 x86/vmx_tests.c         |  1 -
 27 files changed, 89 insertions(+), 84 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-04-29 11:38 UTC #1 lore raw

PATCH v3 1/10 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-04-29 11:38 UTC #2 lore raw

PATCH v3 2/10 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]>

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

---
 arm/selftest.c          | 1 -
 lib/alloc.h             | 1 -
 lib/arm/asm/page.h      | 2 --
 lib/arm/setup.c         | 1 -
 lib/libcflat.h          | 2 ++
 lib/powerpc/asm/setup.h | 1 -
 6 files changed, 2 insertions(+), 6 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/arm/setup.c +0 -1

index 8c6172f..1cba23a 100644
@@ -13,7 +13,6 @@
 #include <libcflat.h>
 #include <libfdt/libfdt.h>
 #include <devicetree.h>
-#include <alloc.h>
 #include <asm/thread_info.h>
 #include <asm/setup.h>
 #include <asm/page.h>

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

Andrew Jones

2016-04-29 14:16 UTC #3 lore raw

Re: [kvm-unit-tests PATCH v3 02/10] Move phys_addr_t type definition to lib/libcflat.h

Quoted 59 lines · 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]>

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

---
 arm/selftest.c          | 1 -
 lib/alloc.h             | 1 -
 lib/arm/asm/page.h      | 2 --
 lib/arm/setup.c         | 1 -
 lib/libcflat.h          | 2 ++
 lib/powerpc/asm/setup.h | 1 -
 6 files changed, 2 insertions(+), 6 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/arm/setup.c +0 -0

index 8c6172f..1cba23a 100644

lib/arm/setup.c line 16

Quoted 4 lines

lib/arm/setup.c +0 -1

@@ -13,7 +13,6 @@
 #include <libcflat.h>
 #include <libfdt/libfdt.h>
 #include <devicetree.h>
-#include <alloc.h>

grrrr..... I pointed out in the last review[*] that this include should be here.

[*] http://www.spinics.net/lists/kvm/msg131441.html

Quoted 35 lines

lib/arm/setup.c +0 -0

 #include <asm/thread_info.h>
 #include <asm/setup.h>
 #include <asm/page.h>

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 · 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-04-29 11:38 UTC #4 lore raw

PATCH v3 3/10 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]>

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

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

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

new file mode 100644
index 0000000..54f6471
@@ -0,0 +1,27 @@
+#ifndef _ASM_X86_PAGE_H_
+#define _ASM_X86_PAGE_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)
+
+#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

Andrew Jones

2016-04-29 14:19 UTC #5 lore raw

Re: [kvm-unit-tests PATCH v3 03/10] x86: Introduce lib/x86/asm/page.h

Quoted 7 lines · 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]>
Signed-off-by Alexander Gordeev <[email protected]>

There's nothing different with this version of the patch than the last posting. I gave my r-b on that. Why didn't you pick up the tag? I don't want to review the same patch over and over.

Thanks,
drew

Quoted 103 lines

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

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

new file mode 100644
index 0000000..54f6471
@@ -0,0 +1,27 @@
+#ifndef _ASM_X86_PAGE_H_
+#define _ASM_X86_PAGE_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)
+
+#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-04-29 11:38 UTC #6 lore raw

PATCH v3 4/10 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]>

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

Andrew Jones

Reviewed-by on v3 4/10

2016-04-29 14:26 UTC #7 lore raw

Re: [kvm-unit-tests PATCH v3 04/10] x86: Introduce lib/x86/asm/io.h

Quoted 16 lines · 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]>
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 -

Quoted 8 lines

 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%)

Assuming x86 (both 32-bit and 64-bit) have been at least compile tested with all these patches one at a time, i.e. no individual patch breaks the build, even temporarily in between patches. Then,

Reviewed-by Andrew Jones <[email protected]>

Quoted 217 lines

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-04-29 11:38 UTC #8 lore raw

PATCH v3 5/10 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]>

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

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

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

new file mode 100644
index 0000000..0ca1c56
@@ -0,0 +1,8 @@
+#ifndef _ASM_X86_BARRIER_H_
+#define _ASM_X86_BARRIER_H_
+
+#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

Andrew Jones

Reviewed-by on v3 5/10

2016-04-29 14:29 UTC #9 lore raw

Re: [kvm-unit-tests PATCH v3 05/10] x86: Introduce lib/x86/asm/barrier.h

Quoted 49 lines · 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]>

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

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

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

new file mode 100644
index 0000000..0ca1c56
@@ -0,0 +1,8 @@
+#ifndef _ASM_X86_BARRIER_H_
+#define _ASM_X86_BARRIER_H_
+
+#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 +0 -0

index 9a971ef..6382938 100644

x86/hyperv_stimer.c line 11

Quoted 4 lines

x86/hyperv_stimer.c +1 -0

@@ -8,6 +8,7 @@
 #include "smp.h"
 #include "atomic.h"
 #include "hyperv.h"
+#include "asm/barrier.h"

This is new from the last version. I guess because you no longer add barrier.h to io.h.

Quoted 18 lines

x86/hyperv_stimer.c +0 -0

 #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

I would have been OK with you leaving my r-b on this patch, as it's just minor, but, anyway, here it is again

Reviewed-by Andrew Jones <[email protected]>

Alexander Gordeev

author

2016-04-29 11:38 UTC #10 lore raw

PATCH v3 6/10 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]>

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

Andrew Jones

Reviewed-by on v3 6/10

2016-04-29 14:30 UTC #11 lore raw

Re: [kvm-unit-tests PATCH v3 06/10] io: Separate overrides of virt_to_phys() and phys_to_virt()

Quoted 5 lines · 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.

Quoted 7 lines

Cc Andrew Jones <[email protected]>
Cc Thomas Huth <[email protected]>
Cc Radim Krčmář <[email protected]>
Signed-off-by Alexander Gordeev <[email protected]>
---
 lib/asm-generic/io.h | 2 ++
 1 file changed, 2 insertions(+)

Reviewed-by Andrew Jones <[email protected]>

Quoted 18 lines

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-04-29 11:38 UTC #12 lore raw

PATCH v3 7/10 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..99dd6a5 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-04-29 11:38 UTC #13 lore raw

PATCH v3 8/10 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 99dd6a5..1710cc4 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-04-29 11:38 UTC #14 lore raw

PATCH v3 9/10 io/x86: Factor out ioremap()

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/io.h   |  2 ++
 lib/x86/asm/page.h |  1 +
 lib/x86/io.c       | 15 ++++++++++++++-
 x86/vmexit.c       | 10 ++--------
 4 files changed, 19 insertions(+), 9 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/asm/page.h +1 -0

index 54f6471..db834ba 100644
@@ -7,6 +7,7 @@
 #else
 #define LARGE_PAGE_SIZE (1024 * PAGE_SIZE)
 #endif
+#define PAGE_MASK   (~(PAGE_SIZE-1))
 
 #define PTE_PRESENT (1ull << 0)
 #define PTE_PSE     (1ull << 7)

lib/x86/io.c +14 -1

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

@@ -81,3 +81,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 off = phys_addr - base;
+	ulong nr = (off + size + (PAGE_SIZE - 1)) / PAGE_SIZE;
+	void *page = alloc_vpages(nr);
+
+	install_page((void *)read_cr3(), base, page);
+
+	return page + off;
+}
+

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-04-29 14:48 UTC #15 lore raw

Re: [kvm-unit-tests PATCH v3 09/10] io/x86: Factor out ioremap()

Quoted 27 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/io.h   |  2 ++
 lib/x86/asm/page.h |  1 +
 lib/x86/io.c       | 15 ++++++++++++++-
 x86/vmexit.c       | 10 ++--------
 4 files changed, 19 insertions(+), 9 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/asm/page.h +0 -0

index 54f6471..db834ba 100644

lib/x86/asm/page.h line 10

Quoted 4 lines

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

@@ -7,6 +7,7 @@
 #else
 #define LARGE_PAGE_SIZE (1024 * PAGE_SIZE)
 #endif
+#define PAGE_MASK   (~(PAGE_SIZE-1))

Hmm... I see two x86 unit tests have their own PAGE_MASK definitions (x86/vmx.h, x86/access.c). I agree it should be in asm/page.h instead, but looks like more cleaning is in order then...

lib/x86/io.c line 3

Quoted 12 lines

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

 #define PTE_PRESENT (1ull << 0)
 #define PTE_PSE     (1ull << 7)

lib/x86/io.c +1 -1

index d396d42..9a84e6b 100644
@@ -1,6 +1,6 @@
 #include "libcflat.h"
+#include "vm.h"
 #include "smp.h"
-#include "asm/io.h"

Why remove this include? io.c uses stuff from asm/io.h. I wouldn't remove it just because vm.h also includes it. Also, I guess io.c should include asm/page.h now.

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

Quoted 13 lines

lib/x86/io.c +6 -0

 #ifndef USE_SERIAL
 #define USE_SERIAL
 #endif

@@ -81,3 +81,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 off = phys_addr - base;
+	ulong nr = (off + size + (PAGE_SIZE - 1)) / PAGE_SIZE;

nit: I prefer
nr = ALIGN(off + size, PAGE_SIZE) >> PAGE_SHIFT

hmm, x86 doesn't have a PAGE_SHIFT yet. Should probably add that along with PAGE_MASK to asm/page.h

Quoted 42 lines

lib/x86/io.c +7 -0

+	void *page = alloc_vpages(nr);
+
+	install_page((void *)read_cr3(), base, page);
+
+	return page + off;
+}
+

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-04-29 18:58 UTC #16 lore raw

Re: [kvm-unit-tests PATCH v3 09/10] io/x86: Factor out ioremap()

Quoted 6 lines · Andrew Jones

Quoted 6 lines
lib/x86/asm/page.h +0 -0
index 54f6471..db834ba 100644
@@ -7,6 +7,7 @@
 #else

Quoted 8 lines

lib/x86/asm/page.h line 10
Quoted 3 lines
lib/x86/asm/page.h +1 -0
 #define LARGE_PAGE_SIZE (1024 * PAGE_SIZE)
 #endif
+#define PAGE_MASK   (~(PAGE_SIZE-1))
Hmm... I see two x86 unit tests have their own PAGE_MASK definitions (x86/vmx.h, x86/access.c). I agree it should be in asm/page.h instead, but looks like more cleaning is in order then...

Yes, I am in a rush to get PCI stuff done and thought those two could wait.

Quoted 5 lines

Quoted 5 lines
lib/x86/io.c +0 -0
index d396d42..9a84e6b 100644
@@ -1,6 +1,6 @@

Quoted 8 lines

lib/x86/io.c line 3
Quoted 4 lines
lib/x86/io.c +1 -1
 #include "libcflat.h"
+#include "vm.h"
 #include "smp.h"
-#include "asm/io.h"
Why remove this include? io.c uses stuff from asm/io.h. I wouldn't remove it just because vm.h also includes it. Also, I guess io.c should include asm/page.h now.

I simply took your words on header inclusion too literally and removed it :) But given that vm.h in x86 code is so ubiquitous and already includes the two headers it did not seem completely unfounded :)

Will re-include both.

Quoted 17 lines

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

Quoted 13 lines

lib/x86/io.c +6 -0

 #ifndef USE_SERIAL
 #define USE_SERIAL
 #endif

@@ -81,3 +81,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 off = phys_addr - base;
+	ulong nr = (off + size + (PAGE_SIZE - 1)) / PAGE_SIZE;

nit: I prefer
nr = ALIGN(off + size, PAGE_SIZE) >> PAGE_SHIFT

Quoted 2 lines

hmm, x86 doesn't have a PAGE_SHIFT yet. Should probably add that along with PAGE_MASK to asm/page.h

Would you prefer to add it in this patch or in patch 3 "x86: Introduce lib/x86/asm/page.h"?

Also what about doing it the way it normally done?

#define PAGE_SHIFT 12
#define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
#define PAGE_MASK (~(PAGE_SIZE-1))

Thanks!

Andrew Jones

2016-04-29 19:16 UTC #17 lore raw

Re: [kvm-unit-tests PATCH v3 09/10] io/x86: Factor out ioremap()

Quoted 16 lines · Alexander Gordeev

Quoted 6 lines · Andrew Jones
Quoted 6 lines
lib/x86/asm/page.h +0 -0
index 54f6471..db834ba 100644
@@ -7,6 +7,7 @@
 #else
Quoted 8 lines
lib/x86/asm/page.h line 10
Quoted 3 lines
lib/x86/asm/page.h +1 -0
 #define LARGE_PAGE_SIZE (1024 * PAGE_SIZE)
 #endif
+#define PAGE_MASK   (~(PAGE_SIZE-1))
Hmm... I see two x86 unit tests have their own PAGE_MASK definitions (x86/vmx.h, x86/access.c). I agree it should be in asm/page.h instead, but looks like more cleaning is in order then...

Quoted 2 lines

Yes, I am in a rush to get PCI stuff done and thought those two could wait.

I don't think they can wait. The problem is that if somebody modifies those files to include something that includes asm/page.h then things will break for them, and they'll have to fix it. We shouldn't allow that to happen.

Quoted 15 lines

Quoted 14 lines
lib/x86/io.c line 3
Quoted 10 lines
lib/x86/io.c +1 -1
index d396d42..9a84e6b 100644
@@ -1,6 +1,6 @@
 #include "libcflat.h"
+#include "vm.h"
 #include "smp.h"
-#include "asm/io.h"
Why remove this include? io.c uses stuff from asm/io.h. I wouldn't remove it just because vm.h also includes it. Also, I guess io.c should include asm/page.h now.

Quoted 1 line

I simply took your words on header inclusion too literally and removed it :)

But I said that we strive to explicitly include everything we depend on, rather than dropping includes since they're already indirectly included. So you shouldn't have removed it based on what I said.

Quoted 22 lines

But given that vm.h in x86 code is so ubiquitous and already includes the two headers it did not seem completely unfounded :)
Will re-include both.
Quoted 17 lines
lib/x86/io.c void exit(int code) · line 89
Quoted 13 lines
lib/x86/io.c +6 -0
 #ifndef USE_SERIAL
 #define USE_SERIAL
 #endif
@@ -81,3 +81,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 off = phys_addr - base;
+	ulong nr = (off + size + (PAGE_SIZE - 1)) / PAGE_SIZE;
nit: I prefer
nr = ALIGN(off + size, PAGE_SIZE) >> PAGE_SHIFT

Quoted 5 lines

Quoted 2 lines
hmm, x86 doesn't have a PAGE_SHIFT yet. Should probably add that along with PAGE_MASK to asm/page.h
Would you prefer to add it in this patch or in patch 3 "x86: Introduce lib/x86/asm/page.h"?

If patch 3 is only code motion at this time (I think that's the case), then it should stay only code motion. Then we introduce new defines, and cleanup the redundant (potentially conflicting defines) in a separate patch.

Quoted 6 lines

Also what about doing it the way it normally done?
#define PAGE_SHIFT 12
#define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
#define PAGE_MASK (~(PAGE_SIZE-1))

These work for me.

Thanks,
drew

Quoted 6 lines

Thanks!
--
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-04-29 11:38 UTC #18 lore raw

PATCH v3 10/10 x86: Remove size specifier from PAGE_SIZE constant

In reply to Alexander Gordeev

Currently PAGE_SIZE constant defaults to unsigned long which does not appear brings any value.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

index db834ba..365d269 100644
@@ -1,7 +1,7 @@
 #ifndef _ASM_X86_PAGE_H_
 #define _ASM_X86_PAGE_H_
 
-#define PAGE_SIZE   4096ul
+#define PAGE_SIZE   4096
 #ifdef __x86_64__
 #define LARGE_PAGE_SIZE (512 * PAGE_SIZE)
 #else

Signature · 1 line

1.8.3.1

Andrew Jones

NAK

2016-04-29 14:53 UTC #19 lore raw

Re: [kvm-unit-tests PATCH v3 10/10] x86: Remove size specifier from PAGE_SIZE constant

Quoted 16 lines · Alexander Gordeev

Currently PAGE_SIZE constant defaults to unsigned long which does not appear brings any value.
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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
lib/x86/asm/page.h +0 -0
index db834ba..365d269 100644

lib/x86/asm/page.h line 4

Quoted 5 lines

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

@@ -1,7 +1,7 @@
 #ifndef _ASM_X86_PAGE_H_
 #define _ASM_X86_PAGE_H_
 
-#define PAGE_SIZE   4096ul
+#define PAGE_SIZE   4096

nack

If PAGE_SIZE is only an int (32bits) then PAGE_MASK, which is (~(PAGE_SIZE-1)), would only be 0xfffff000, rather than 0xfffffffffffff000, as it should be.

Quoted 10 lines

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

 #ifdef __x86_64__
 #define LARGE_PAGE_SIZE (512 * PAGE_SIZE)
 #else

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-04-29 17:52 UTC #20 lore raw

Re: [kvm-unit-tests PATCH v3 10/10] x86: Remove size specifier from PAGE_SIZE constant

Quoted 8 lines · Andrew Jones

lib/x86/asm/page.h line 4
Quoted 2 lines
lib/x86/asm/page.h +1 -1
-#define PAGE_SIZE   4096ul
+#define PAGE_SIZE   4096
nack
If PAGE_SIZE is only an int (32bits) then PAGE_MASK, which is (~(PAGE_SIZE-1)), would only be 0xfffff000, rather than 0xfffffffffffff000, as it should be.

Oh, right. Thanks!

Andrew Jones

2016-04-29 19:09 UTC #21 lore raw

Re: [kvm-unit-tests PATCH v3 10/10] x86: Remove size specifier from PAGE_SIZE constant

Quoted 11 lines · Alexander Gordeev

Quoted 8 lines · Andrew Jones
lib/x86/asm/page.h line 4
Quoted 2 lines
lib/x86/asm/page.h +1 -1
-#define PAGE_SIZE   4096ul
+#define PAGE_SIZE   4096
nack
If PAGE_SIZE is only an int (32bits) then PAGE_MASK, which is (~(PAGE_SIZE-1)), would only be 0xfffff000, rather than 0xfffffffffffff000, as it should be.
Oh, right. Thanks!

Well, sign-extension will apply to it. So, since we're almost always going to use PAGE_MASK with a unsigned long variable, then most likely it won't matter in practice. But, anyway, I'd keep the UL and drop this patch :-)

drew

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