xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/5] Minimal build for RISCV
@ 2021-05-14 18:53 Connor Davis
  2021-05-14 18:53 ` [PATCH v3 1/5] xen/char: Default HAS_NS16550 to y only for X86 and ARM Connor Davis
                   ` (5 more replies)
  0 siblings, 6 replies; 31+ messages in thread
From: Connor Davis @ 2021-05-14 18:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Bobby Eshleman, Alistair Francis, Connor Davis, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Julien Grall,
	Stefano Stabellini, Wei Liu, Paul Durrant, Doug Goldstein

Hi all,

This series introduces a minimal build for RISCV. It is based on Bobby's
previous work from last year[0] rebased onto current Xen.

This series provides the patches necessary to get a minimal build
working. The build is "minimal" in the sense that it only supports
building TARGET=head.o. The arch/riscv/head.S is just a simple while(1).

The first 3 patches are mods to non-RISCV bits that enable building a
config with:

  !CONFIG_HAS_NS16550
  !CONFIG_HAS_PASSTHROUGH
  !CONFIG_GRANT_TABLE

respectively. The fourth patch adds the make/Kconfig boilerplate
alongside head.S and asm-riscv/config.h (head.S references ENTRY
that is defined in asm-riscv/config.h).

The last adds a docker container for doing the build. To build from the
docker container (after creating it locally), you can run the following:

  $ make XEN_TARGET_ARCH=riscv64 SUBSYSTEMS=xen -C xen TARGET=head.o

--
Changes since v2:
  - Reduced number of riscv files added to ease review

Changes since v1:
  - Dropped "xen/sched: Fix build when NR_CPUS == 1" since this was
    fixed for 4.15
  - Moved #ifdef-ary around iommu_enabled to iommu.h
  - Moved struct grant_table declaration above ifdef CONFIG_GRANT_TABLE
    instead of defining an empty struct when !CONFIG_GRANT_TABLE

Connor Davis (5):
  xen/char: Default HAS_NS16550 to y only for X86 and ARM
  xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH
  xen: Fix build when !CONFIG_GRANT_TABLE
  xen: Add files needed for minimal riscv build
  automation: Add container for riscv64 builds

 automation/build/archlinux/riscv64.dockerfile |  33 ++++++
 automation/scripts/containerize               |   1 +
 config/riscv64.mk                             |   5 +
 xen/Makefile                                  |   8 +-
 xen/arch/riscv/Kconfig                        |  52 +++++++++
 xen/arch/riscv/Kconfig.debug                  |   0
 xen/arch/riscv/Makefile                       |   0
 xen/arch/riscv/Rules.mk                       |   0
 xen/arch/riscv/arch.mk                        |  16 +++
 xen/arch/riscv/asm-offsets.c                  |   0
 xen/arch/riscv/configs/riscv64_defconfig      |  12 ++
 xen/arch/riscv/head.S                         |   6 +
 xen/common/memory.c                           |  10 ++
 xen/drivers/char/Kconfig                      |   2 +-
 xen/include/asm-riscv/config.h                | 110 ++++++++++++++++++
 xen/include/xen/grant_table.h                 |   3 +-
 xen/include/xen/iommu.h                       |   8 +-
 17 files changed, 261 insertions(+), 5 deletions(-)
 create mode 100644 automation/build/archlinux/riscv64.dockerfile
 create mode 100644 config/riscv64.mk
 create mode 100644 xen/arch/riscv/Kconfig
 create mode 100644 xen/arch/riscv/Kconfig.debug
 create mode 100644 xen/arch/riscv/Makefile
 create mode 100644 xen/arch/riscv/Rules.mk
 create mode 100644 xen/arch/riscv/arch.mk
 create mode 100644 xen/arch/riscv/asm-offsets.c
 create mode 100644 xen/arch/riscv/configs/riscv64_defconfig
 create mode 100644 xen/arch/riscv/head.S
 create mode 100644 xen/include/asm-riscv/config.h

-- 
2.31.1



^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 1/5] xen/char: Default HAS_NS16550 to y only for X86 and ARM
  2021-05-14 18:53 [PATCH v3 0/5] Minimal build for RISCV Connor Davis
@ 2021-05-14 18:53 ` Connor Davis
  2021-05-16 22:48   ` Alistair Francis
  2021-05-17 11:56   ` Jan Beulich
  2021-05-14 18:53 ` [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH Connor Davis
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 31+ messages in thread
From: Connor Davis @ 2021-05-14 18:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Bobby Eshleman, Alistair Francis, Connor Davis, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Julien Grall,
	Stefano Stabellini, Wei Liu

Defaulting to yes only for X86 and ARM reduces the requirements
for a minimal build when porting new architectures.

Signed-off-by: Connor Davis <connojdavis@gmail.com>
---
 xen/drivers/char/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index b572305657..b15b0c8d6a 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -1,6 +1,6 @@
 config HAS_NS16550
 	bool "NS16550 UART driver" if ARM
-	default y
+	default y if (ARM || X86)
 	help
 	  This selects the 16550-series UART support. For most systems, say Y.
 
-- 
2.31.1



^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH
  2021-05-14 18:53 [PATCH v3 0/5] Minimal build for RISCV Connor Davis
  2021-05-14 18:53 ` [PATCH v3 1/5] xen/char: Default HAS_NS16550 to y only for X86 and ARM Connor Davis
@ 2021-05-14 18:53 ` Connor Davis
  2021-05-17 11:16   ` Jan Beulich
  2021-05-14 18:53 ` [PATCH v3 3/5] xen: Fix build when !CONFIG_GRANT_TABLE Connor Davis
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 31+ messages in thread
From: Connor Davis @ 2021-05-14 18:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Bobby Eshleman, Alistair Francis, Connor Davis, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Julien Grall,
	Stefano Stabellini, Wei Liu, Paul Durrant

The variables iommu_enabled and iommu_dont_flush_iotlb are defined in
drivers/passthrough/iommu.c and are referenced in common code, which
causes the link to fail when !CONFIG_HAS_PASSTHROUGH.

Guard references to these variables in common code so that xen
builds when !CONFIG_HAS_PASSTHROUGH.

Signed-off-by: Connor Davis <connojdavis@gmail.com>
---
 xen/common/memory.c     | 10 ++++++++++
 xen/include/xen/iommu.h |  8 +++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index b5c70c4b85..72a6b70cb5 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
     p2m_type_t p2mt;
 #endif
     mfn_t mfn;
+#ifdef CONFIG_HAS_PASSTHROUGH
     bool *dont_flush_p, dont_flush;
+#endif
     int rc;
 
 #ifdef CONFIG_X86
@@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
      * Since we're likely to free the page below, we need to suspend
      * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes.
      */
+#ifdef CONFIG_HAS_PASSTHROUGH
     dont_flush_p = &this_cpu(iommu_dont_flush_iotlb);
     dont_flush = *dont_flush_p;
     *dont_flush_p = false;
+#endif
 
     rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
 
+#ifdef CONFIG_HAS_PASSTHROUGH
     *dont_flush_p = dont_flush;
+#endif
 
     /*
      * With the lack of an IOMMU on some platforms, domains with DMA-capable
@@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
     xatp->gpfn += start;
     xatp->size -= start;
 
+#ifdef CONFIG_HAS_PASSTHROUGH
     if ( is_iommu_enabled(d) )
     {
        this_cpu(iommu_dont_flush_iotlb) = 1;
        extra.ppage = &pages[0];
     }
+#endif
 
     while ( xatp->size > done )
     {
@@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
         }
     }
 
+#ifdef CONFIG_HAS_PASSTHROUGH
     if ( is_iommu_enabled(d) )
     {
         int ret;
@@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
         if ( unlikely(ret) && rc >= 0 )
             rc = ret;
     }
+#endif
 
     return rc;
 }
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 460755df29..d878a93269 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -51,9 +51,15 @@ static inline bool_t dfn_eq(dfn_t x, dfn_t y)
     return dfn_x(x) == dfn_x(y);
 }
 
-extern bool_t iommu_enable, iommu_enabled;
+extern bool_t iommu_enable;
 extern bool force_iommu, iommu_quarantine, iommu_verbose;
 
+#ifdef CONFIG_HAS_PASSTHROUGH
+extern bool_t iommu_enabled;
+#else
+#define iommu_enabled false
+#endif
+
 #ifdef CONFIG_X86
 extern enum __packed iommu_intremap {
    /*
-- 
2.31.1



^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 3/5] xen: Fix build when !CONFIG_GRANT_TABLE
  2021-05-14 18:53 [PATCH v3 0/5] Minimal build for RISCV Connor Davis
  2021-05-14 18:53 ` [PATCH v3 1/5] xen/char: Default HAS_NS16550 to y only for X86 and ARM Connor Davis
  2021-05-14 18:53 ` [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH Connor Davis
@ 2021-05-14 18:53 ` Connor Davis
  2021-05-17 11:22   ` Jan Beulich
  2021-05-14 18:53 ` [PATCH v3 4/5] xen: Add files needed for minimal riscv build Connor Davis
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 31+ messages in thread
From: Connor Davis @ 2021-05-14 18:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Bobby Eshleman, Alistair Francis, Connor Davis, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Julien Grall,
	Stefano Stabellini, Wei Liu

Move struct grant_table; in grant_table.h above
ifdef CONFIG_GRANT_TABLE. This fixes the following:

/build/xen/include/xen/grant_table.h:84:50: error: 'struct grant_table'
declared inside parameter list will not be visible outside of this
definition or declaration [-Werror]
   84 | static inline int mem_sharing_gref_to_gfn(struct grant_table *gt,
      |

Signed-off-by: Connor Davis <connojdavis@gmail.com>
---
 xen/include/xen/grant_table.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 63b6dc78f4..9f8b7e66c1 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -28,9 +28,10 @@
 #include <public/grant_table.h>
 #include <asm/grant_table.h>
 
-#ifdef CONFIG_GRANT_TABLE
 struct grant_table;
 
+#ifdef CONFIG_GRANT_TABLE
+
 extern unsigned int opt_max_grant_frames;
 
 /* Create/destroy per-domain grant table context. */
-- 
2.31.1



^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 4/5] xen: Add files needed for minimal riscv build
  2021-05-14 18:53 [PATCH v3 0/5] Minimal build for RISCV Connor Davis
                   ` (2 preceding siblings ...)
  2021-05-14 18:53 ` [PATCH v3 3/5] xen: Fix build when !CONFIG_GRANT_TABLE Connor Davis
@ 2021-05-14 18:53 ` Connor Davis
  2021-05-14 21:53   ` Bob Eshleman
  2021-05-17 11:51   ` Jan Beulich
  2021-05-14 18:53 ` [PATCH v3 5/5] automation: Add container for riscv64 builds Connor Davis
  2021-05-17 23:20 ` [PATCH v3 0/5] Minimal build for RISCV Roman Shaposhnik
  5 siblings, 2 replies; 31+ messages in thread
From: Connor Davis @ 2021-05-14 18:53 UTC (permalink / raw)
  To: xen-devel
  Cc: Bobby Eshleman, Alistair Francis, Connor Davis, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Julien Grall,
	Stefano Stabellini, Wei Liu

Add arch-specific makefiles and configs needed to build for
riscv64. Also add a minimal head.S that is a simple infinite loop.
head.o can be built with

$ make XEN_TARGET_ARCH=riscv64 SUBSYSTEMS=xen -C xen TARGET=head.o

No other TARGET is supported at the moment.

Signed-off-by: Connor Davis <connojdavis@gmail.com>
---
 config/riscv64.mk                        |   5 ++
 xen/Makefile                             |   8 +-
 xen/arch/riscv/Kconfig                   |  52 +++++++++++
 xen/arch/riscv/Kconfig.debug             |   0
 xen/arch/riscv/Makefile                  |   0
 xen/arch/riscv/Rules.mk                  |   0
 xen/arch/riscv/arch.mk                   |  16 ++++
 xen/arch/riscv/asm-offsets.c             |   0
 xen/arch/riscv/configs/riscv64_defconfig |  12 +++
 xen/arch/riscv/head.S                    |   6 ++
 xen/include/asm-riscv/config.h           | 110 +++++++++++++++++++++++
 11 files changed, 207 insertions(+), 2 deletions(-)
 create mode 100644 config/riscv64.mk
 create mode 100644 xen/arch/riscv/Kconfig
 create mode 100644 xen/arch/riscv/Kconfig.debug
 create mode 100644 xen/arch/riscv/Makefile
 create mode 100644 xen/arch/riscv/Rules.mk
 create mode 100644 xen/arch/riscv/arch.mk
 create mode 100644 xen/arch/riscv/asm-offsets.c
 create mode 100644 xen/arch/riscv/configs/riscv64_defconfig
 create mode 100644 xen/arch/riscv/head.S
 create mode 100644 xen/include/asm-riscv/config.h

diff --git a/config/riscv64.mk b/config/riscv64.mk
new file mode 100644
index 0000000000..a5a21e5fa2
--- /dev/null
+++ b/config/riscv64.mk
@@ -0,0 +1,5 @@
+CONFIG_RISCV := y
+CONFIG_RISCV_64 := y
+CONFIG_RISCV_$(XEN_OS) := y
+
+CONFIG_XEN_INSTALL_SUFFIX :=
diff --git a/xen/Makefile b/xen/Makefile
index 9f3be7766d..60de4cc6cd 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -26,7 +26,9 @@ MAKEFLAGS += -rR
 EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
 
 ARCH=$(XEN_TARGET_ARCH)
-SRCARCH=$(shell echo $(ARCH) | sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g')
+SRCARCH=$(shell echo $(ARCH) | \
+	  sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
+	      -e s'/riscv.*/riscv/g')
 
 # Don't break if the build process wasn't called from the top level
 # we need XEN_TARGET_ARCH to generate the proper config
@@ -35,7 +37,8 @@ include $(XEN_ROOT)/Config.mk
 # Set ARCH/SUBARCH appropriately.
 export TARGET_SUBARCH  := $(XEN_TARGET_ARCH)
 export TARGET_ARCH     := $(shell echo $(XEN_TARGET_ARCH) | \
-                            sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g')
+                            sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
+			        -e s'/riscv.*/riscv/g')
 
 # Allow someone to change their config file
 export KCONFIG_CONFIG ?= .config
@@ -335,6 +338,7 @@ _clean: delete-unfresh-files
 	$(MAKE) $(clean) xsm
 	$(MAKE) $(clean) crypto
 	$(MAKE) $(clean) arch/arm
+	$(MAKE) $(clean) arch/riscv
 	$(MAKE) $(clean) arch/x86
 	$(MAKE) $(clean) test
 	$(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) clean
diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
new file mode 100644
index 0000000000..d4bbd4294e
--- /dev/null
+++ b/xen/arch/riscv/Kconfig
@@ -0,0 +1,52 @@
+config 64BIT
+	bool
+
+config RISCV_64
+	bool
+	depends on 64BIT
+
+config RISCV
+	def_bool y
+
+config ARCH_DEFCONFIG
+	string
+	default "arch/riscv/configs/riscv64_defconfig" if RISCV_64
+
+menu "Architecture Features"
+
+source "arch/Kconfig"
+
+endmenu
+
+menu "ISA Selection"
+
+choice
+	prompt "Base ISA"
+        default RISCV_ISA_RV64IMA
+        help
+          This selects the base ISA extensions that Xen will target.
+
+config RISCV_ISA_RV64IMA
+	bool "RV64IMA"
+        select 64BIT
+        select RISCV_64
+        help
+           Use the RV64I base ISA, plus the "M" and "A" extensions
+           for integer multiply/divide and atomic instructions, respectively.
+
+endchoice
+
+config RISCV_ISA_C
+	bool "Compressed extension"
+        help
+           Add "C" to the ISA subsets that the toolchain is allowed
+           to emit when building Xen, which results in compressed
+           instructions in the Xen binary.
+
+           If unsure, say N.
+
+endmenu
+
+source "common/Kconfig"
+
+source "drivers/Kconfig"
diff --git a/xen/arch/riscv/Kconfig.debug b/xen/arch/riscv/Kconfig.debug
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/xen/arch/riscv/Rules.mk b/xen/arch/riscv/Rules.mk
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk
new file mode 100644
index 0000000000..10229c5440
--- /dev/null
+++ b/xen/arch/riscv/arch.mk
@@ -0,0 +1,16 @@
+########################################
+# RISCV-specific definitions
+
+ifeq ($(CONFIG_RISCV_64),y)
+    CFLAGS += -mabi=lp64
+endif
+
+riscv-march-$(CONFIG_RISCV_ISA_RV64IMA) := rv64ima
+riscv-march-$(CONFIG_RISCV_ISA_C)       := $(riscv-march-y)c
+
+# Note that -mcmodel=medany is used so that Xen can be mapped
+# into the upper half _or_ the lower half of the address space.
+# -mcmodel=medlow would force Xen into the lower half.
+
+CFLAGS += -march=$(riscv-march-y) -mstrict-align -mcmodel=medany
+CFLAGS += -I$(BASEDIR)/include
diff --git a/xen/arch/riscv/asm-offsets.c b/xen/arch/riscv/asm-offsets.c
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/xen/arch/riscv/configs/riscv64_defconfig b/xen/arch/riscv/configs/riscv64_defconfig
new file mode 100644
index 0000000000..664a5d2378
--- /dev/null
+++ b/xen/arch/riscv/configs/riscv64_defconfig
@@ -0,0 +1,12 @@
+# CONFIG_SCHED_CREDIT is not set
+# CONFIG_SCHED_RTDS is not set
+# CONFIG_SCHED_NULL is not set
+# CONFIG_SCHED_ARINC653 is not set
+# CONFIG_TRACEBUFFER is not set
+# CONFIG_DEBUG is not set
+# CONFIG_DEBUG_INFO is not set
+# CONFIG_HYPFS is not set
+# CONFIG_GRANT_TABLE is not set
+# CONFIG_SPECULATIVE_HARDEN_ARRAY is not set
+
+CONFIG_EXPERT=y
diff --git a/xen/arch/riscv/head.S b/xen/arch/riscv/head.S
new file mode 100644
index 0000000000..0dbc27ba75
--- /dev/null
+++ b/xen/arch/riscv/head.S
@@ -0,0 +1,6 @@
+#include <asm/config.h>
+
+        .text
+
+ENTRY(start)
+        j  start
diff --git a/xen/include/asm-riscv/config.h b/xen/include/asm-riscv/config.h
new file mode 100644
index 0000000000..84cb436dc1
--- /dev/null
+++ b/xen/include/asm-riscv/config.h
@@ -0,0 +1,110 @@
+/******************************************************************************
+ * config.h
+ *
+ * A Linux-style configuration list.
+ */
+
+#ifndef __RISCV_CONFIG_H__
+#define __RISCV_CONFIG_H__
+
+#if defined(CONFIG_RISCV_64)
+# define LONG_BYTEORDER 3
+# define ELFSIZE 64
+#else
+# error "Unsupported RISCV variant"
+#endif
+
+#define BYTES_PER_LONG (1 << LONG_BYTEORDER)
+#define BITS_PER_LONG  (BYTES_PER_LONG << 3)
+#define POINTER_ALIGN  BYTES_PER_LONG
+
+#define BITS_PER_LLONG 64
+
+/* xen_ulong_t is always 64 bits */
+#define BITS_PER_XEN_ULONG 64
+
+#define CONFIG_RISCV 1
+#define CONFIG_RISCV_L1_CACHE_SHIFT 6
+
+#define CONFIG_PAGEALLOC_MAX_ORDER 18
+#define CONFIG_DOMU_MAX_ORDER      9
+#define CONFIG_HWDOM_MAX_ORDER     10
+
+#define OPT_CONSOLE_STR "dtuart"
+
+#ifdef CONFIG_RISCV_64
+#define MAX_VIRT_CPUS 128u
+#else
+#error "Unsupported RISCV variant"
+#endif
+
+#define INVALID_VCPU_ID MAX_VIRT_CPUS
+
+/* Linkage for RISCV */
+#ifdef __ASSEMBLY__
+#define ALIGN .align 2
+
+#define ENTRY(name)                                \
+  .globl name;                                     \
+  ALIGN;                                           \
+  name:
+#endif
+
+#include <xen/const.h>
+
+#ifdef CONFIG_RISCV_64
+
+/*
+ * RISC-V Layout:
+ *   0x0000000000000000 - 0x0000003fffffffff (256GB, L2 slots [0-255])
+ *     Unmapped
+ *   0x0000004000000000 - 0xffffffbfffffffff
+ *     Inaccessible: sv39 only supports 39-bit sign-extended VAs.
+ *   0xffffffc000000000 - 0xffffffc0001fffff (2MB, L2 slot [256])
+ *     Unmapped
+ *   0xffffffc000200000 - 0xffffffc0003fffff (2MB, L2 slot [256])
+ *     Xen text, data, bss
+ *   0xffffffc000400000 - 0xffffffc0005fffff (2MB, L2 slot [256])
+ *     Fixmap: special-purpose 4K mapping slots
+ *   0xffffffc000600000 - 0xffffffc0009fffff (4MB, L2 slot [256])
+ *     Early boot mapping of FDT
+ *   0xffffffc000a00000 - 0xffffffc000bfffff (2MB, L2 slot [256])
+ *     Early relocation address, used when relocating Xen and later
+ *     for livepatch vmap (if compiled in)
+ *   0xffffffc040000000 - 0xffffffc07fffffff (1GB, L2 slot [257])
+ *     VMAP: ioremap and early_ioremap
+ *   0xffffffc080000000 - 0xffffffc13fffffff (3GB, L2 slots [258..260])
+ *     Unmapped
+ *   0xffffffc140000000 - 0xffffffc1bfffffff (2GB, L2 slots [261..262])
+ *     Frametable: 48 bytes per page for 133GB of RAM
+ *   0xffffffc1c0000000 - 0xffffffe1bfffffff (128GB, L2 slots [263..390])
+ *     1:1 direct mapping of RAM
+ *   0xffffffe1c0000000 - 0xffffffffffffffff (121GB, L2 slots [391..511])
+ *     Unmapped
+ */
+
+#define L2_ENTRY_BITS  30
+#define L2_ENTRY_BYTES (_AC(1,UL) << L2_ENTRY_BITS)
+#define L2_ADDR(_slot)                                      \
+    (((_AC(_slot, UL) >> 8) * _AC(0xffffff8000000000,UL)) | \
+     (_AC(_slot, UL) << L2_ENTRY_BITS))
+
+#define XEN_VIRT_START         _AT(vaddr_t, L2_ADDR(256) + MB(2))
+#define HYPERVISOR_VIRT_START  XEN_VIRT_START
+
+#define FRAMETABLE_VIRT_START  _AT(vaddr_t, L2_ADDR(261))
+
+#endif /* CONFIG_RISCV_64 */
+
+#define STACK_ORDER            3
+#define STACK_SIZE             (PAGE_SIZE << STACK_ORDER)
+
+#endif /* __RISCV_CONFIG_H__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.31.1



^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 5/5] automation: Add container for riscv64 builds
  2021-05-14 18:53 [PATCH v3 0/5] Minimal build for RISCV Connor Davis
                   ` (3 preceding siblings ...)
  2021-05-14 18:53 ` [PATCH v3 4/5] xen: Add files needed for minimal riscv build Connor Davis
@ 2021-05-14 18:53 ` Connor Davis
  2021-05-14 21:01   ` Bob Eshleman
  2021-05-17 23:20 ` [PATCH v3 0/5] Minimal build for RISCV Roman Shaposhnik
  5 siblings, 1 reply; 31+ messages in thread
From: Connor Davis @ 2021-05-14 18:53 UTC (permalink / raw)
  To: xen-devel; +Cc: Bobby Eshleman, Alistair Francis, Connor Davis, Doug Goldstein

Add a container for cross-compiling xen to riscv64.
This just includes the cross-compiler and necessary packages for
building xen itself (packages for tools, stubdoms, etc., can be
added later).

Signed-off-by: Connor Davis <connojdavis@gmail.com>
---
 automation/build/archlinux/riscv64.dockerfile | 33 +++++++++++++++++++
 automation/scripts/containerize               |  1 +
 2 files changed, 34 insertions(+)
 create mode 100644 automation/build/archlinux/riscv64.dockerfile

diff --git a/automation/build/archlinux/riscv64.dockerfile b/automation/build/archlinux/riscv64.dockerfile
new file mode 100644
index 0000000000..505b623c01
--- /dev/null
+++ b/automation/build/archlinux/riscv64.dockerfile
@@ -0,0 +1,33 @@
+FROM archlinux
+LABEL maintainer.name="The Xen Project" \
+      maintainer.email="xen-devel@lists.xenproject.org"
+
+# Packages needed for the build
+RUN pacman --noconfirm --needed -Syu \
+    base-devel \
+    gcc \
+    git
+
+# Packages needed for QEMU
+RUN pacman --noconfirm --needed -Syu \
+    pixman \
+    python \
+    sh
+
+# There is a regression in GDB that causes an assertion error
+# when setting breakpoints, use this commit until it is fixed!
+RUN git clone --recursive -j$(nproc) --progress https://github.com/riscv/riscv-gnu-toolchain && \
+    cd riscv-gnu-toolchain/riscv-gdb && \
+    git checkout 1dd588507782591478882a891f64945af9e2b86c && \
+    cd  .. && \
+    ./configure --prefix=/opt/riscv && \
+    make linux -j$(nproc) && \
+    rm -R /riscv-gnu-toolchain
+
+# Add compiler path
+ENV PATH=/opt/riscv/bin/:${PATH}
+ENV CROSS_COMPILE=riscv64-unknown-linux-gnu-
+
+RUN useradd --create-home user
+USER user
+WORKDIR /build
diff --git a/automation/scripts/containerize b/automation/scripts/containerize
index b7c81559fb..59edf0ba40 100755
--- a/automation/scripts/containerize
+++ b/automation/scripts/containerize
@@ -26,6 +26,7 @@ BASE="registry.gitlab.com/xen-project/xen"
 case "_${CONTAINER}" in
     _alpine) CONTAINER="${BASE}/alpine:3.12" ;;
     _archlinux|_arch) CONTAINER="${BASE}/archlinux:current" ;;
+    _riscv64) CONTAINER="${BASE}/archlinux:riscv64" ;;
     _centos7) CONTAINER="${BASE}/centos:7" ;;
     _centos72) CONTAINER="${BASE}/centos:7.2" ;;
     _fedora) CONTAINER="${BASE}/fedora:29";;
-- 
2.31.1



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 5/5] automation: Add container for riscv64 builds
  2021-05-14 18:53 ` [PATCH v3 5/5] automation: Add container for riscv64 builds Connor Davis
@ 2021-05-14 21:01   ` Bob Eshleman
  2021-05-14 23:54     ` Connor Davis
  0 siblings, 1 reply; 31+ messages in thread
From: Bob Eshleman @ 2021-05-14 21:01 UTC (permalink / raw)
  To: Connor Davis, xen-devel; +Cc: Alistair Francis, Doug Goldstein

On 5/14/21 11:53 AM, Connor Davis wrote:
> +
> +# There is a regression in GDB that causes an assertion error
> +# when setting breakpoints, use this commit until it is fixed!
> +RUN git clone --recursive -j$(nproc) --progress https://github.com/riscv/riscv-gnu-toolchain && \
> +    cd riscv-gnu-toolchain/riscv-gdb && \
> +    git checkout 1dd588507782591478882a891f64945af9e2b86c && \
> +    cd  .. && \
> +    ./configure --prefix=/opt/riscv && \
> +    make linux -j$(nproc) && \
> +    rm -R /riscv-gnu-toolchai

What do you think about using the RISCV tool chain from the Arch
repos now?

I've also discovered that the sym table error avoided by the commit
pin can be worked around by removing already loaded symbols with
`file` (no args) prior to calling `file path/to/file` to load new
ones.  So if people did still want to use the container for
development, they could still use the gdb installed by pacman
(with the symbols caveat).

-- 
Bobby Eshleman
SE at Vates SAS


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 4/5] xen: Add files needed for minimal riscv build
  2021-05-14 18:53 ` [PATCH v3 4/5] xen: Add files needed for minimal riscv build Connor Davis
@ 2021-05-14 21:53   ` Bob Eshleman
  2021-05-14 23:47     ` Connor Davis
  2021-05-17 11:51   ` Jan Beulich
  1 sibling, 1 reply; 31+ messages in thread
From: Bob Eshleman @ 2021-05-14 21:53 UTC (permalink / raw)
  To: Connor Davis, xen-devel
  Cc: Alistair Francis, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu

On 5/14/21 11:53 AM, Connor Davis wrote:
> Add arch-specific makefiles and configs needed to build for
> riscv64. Also add a minimal head.S that is a simple infinite loop.
> head.o can be built with
> 
> $ make XEN_TARGET_ARCH=riscv64 SUBSYSTEMS=xen -C xen TARGET=head.o
> 

I recently realized that the Linux kernel build uses `ARCH=riscv`
with 32 vs 64 being differentiated by Kconfig opts (and __riscv_xlen).
I think `riscv64` was inherited by `arm64`.  This is something I'd
like to eventually change the Xen build to (i.e.,
`XEN_TARGET_ARCH=riscv make`) would it be possible for us to get that
in this series?

...

> diff --git a/xen/include/asm-riscv/config.h b/xen/include/asm-riscv/config.h
> new file mode 100644
> index 0000000000..84cb436dc1
> --- /dev/null
> +++ b/xen/include/asm-riscv/config.h
> @@ -0,0 +1,110 @@
> +/******************************************************************************
> + * config.h
> + *
> + * A Linux-style configuration list.
> + */
> +
> +#ifndef __RISCV_CONFIG_H__
> +#define __RISCV_CONFIG_H__
> +

...

> +
> +#ifdef CONFIG_RISCV_64
> +
> +/*
> + * RISC-V Layout:
> + *   0x0000000000000000 - 0x0000003fffffffff (256GB, L2 slots [0-255])
> + *     Unmapped
> + *   0x0000004000000000 - 0xffffffbfffffffff
> + *     Inaccessible: sv39 only supports 39-bit sign-extended VAs.
> + *   0xffffffc000000000 - 0xffffffc0001fffff (2MB, L2 slot [256])
> + *     Unmapped
> + *   0xffffffc000200000 - 0xffffffc0003fffff (2MB, L2 slot [256])
> + *     Xen text, data, bss
> + *   0xffffffc000400000 - 0xffffffc0005fffff (2MB, L2 slot [256])
> + *     Fixmap: special-purpose 4K mapping slots
> + *   0xffffffc000600000 - 0xffffffc0009fffff (4MB, L2 slot [256])
> + *     Early boot mapping of FDT
> + *   0xffffffc000a00000 - 0xffffffc000bfffff (2MB, L2 slot [256])
> + *     Early relocation address, used when relocating Xen and later
> + *     for livepatch vmap (if compiled in)
> + *   0xffffffc040000000 - 0xffffffc07fffffff (1GB, L2 slot [257])
> + *     VMAP: ioremap and early_ioremap
> + *   0xffffffc080000000 - 0xffffffc13fffffff (3GB, L2 slots [258..260])
> + *     Unmapped
> + *   0xffffffc140000000 - 0xffffffc1bfffffff (2GB, L2 slots [261..262])
> + *     Frametable: 48 bytes per page for 133GB of RAM
> + *   0xffffffc1c0000000 - 0xffffffe1bfffffff (128GB, L2 slots [263..390])
> + *     1:1 direct mapping of RAM
> + *   0xffffffe1c0000000 - 0xffffffffffffffff (121GB, L2 slots [391..511])
> + *     Unmapped
> + */
> +
What is the benefit of moving the layout up to 0xffffffc000000000?

-- 
Bobby Eshleman
SE at Vates SAS


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 4/5] xen: Add files needed for minimal riscv build
  2021-05-14 21:53   ` Bob Eshleman
@ 2021-05-14 23:47     ` Connor Davis
  2021-05-18  1:43       ` Bob Eshleman
  0 siblings, 1 reply; 31+ messages in thread
From: Connor Davis @ 2021-05-14 23:47 UTC (permalink / raw)
  To: Bob Eshleman, xen-devel
  Cc: Alistair Francis, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu


On 5/14/21 3:53 PM, Bob Eshleman wrote:
> On 5/14/21 11:53 AM, Connor Davis wrote:
>> Add arch-specific makefiles and configs needed to build for
>> riscv64. Also add a minimal head.S that is a simple infinite loop.
>> head.o can be built with
>>
>> $ make XEN_TARGET_ARCH=riscv64 SUBSYSTEMS=xen -C xen TARGET=head.o
>>
> I recently realized that the Linux kernel build uses `ARCH=riscv`
> with 32 vs 64 being differentiated by Kconfig opts (and __riscv_xlen).
> I think `riscv64` was inherited by `arm64`.  This is something I'd
> like to eventually change the Xen build to (i.e.,
> `XEN_TARGET_ARCH=riscv make`) would it be possible for us to get that
> in this series?

Sure, I can do that

>
>> diff --git a/xen/include/asm-riscv/config.h b/xen/include/asm-riscv/config.h
>> new file mode 100644
>> index 0000000000..84cb436dc1
>> --- /dev/null
>> +++ b/xen/include/asm-riscv/config.h
>> @@ -0,0 +1,110 @@
>> +/******************************************************************************
>> + * config.h
>> + *
>> + * A Linux-style configuration list.
>> + */
>> +
>> +#ifndef __RISCV_CONFIG_H__
>> +#define __RISCV_CONFIG_H__
>> +
> ...
>
>> +
>> +#ifdef CONFIG_RISCV_64
>> +
>> +/*
>> + * RISC-V Layout:
>> + *   0x0000000000000000 - 0x0000003fffffffff (256GB, L2 slots [0-255])
>> + *     Unmapped
>> + *   0x0000004000000000 - 0xffffffbfffffffff
>> + *     Inaccessible: sv39 only supports 39-bit sign-extended VAs.
>> + *   0xffffffc000000000 - 0xffffffc0001fffff (2MB, L2 slot [256])
>> + *     Unmapped
>> + *   0xffffffc000200000 - 0xffffffc0003fffff (2MB, L2 slot [256])
>> + *     Xen text, data, bss
>> + *   0xffffffc000400000 - 0xffffffc0005fffff (2MB, L2 slot [256])
>> + *     Fixmap: special-purpose 4K mapping slots
>> + *   0xffffffc000600000 - 0xffffffc0009fffff (4MB, L2 slot [256])
>> + *     Early boot mapping of FDT
>> + *   0xffffffc000a00000 - 0xffffffc000bfffff (2MB, L2 slot [256])
>> + *     Early relocation address, used when relocating Xen and later
>> + *     for livepatch vmap (if compiled in)
>> + *   0xffffffc040000000 - 0xffffffc07fffffff (1GB, L2 slot [257])
>> + *     VMAP: ioremap and early_ioremap
>> + *   0xffffffc080000000 - 0xffffffc13fffffff (3GB, L2 slots [258..260])
>> + *     Unmapped
>> + *   0xffffffc140000000 - 0xffffffc1bfffffff (2GB, L2 slots [261..262])
>> + *     Frametable: 48 bytes per page for 133GB of RAM
>> + *   0xffffffc1c0000000 - 0xffffffe1bfffffff (128GB, L2 slots [263..390])
>> + *     1:1 direct mapping of RAM
>> + *   0xffffffe1c0000000 - 0xffffffffffffffff (121GB, L2 slots [391..511])
>> + *     Unmapped
>> + */
>> +
> What is the benefit of moving the layout up to 0xffffffc000000000?

I thought it made the most sense to use the upper half since Xen is 
privileged

and privileged code is typically mapped in the upper half, at least on 
x86. I'm happy to

move it down if that would be preferred.


Thanks,

Connor



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 5/5] automation: Add container for riscv64 builds
  2021-05-14 21:01   ` Bob Eshleman
@ 2021-05-14 23:54     ` Connor Davis
  0 siblings, 0 replies; 31+ messages in thread
From: Connor Davis @ 2021-05-14 23:54 UTC (permalink / raw)
  To: Bob Eshleman, xen-devel; +Cc: Alistair Francis, Doug Goldstein


On 5/14/21 3:01 PM, Bob Eshleman wrote:
> On 5/14/21 11:53 AM, Connor Davis wrote:
>> +
>> +# There is a regression in GDB that causes an assertion error
>> +# when setting breakpoints, use this commit until it is fixed!
>> +RUN git clone --recursive -j$(nproc) --progress https://github.com/riscv/riscv-gnu-toolchain && \
>> +    cd riscv-gnu-toolchain/riscv-gdb && \
>> +    git checkout 1dd588507782591478882a891f64945af9e2b86c && \
>> +    cd  .. && \
>> +    ./configure --prefix=/opt/riscv && \
>> +    make linux -j$(nproc) && \
>> +    rm -R /riscv-gnu-toolchai
> What do you think about using the RISCV tool chain from the Arch
> repos now?
That sounds much better, will update
>
> I've also discovered that the sym table error avoided by the commit
> pin can be worked around by removing already loaded symbols with
> `file` (no args) prior to calling `file path/to/file` to load new
> ones.  So if people did still want to use the container for
> development, they could still use the gdb installed by pacman
> (with the symbols caveat).
>
Thanks,

Connor



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 1/5] xen/char: Default HAS_NS16550 to y only for X86 and ARM
  2021-05-14 18:53 ` [PATCH v3 1/5] xen/char: Default HAS_NS16550 to y only for X86 and ARM Connor Davis
@ 2021-05-16 22:48   ` Alistair Francis
  2021-05-17 11:56   ` Jan Beulich
  1 sibling, 0 replies; 31+ messages in thread
From: Alistair Francis @ 2021-05-16 22:48 UTC (permalink / raw)
  To: Connor Davis
  Cc: open list:X86, Bobby Eshleman, Andrew Cooper, George Dunlap,
	Ian Jackson, Jan Beulich, Julien Grall, Stefano Stabellini,
	Wei Liu

On Sat, May 15, 2021 at 4:54 AM Connor Davis <connojdavis@gmail.com> wrote:
>
> Defaulting to yes only for X86 and ARM reduces the requirements
> for a minimal build when porting new architectures.
>
> Signed-off-by: Connor Davis <connojdavis@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  xen/drivers/char/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index b572305657..b15b0c8d6a 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -1,6 +1,6 @@
>  config HAS_NS16550
>         bool "NS16550 UART driver" if ARM
> -       default y
> +       default y if (ARM || X86)
>         help
>           This selects the 16550-series UART support. For most systems, say Y.
>
> --
> 2.31.1
>


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH
  2021-05-14 18:53 ` [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH Connor Davis
@ 2021-05-17 11:16   ` Jan Beulich
  2021-05-17 13:52     ` Connor Davis
  2021-05-17 15:42     ` Julien Grall
  0 siblings, 2 replies; 31+ messages in thread
From: Jan Beulich @ 2021-05-17 11:16 UTC (permalink / raw)
  To: Connor Davis, Julien Grall
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Stefano Stabellini, Wei Liu, Paul Durrant,
	xen-devel

On 14.05.2021 20:53, Connor Davis wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>      p2m_type_t p2mt;
>  #endif
>      mfn_t mfn;
> +#ifdef CONFIG_HAS_PASSTHROUGH
>      bool *dont_flush_p, dont_flush;
> +#endif
>      int rc;
>  
>  #ifdef CONFIG_X86
> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>       * Since we're likely to free the page below, we need to suspend
>       * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes.
>       */
> +#ifdef CONFIG_HAS_PASSTHROUGH
>      dont_flush_p = &this_cpu(iommu_dont_flush_iotlb);
>      dont_flush = *dont_flush_p;
>      *dont_flush_p = false;
> +#endif
>  
>      rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>  
> +#ifdef CONFIG_HAS_PASSTHROUGH
>      *dont_flush_p = dont_flush;
> +#endif
>  
>      /*
>       * With the lack of an IOMMU on some platforms, domains with DMA-capable
> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
>      xatp->gpfn += start;
>      xatp->size -= start;
>  
> +#ifdef CONFIG_HAS_PASSTHROUGH
>      if ( is_iommu_enabled(d) )
>      {
>         this_cpu(iommu_dont_flush_iotlb) = 1;
>         extra.ppage = &pages[0];
>      }
> +#endif
>  
>      while ( xatp->size > done )
>      {
> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
>          }
>      }
>  
> +#ifdef CONFIG_HAS_PASSTHROUGH
>      if ( is_iommu_enabled(d) )
>      {
>          int ret;
> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
>          if ( unlikely(ret) && rc >= 0 )
>              rc = ret;
>      }
> +#endif
>  
>      return rc;
>  }

I wonder whether all of these wouldn't better become CONFIG_X86:
ISTR Julien indicating that he doesn't see the override getting used
on Arm. (Julien, please correct me if I'm misremembering.)

> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -51,9 +51,15 @@ static inline bool_t dfn_eq(dfn_t x, dfn_t y)
>      return dfn_x(x) == dfn_x(y);
>  }
>  
> -extern bool_t iommu_enable, iommu_enabled;
> +extern bool_t iommu_enable;
>  extern bool force_iommu, iommu_quarantine, iommu_verbose;
>  
> +#ifdef CONFIG_HAS_PASSTHROUGH
> +extern bool_t iommu_enabled;

Just bool please, like is already the case for the line in context
above. We're in the process of phasing out bool_t.

Jan


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 3/5] xen: Fix build when !CONFIG_GRANT_TABLE
  2021-05-14 18:53 ` [PATCH v3 3/5] xen: Fix build when !CONFIG_GRANT_TABLE Connor Davis
@ 2021-05-17 11:22   ` Jan Beulich
  2021-05-17 23:46     ` Connor Davis
  2021-05-18  3:58     ` Connor Davis
  0 siblings, 2 replies; 31+ messages in thread
From: Jan Beulich @ 2021-05-17 11:22 UTC (permalink / raw)
  To: Connor Davis
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
	xen-devel

On 14.05.2021 20:53, Connor Davis wrote:
> Move struct grant_table; in grant_table.h above
> ifdef CONFIG_GRANT_TABLE. This fixes the following:
> 
> /build/xen/include/xen/grant_table.h:84:50: error: 'struct grant_table'
> declared inside parameter list will not be visible outside of this
> definition or declaration [-Werror]
>    84 | static inline int mem_sharing_gref_to_gfn(struct grant_table *gt,
>       |

There must be more to this, as e.g. the PV shim does get built with
!GRANT_TABLE. Nevertheless, ...

> Signed-off-by: Connor Davis <connojdavis@gmail.com>

... since the potential of breaking the build is obvious enough,
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 4/5] xen: Add files needed for minimal riscv build
  2021-05-14 18:53 ` [PATCH v3 4/5] xen: Add files needed for minimal riscv build Connor Davis
  2021-05-14 21:53   ` Bob Eshleman
@ 2021-05-17 11:51   ` Jan Beulich
  2021-05-18  4:58     ` Connor Davis
  1 sibling, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2021-05-17 11:51 UTC (permalink / raw)
  To: Connor Davis
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
	xen-devel

On 14.05.2021 20:53, Connor Davis wrote:
> --- /dev/null
> +++ b/config/riscv64.mk
> @@ -0,0 +1,5 @@
> +CONFIG_RISCV := y
> +CONFIG_RISCV_64 := y
> +CONFIG_RISCV_$(XEN_OS) := y

I wonder whether the last one actually gets used anywhere, but I do
realize that other architectures have similar definitions.

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -26,7 +26,9 @@ MAKEFLAGS += -rR
>  EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
>  
>  ARCH=$(XEN_TARGET_ARCH)
> -SRCARCH=$(shell echo $(ARCH) | sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g')
> +SRCARCH=$(shell echo $(ARCH) | \
> +	  sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
> +	      -e s'/riscv.*/riscv/g')

I think in makefiles tab indentation would better be restricted to
rules. While here it's a minor nit, ...

> @@ -35,7 +37,8 @@ include $(XEN_ROOT)/Config.mk
>  # Set ARCH/SUBARCH appropriately.
>  export TARGET_SUBARCH  := $(XEN_TARGET_ARCH)
>  export TARGET_ARCH     := $(shell echo $(XEN_TARGET_ARCH) | \
> -                            sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g')
> +                            sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
> +			        -e s'/riscv.*/riscv/g')

... here you're actually introducing locally inconsistent indentation.

> --- /dev/null
> +++ b/xen/arch/riscv/Kconfig
> @@ -0,0 +1,52 @@
> +config 64BIT
> +	bool

I'm afraid this is stale now - the option now lives in xen/arch/Kconfig,
available to all architectures.

> +config RISCV_64
> +	bool
> +	depends on 64BIT

Such a "depends on" is relatively pointless - they're more important to
have when there is a prompt.

> +config ARCH_DEFCONFIG
> +	string
> +	default "arch/riscv/configs/riscv64_defconfig" if RISCV_64

For the RISCV_64 dependency to be really useful (at least with the
command line kconfig), you want its selection to live above the use.

> +menu "Architecture Features"
> +
> +source "arch/Kconfig"
> +
> +endmenu
> +
> +menu "ISA Selection"
> +
> +choice
> +	prompt "Base ISA"
> +        default RISCV_ISA_RV64IMA
> +        help
> +          This selects the base ISA extensions that Xen will target.
> +
> +config RISCV_ISA_RV64IMA
> +	bool "RV64IMA"
> +        select 64BIT
> +        select RISCV_64

I think you want only the latter here, and the former done by RISCV_64
(or select the former here, and have "default y if 64BIT" at RISCV_64).
That way, not every party wanting 64-bit has to select both.

> +        help
> +           Use the RV64I base ISA, plus the "M" and "A" extensions
> +           for integer multiply/divide and atomic instructions, respectively.
> +
> +endchoice
> +
> +config RISCV_ISA_C
> +	bool "Compressed extension"
> +        help
> +           Add "C" to the ISA subsets that the toolchain is allowed
> +           to emit when building Xen, which results in compressed
> +           instructions in the Xen binary.
> +
> +           If unsure, say N.

For all of the above, please adjust indentation to be consistent with
(the bulk of) what we have elsewhere.

> --- /dev/null
> +++ b/xen/arch/riscv/arch.mk
> @@ -0,0 +1,16 @@
> +########################################
> +# RISCV-specific definitions
> +
> +ifeq ($(CONFIG_RISCV_64),y)
> +    CFLAGS += -mabi=lp64
> +endif

Wherever possible I think we should prefer the list approach:

CFLAGS-$(CONFIG_RISCV_64) += -mabi=lp64

> --- /dev/null
> +++ b/xen/arch/riscv/configs/riscv64_defconfig
> @@ -0,0 +1,12 @@
> +# CONFIG_SCHED_CREDIT is not set
> +# CONFIG_SCHED_RTDS is not set
> +# CONFIG_SCHED_NULL is not set
> +# CONFIG_SCHED_ARINC653 is not set
> +# CONFIG_TRACEBUFFER is not set
> +# CONFIG_DEBUG is not set
> +# CONFIG_DEBUG_INFO is not set
> +# CONFIG_HYPFS is not set
> +# CONFIG_GRANT_TABLE is not set
> +# CONFIG_SPECULATIVE_HARDEN_ARRAY is not set
> +
> +CONFIG_EXPERT=y

These are rather odd defaults, more like for a special purpose
config than a general purpose one. None of what you turn off here
will guarantee to be off for people actually trying to build
things, so it's not clear to me what the idea here is. As a
specific remark, especially during bringup work I think it is
quite important to not default DEBUG to off: You definitely want
to see whether any assertions trigger.

> --- /dev/null
> +++ b/xen/include/asm-riscv/config.h
> @@ -0,0 +1,110 @@
> +/******************************************************************************
> + * config.h
> + *
> + * A Linux-style configuration list.

May I suggest to not further replicate this now inapplicable
comment? It was already somewhat bogus for Arm to clone from x86.

> + */
> +
> +#ifndef __RISCV_CONFIG_H__
> +#define __RISCV_CONFIG_H__
> +
> +#if defined(CONFIG_RISCV_64)
> +# define LONG_BYTEORDER 3
> +# define ELFSIZE 64
> +#else
> +# error "Unsupported RISCV variant"
> +#endif
> +
> +#define BYTES_PER_LONG (1 << LONG_BYTEORDER)
> +#define BITS_PER_LONG  (BYTES_PER_LONG << 3)
> +#define POINTER_ALIGN  BYTES_PER_LONG
> +
> +#define BITS_PER_LLONG 64
> +
> +/* xen_ulong_t is always 64 bits */
> +#define BITS_PER_XEN_ULONG 64
> +
> +#define CONFIG_RISCV 1

This duplicates a "real" Kconfig setting, doesn't it?

> +#define CONFIG_RISCV_L1_CACHE_SHIFT 6
> +
> +#define CONFIG_PAGEALLOC_MAX_ORDER 18
> +#define CONFIG_DOMU_MAX_ORDER      9
> +#define CONFIG_HWDOM_MAX_ORDER     10
> +
> +#define OPT_CONSOLE_STR "dtuart"
> +
> +#ifdef CONFIG_RISCV_64
> +#define MAX_VIRT_CPUS 128u
> +#else
> +#error "Unsupported RISCV variant"
> +#endif

Could this be folded with the other similar construct further up?

Jan


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 1/5] xen/char: Default HAS_NS16550 to y only for X86 and ARM
  2021-05-14 18:53 ` [PATCH v3 1/5] xen/char: Default HAS_NS16550 to y only for X86 and ARM Connor Davis
  2021-05-16 22:48   ` Alistair Francis
@ 2021-05-17 11:56   ` Jan Beulich
  2021-05-17 23:43     ` Connor Davis
  1 sibling, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2021-05-17 11:56 UTC (permalink / raw)
  To: Connor Davis
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
	xen-devel

On 14.05.2021 20:53, Connor Davis wrote:
> Defaulting to yes only for X86 and ARM reduces the requirements
> for a minimal build when porting new architectures.

While I agree with the statement, ...

> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -1,6 +1,6 @@
>  config HAS_NS16550
>  	bool "NS16550 UART driver" if ARM
> -	default y
> +	default y if (ARM || X86)

... this approach doesn't scale very well. You would likely have
been hesitant to add a, say, 12-way || here if we had this many
architectures already. I think you instead want

 config HAS_NS16550
 	bool "NS16550 UART driver" if ARM
+	default n if RISCV
 	default y

which then can be adjusted back by another one line change once
the driver code actually builds.

Jan


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH
  2021-05-17 11:16   ` Jan Beulich
@ 2021-05-17 13:52     ` Connor Davis
  2021-05-17 15:42     ` Julien Grall
  1 sibling, 0 replies; 31+ messages in thread
From: Connor Davis @ 2021-05-17 13:52 UTC (permalink / raw)
  To: Jan Beulich, Julien Grall
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Stefano Stabellini, Wei Liu, Paul Durrant,
	xen-devel


On 5/17/21 5:16 AM, Jan Beulich wrote:
> On 14.05.2021 20:53, Connor Davis wrote:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>>       p2m_type_t p2mt;
>>   #endif
>>       mfn_t mfn;
>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>       bool *dont_flush_p, dont_flush;
>> +#endif
>>       int rc;
>>   
>>   #ifdef CONFIG_X86
>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>>        * Since we're likely to free the page below, we need to suspend
>>        * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes.
>>        */
>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>       dont_flush_p = &this_cpu(iommu_dont_flush_iotlb);
>>       dont_flush = *dont_flush_p;
>>       *dont_flush_p = false;
>> +#endif
>>   
>>       rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>>   
>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>       *dont_flush_p = dont_flush;
>> +#endif
>>   
>>       /*
>>        * With the lack of an IOMMU on some platforms, domains with DMA-capable
>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
>>       xatp->gpfn += start;
>>       xatp->size -= start;
>>   
>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>       if ( is_iommu_enabled(d) )
>>       {
>>          this_cpu(iommu_dont_flush_iotlb) = 1;
>>          extra.ppage = &pages[0];
>>       }
>> +#endif
>>   
>>       while ( xatp->size > done )
>>       {
>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
>>           }
>>       }
>>   
>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>       if ( is_iommu_enabled(d) )
>>       {
>>           int ret;
>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
>>           if ( unlikely(ret) && rc >= 0 )
>>               rc = ret;
>>       }
>> +#endif
>>   
>>       return rc;
>>   }
> I wonder whether all of these wouldn't better become CONFIG_X86:
> ISTR Julien indicating that he doesn't see the override getting used
> on Arm. (Julien, please correct me if I'm misremembering.)
>
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -51,9 +51,15 @@ static inline bool_t dfn_eq(dfn_t x, dfn_t y)
>>       return dfn_x(x) == dfn_x(y);
>>   }
>>   
>> -extern bool_t iommu_enable, iommu_enabled;
>> +extern bool_t iommu_enable;
>>   extern bool force_iommu, iommu_quarantine, iommu_verbose;
>>   
>> +#ifdef CONFIG_HAS_PASSTHROUGH
>> +extern bool_t iommu_enabled;
> Just bool please, like is already the case for the line in context
> above. We're in the process of phasing out bool_t.
Got it, thanks.
>
> Jan


Thanks,

Connor



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH
  2021-05-17 11:16   ` Jan Beulich
  2021-05-17 13:52     ` Connor Davis
@ 2021-05-17 15:42     ` Julien Grall
  2021-05-18  4:11       ` Connor Davis
  1 sibling, 1 reply; 31+ messages in thread
From: Julien Grall @ 2021-05-17 15:42 UTC (permalink / raw)
  To: Jan Beulich, Connor Davis
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Stefano Stabellini, Wei Liu, Paul Durrant,
	xen-devel

Hi Jan,

On 17/05/2021 12:16, Jan Beulich wrote:
> On 14.05.2021 20:53, Connor Davis wrote:
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>>       p2m_type_t p2mt;
>>   #endif
>>       mfn_t mfn;
>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>       bool *dont_flush_p, dont_flush;
>> +#endif
>>       int rc;
>>   
>>   #ifdef CONFIG_X86
>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>>        * Since we're likely to free the page below, we need to suspend
>>        * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes.
>>        */
>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>       dont_flush_p = &this_cpu(iommu_dont_flush_iotlb);
>>       dont_flush = *dont_flush_p;
>>       *dont_flush_p = false;
>> +#endif
>>   
>>       rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>>   
>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>       *dont_flush_p = dont_flush;
>> +#endif
>>   
>>       /*
>>        * With the lack of an IOMMU on some platforms, domains with DMA-capable
>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
>>       xatp->gpfn += start;
>>       xatp->size -= start;
>>   
>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>       if ( is_iommu_enabled(d) )
>>       {
>>          this_cpu(iommu_dont_flush_iotlb) = 1;
>>          extra.ppage = &pages[0];
>>       }
>> +#endif
>>   
>>       while ( xatp->size > done )
>>       {
>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
>>           }
>>       }
>>   
>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>       if ( is_iommu_enabled(d) )
>>       {
>>           int ret;
>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,
>>           if ( unlikely(ret) && rc >= 0 )
>>               rc = ret;
>>       }
>> +#endif
>>   
>>       return rc;
>>   }
> 
> I wonder whether all of these wouldn't better become CONFIG_X86:
> ISTR Julien indicating that he doesn't see the override getting used
> on Arm. (Julien, please correct me if I'm misremembering.)

Right, so far, I haven't been in favor to introduce it because:
    1) The P2M code may free some memory. So you can't always ignore the 
flush (I think this is wrong for the upper layer to know when this can 
happen).
    2) It is unclear what happen if the IOMMU TLBs and the PT contains 
different mappings (I received conflicted advice).

So it is better to always flush and as early as possible.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 0/5] Minimal build for RISCV
  2021-05-14 18:53 [PATCH v3 0/5] Minimal build for RISCV Connor Davis
                   ` (4 preceding siblings ...)
  2021-05-14 18:53 ` [PATCH v3 5/5] automation: Add container for riscv64 builds Connor Davis
@ 2021-05-17 23:20 ` Roman Shaposhnik
  5 siblings, 0 replies; 31+ messages in thread
From: Roman Shaposhnik @ 2021-05-17 23:20 UTC (permalink / raw)
  To: Connor Davis
  Cc: Xen-devel, Bobby Eshleman, Alistair Francis, Andrew Cooper,
	George Dunlap, Ian Jackson, Jan Beulich, Julien Grall,
	Stefano Stabellini, Wei Liu, Paul Durrant, Doug Goldstein

FWIW: for project EVE, the timing on this is perfect -- I've just
started a complete RISC-V port:
    https://github.com/lf-edge/eve/pull/2083

Targeting KVM for now, but would be awesome to see at least some
rudimentary RISC-V support land in Xen.

Connor, I'll be merging your changes into my patchset on EVE side ASAP
-- just to start playing with it.

Thanks,
Roman.

On Fri, May 14, 2021 at 11:54 AM Connor Davis <connojdavis@gmail.com> wrote:
>
> Hi all,
>
> This series introduces a minimal build for RISCV. It is based on Bobby's
> previous work from last year[0] rebased onto current Xen.
>
> This series provides the patches necessary to get a minimal build
> working. The build is "minimal" in the sense that it only supports
> building TARGET=head.o. The arch/riscv/head.S is just a simple while(1).
>
> The first 3 patches are mods to non-RISCV bits that enable building a
> config with:
>
>   !CONFIG_HAS_NS16550
>   !CONFIG_HAS_PASSTHROUGH
>   !CONFIG_GRANT_TABLE
>
> respectively. The fourth patch adds the make/Kconfig boilerplate
> alongside head.S and asm-riscv/config.h (head.S references ENTRY
> that is defined in asm-riscv/config.h).
>
> The last adds a docker container for doing the build. To build from the
> docker container (after creating it locally), you can run the following:
>
>   $ make XEN_TARGET_ARCH=riscv64 SUBSYSTEMS=xen -C xen TARGET=head.o
>
> --
> Changes since v2:
>   - Reduced number of riscv files added to ease review
>
> Changes since v1:
>   - Dropped "xen/sched: Fix build when NR_CPUS == 1" since this was
>     fixed for 4.15
>   - Moved #ifdef-ary around iommu_enabled to iommu.h
>   - Moved struct grant_table declaration above ifdef CONFIG_GRANT_TABLE
>     instead of defining an empty struct when !CONFIG_GRANT_TABLE
>
> Connor Davis (5):
>   xen/char: Default HAS_NS16550 to y only for X86 and ARM
>   xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH
>   xen: Fix build when !CONFIG_GRANT_TABLE
>   xen: Add files needed for minimal riscv build
>   automation: Add container for riscv64 builds
>
>  automation/build/archlinux/riscv64.dockerfile |  33 ++++++
>  automation/scripts/containerize               |   1 +
>  config/riscv64.mk                             |   5 +
>  xen/Makefile                                  |   8 +-
>  xen/arch/riscv/Kconfig                        |  52 +++++++++
>  xen/arch/riscv/Kconfig.debug                  |   0
>  xen/arch/riscv/Makefile                       |   0
>  xen/arch/riscv/Rules.mk                       |   0
>  xen/arch/riscv/arch.mk                        |  16 +++
>  xen/arch/riscv/asm-offsets.c                  |   0
>  xen/arch/riscv/configs/riscv64_defconfig      |  12 ++
>  xen/arch/riscv/head.S                         |   6 +
>  xen/common/memory.c                           |  10 ++
>  xen/drivers/char/Kconfig                      |   2 +-
>  xen/include/asm-riscv/config.h                | 110 ++++++++++++++++++
>  xen/include/xen/grant_table.h                 |   3 +-
>  xen/include/xen/iommu.h                       |   8 +-
>  17 files changed, 261 insertions(+), 5 deletions(-)
>  create mode 100644 automation/build/archlinux/riscv64.dockerfile
>  create mode 100644 config/riscv64.mk
>  create mode 100644 xen/arch/riscv/Kconfig
>  create mode 100644 xen/arch/riscv/Kconfig.debug
>  create mode 100644 xen/arch/riscv/Makefile
>  create mode 100644 xen/arch/riscv/Rules.mk
>  create mode 100644 xen/arch/riscv/arch.mk
>  create mode 100644 xen/arch/riscv/asm-offsets.c
>  create mode 100644 xen/arch/riscv/configs/riscv64_defconfig
>  create mode 100644 xen/arch/riscv/head.S
>  create mode 100644 xen/include/asm-riscv/config.h
>
> --
> 2.31.1
>
>


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 1/5] xen/char: Default HAS_NS16550 to y only for X86 and ARM
  2021-05-17 11:56   ` Jan Beulich
@ 2021-05-17 23:43     ` Connor Davis
  0 siblings, 0 replies; 31+ messages in thread
From: Connor Davis @ 2021-05-17 23:43 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
	xen-devel


On 5/17/21 5:56 AM, Jan Beulich wrote:
>> --- a/xen/drivers/char/Kconfig
>> +++ b/xen/drivers/char/Kconfig
>> @@ -1,6 +1,6 @@
>>   config HAS_NS16550
>>   	bool "NS16550 UART driver" if ARM
>> -	default y
>> +	default y if (ARM || X86)
> ... this approach doesn't scale very well. You would likely have
> been hesitant to add a, say, 12-way || here if we had this many
> architectures already. I think you instead want
>
>   config HAS_NS16550
>   	bool "NS16550 UART driver" if ARM
> +	default n if RISCV
>   	default y
>
> which then can be adjusted back by another one line change once
> the driver code actually builds.
>
> Jan

Agreed, I will update this in the next version.


Thanks,

Connor



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 3/5] xen: Fix build when !CONFIG_GRANT_TABLE
  2021-05-17 11:22   ` Jan Beulich
@ 2021-05-17 23:46     ` Connor Davis
  2021-05-18  3:58     ` Connor Davis
  1 sibling, 0 replies; 31+ messages in thread
From: Connor Davis @ 2021-05-17 23:46 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
	xen-devel


On 5/17/21 5:22 AM, Jan Beulich wrote:
> On 14.05.2021 20:53, Connor Davis wrote:
>> Move struct grant_table; in grant_table.h above
>> ifdef CONFIG_GRANT_TABLE. This fixes the following:
>>
>> /build/xen/include/xen/grant_table.h:84:50: error: 'struct grant_table'
>> declared inside parameter list will not be visible outside of this
>> definition or declaration [-Werror]
>>     84 | static inline int mem_sharing_gref_to_gfn(struct grant_table *gt,
>>        |
> There must be more to this, as e.g. the PV shim does get built with
> !GRANT_TABLE. Nevertheless, ...
>
You are right... you're comment made me realize I had only tested this

with XEN_TARGET_ARCH=riscv64. I will rework this.

>> Signed-off-by: Connor Davis <connojdavis@gmail.com>
> ... since the potential of breaking the build is obvious enough,
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> Jan


Thanks,

Connor



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 4/5] xen: Add files needed for minimal riscv build
  2021-05-14 23:47     ` Connor Davis
@ 2021-05-18  1:43       ` Bob Eshleman
  2021-05-18  4:05         ` Connor Davis
  0 siblings, 1 reply; 31+ messages in thread
From: Bob Eshleman @ 2021-05-18  1:43 UTC (permalink / raw)
  To: Connor Davis, xen-devel
  Cc: Alistair Francis, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu

On 5/14/21 4:47 PM, Connor Davis wrote:
> 
> On 5/14/21 3:53 PM, Bob Eshleman wrote:
>> On 5/14/21 11:53 AM, Connor Davis wrote:
>>
>>> +
>>> +#ifdef CONFIG_RISCV_64
>>> +
>>> +/*
>>> + * RISC-V Layout:
>>> + *   0x0000000000000000 - 0x0000003fffffffff (256GB, L2 slots [0-255])
>>> + *     Unmapped
>>> + *   0x0000004000000000 - 0xffffffbfffffffff
>>> + *     Inaccessible: sv39 only supports 39-bit sign-extended VAs.
>>> + *   0xffffffc000000000 - 0xffffffc0001fffff (2MB, L2 slot [256])
>>> + *     Unmapped
>>> + *   0xffffffc000200000 - 0xffffffc0003fffff (2MB, L2 slot [256])
>>> + *     Xen text, data, bss
>>> + *   0xffffffc000400000 - 0xffffffc0005fffff (2MB, L2 slot [256])
>>> + *     Fixmap: special-purpose 4K mapping slots
>>> + *   0xffffffc000600000 - 0xffffffc0009fffff (4MB, L2 slot [256])
>>> + *     Early boot mapping of FDT
>>> + *   0xffffffc000a00000 - 0xffffffc000bfffff (2MB, L2 slot [256])
>>> + *     Early relocation address, used when relocating Xen and later
>>> + *     for livepatch vmap (if compiled in)
>>> + *   0xffffffc040000000 - 0xffffffc07fffffff (1GB, L2 slot [257])
>>> + *     VMAP: ioremap and early_ioremap
>>> + *   0xffffffc080000000 - 0xffffffc13fffffff (3GB, L2 slots [258..260])
>>> + *     Unmapped
>>> + *   0xffffffc140000000 - 0xffffffc1bfffffff (2GB, L2 slots [261..262])
>>> + *     Frametable: 48 bytes per page for 133GB of RAM
>>> + *   0xffffffc1c0000000 - 0xffffffe1bfffffff (128GB, L2 slots [263..390])
>>> + *     1:1 direct mapping of RAM
>>> + *   0xffffffe1c0000000 - 0xffffffffffffffff (121GB, L2 slots [391..511])
>>> + *     Unmapped
>>> + */
>>> +
>> What is the benefit of moving the layout up to 0xffffffc000000000?
> 
> I thought it made the most sense to use the upper half since Xen is privileged
> 
> and privileged code is typically mapped in the upper half, at least on x86. I'm happy to
> 
> move it down if that would be preferred.
> 
> 

I do like the idea of following norms, but I think I prefer following the ARM norm
over the x86 norm unless there is a technical reason not to. Just due to
ARM and RISC-V having much more overlap than x86 and RISC-V.  In this case,
all things being equal, I'd prefer following the ARM model and use the lower half.
I definitely like adding the note on the inaccessible region.

Thanks,

-- 
Bobby Eshleman
SE at Vates SAS


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 3/5] xen: Fix build when !CONFIG_GRANT_TABLE
  2021-05-17 11:22   ` Jan Beulich
  2021-05-17 23:46     ` Connor Davis
@ 2021-05-18  3:58     ` Connor Davis
  2021-05-18  6:31       ` Jan Beulich
  1 sibling, 1 reply; 31+ messages in thread
From: Connor Davis @ 2021-05-18  3:58 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
	xen-devel


On 5/17/21 5:22 AM, Jan Beulich wrote:
> On 14.05.2021 20:53, Connor Davis wrote:
>> Move struct grant_table; in grant_table.h above
>> ifdef CONFIG_GRANT_TABLE. This fixes the following:
>>
>> /build/xen/include/xen/grant_table.h:84:50: error: 'struct grant_table'
>> declared inside parameter list will not be visible outside of this
>> definition or declaration [-Werror]
>>     84 | static inline int mem_sharing_gref_to_gfn(struct grant_table *gt,
>>        |
> There must be more to this, as e.g. the PV shim does get built with
> !GRANT_TABLE. Nevertheless, ...
>
Can you elaborate? I tested all defconfigs with and without grant tables

enabled on x86 and ARM and they all build fine.

>> Signed-off-by: Connor Davis <connojdavis@gmail.com>
> ... since the potential of breaking the build is obvious enough,
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> Jan


Thanks,

Connor



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 4/5] xen: Add files needed for minimal riscv build
  2021-05-18  1:43       ` Bob Eshleman
@ 2021-05-18  4:05         ` Connor Davis
  0 siblings, 0 replies; 31+ messages in thread
From: Connor Davis @ 2021-05-18  4:05 UTC (permalink / raw)
  To: Bob Eshleman, xen-devel
  Cc: Alistair Francis, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu


On 5/17/21 7:43 PM, Bob Eshleman wrote:
> On 5/14/21 4:47 PM, Connor Davis wrote:
>> On 5/14/21 3:53 PM, Bob Eshleman wrote:
>>> On 5/14/21 11:53 AM, Connor Davis wrote:
>>>
>>>> +
>>>> +#ifdef CONFIG_RISCV_64
>>>> +
>>>> +/*
>>>> + * RISC-V Layout:
>>>> + *   0x0000000000000000 - 0x0000003fffffffff (256GB, L2 slots [0-255])
>>>> + *     Unmapped
>>>> + *   0x0000004000000000 - 0xffffffbfffffffff
>>>> + *     Inaccessible: sv39 only supports 39-bit sign-extended VAs.
>>>> + *   0xffffffc000000000 - 0xffffffc0001fffff (2MB, L2 slot [256])
>>>> + *     Unmapped
>>>> + *   0xffffffc000200000 - 0xffffffc0003fffff (2MB, L2 slot [256])
>>>> + *     Xen text, data, bss
>>>> + *   0xffffffc000400000 - 0xffffffc0005fffff (2MB, L2 slot [256])
>>>> + *     Fixmap: special-purpose 4K mapping slots
>>>> + *   0xffffffc000600000 - 0xffffffc0009fffff (4MB, L2 slot [256])
>>>> + *     Early boot mapping of FDT
>>>> + *   0xffffffc000a00000 - 0xffffffc000bfffff (2MB, L2 slot [256])
>>>> + *     Early relocation address, used when relocating Xen and later
>>>> + *     for livepatch vmap (if compiled in)
>>>> + *   0xffffffc040000000 - 0xffffffc07fffffff (1GB, L2 slot [257])
>>>> + *     VMAP: ioremap and early_ioremap
>>>> + *   0xffffffc080000000 - 0xffffffc13fffffff (3GB, L2 slots [258..260])
>>>> + *     Unmapped
>>>> + *   0xffffffc140000000 - 0xffffffc1bfffffff (2GB, L2 slots [261..262])
>>>> + *     Frametable: 48 bytes per page for 133GB of RAM
>>>> + *   0xffffffc1c0000000 - 0xffffffe1bfffffff (128GB, L2 slots [263..390])
>>>> + *     1:1 direct mapping of RAM
>>>> + *   0xffffffe1c0000000 - 0xffffffffffffffff (121GB, L2 slots [391..511])
>>>> + *     Unmapped
>>>> + */
>>>> +
>>> What is the benefit of moving the layout up to 0xffffffc000000000?
>> I thought it made the most sense to use the upper half since Xen is privileged
>>
>> and privileged code is typically mapped in the upper half, at least on x86. I'm happy to
>>
>> move it down if that would be preferred.
>>
>>
> I do like the idea of following norms, but I think I prefer following the ARM norm
> over the x86 norm unless there is a technical reason not to. Just due to
> ARM and RISC-V having much more overlap than x86 and RISC-V.  In this case,
> all things being equal, I'd prefer following the ARM model and use the lower half.
> I definitely like adding the note on the inaccessible region.

Sounds good, I will move it down.


Thanks,

Connor



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH
  2021-05-17 15:42     ` Julien Grall
@ 2021-05-18  4:11       ` Connor Davis
  2021-05-18  6:27         ` Jan Beulich
  0 siblings, 1 reply; 31+ messages in thread
From: Connor Davis @ 2021-05-18  4:11 UTC (permalink / raw)
  To: Julien Grall, Jan Beulich
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Stefano Stabellini, Wei Liu, Paul Durrant,
	xen-devel


On 5/17/21 9:42 AM, Julien Grall wrote:
> Hi Jan,
>
> On 17/05/2021 12:16, Jan Beulich wrote:
>> On 14.05.2021 20:53, Connor Davis wrote:
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned 
>>> long gmfn)
>>>       p2m_type_t p2mt;
>>>   #endif
>>>       mfn_t mfn;
>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>       bool *dont_flush_p, dont_flush;
>>> +#endif
>>>       int rc;
>>>     #ifdef CONFIG_X86
>>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, 
>>> unsigned long gmfn)
>>>        * Since we're likely to free the page below, we need to suspend
>>>        * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes.
>>>        */
>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>       dont_flush_p = &this_cpu(iommu_dont_flush_iotlb);
>>>       dont_flush = *dont_flush_p;
>>>       *dont_flush_p = false;
>>> +#endif
>>>         rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>>>   +#ifdef CONFIG_HAS_PASSTHROUGH
>>>       *dont_flush_p = dont_flush;
>>> +#endif
>>>         /*
>>>        * With the lack of an IOMMU on some platforms, domains with 
>>> DMA-capable
>>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, 
>>> struct xen_add_to_physmap *xatp,
>>>       xatp->gpfn += start;
>>>       xatp->size -= start;
>>>   +#ifdef CONFIG_HAS_PASSTHROUGH
>>>       if ( is_iommu_enabled(d) )
>>>       {
>>>          this_cpu(iommu_dont_flush_iotlb) = 1;
>>>          extra.ppage = &pages[0];
>>>       }
>>> +#endif
>>>         while ( xatp->size > done )
>>>       {
>>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, 
>>> struct xen_add_to_physmap *xatp,
>>>           }
>>>       }
>>>   +#ifdef CONFIG_HAS_PASSTHROUGH
>>>       if ( is_iommu_enabled(d) )
>>>       {
>>>           int ret;
>>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, 
>>> struct xen_add_to_physmap *xatp,
>>>           if ( unlikely(ret) && rc >= 0 )
>>>               rc = ret;
>>>       }
>>> +#endif
>>>         return rc;
>>>   }
>>
>> I wonder whether all of these wouldn't better become CONFIG_X86:
>> ISTR Julien indicating that he doesn't see the override getting used
>> on Arm. (Julien, please correct me if I'm misremembering.)
>
> Right, so far, I haven't been in favor to introduce it because:
>    1) The P2M code may free some memory. So you can't always ignore 
> the flush (I think this is wrong for the upper layer to know when this 
> can happen).
>    2) It is unclear what happen if the IOMMU TLBs and the PT contains 
> different mappings (I received conflicted advice).
>
> So it is better to always flush and as early as possible.

So keep it as is or switch to CONFIG_X86?


Thanks,

Connor



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 4/5] xen: Add files needed for minimal riscv build
  2021-05-17 11:51   ` Jan Beulich
@ 2021-05-18  4:58     ` Connor Davis
  2021-05-18  6:33       ` Jan Beulich
  0 siblings, 1 reply; 31+ messages in thread
From: Connor Davis @ 2021-05-18  4:58 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
	xen-devel



On 5/17/21 5:51 AM, Jan Beulich wrote:
> On 14.05.2021 20:53, Connor Davis wrote:
>> --- /dev/null
>> +++ b/config/riscv64.mk
>> @@ -0,0 +1,5 @@
>> +CONFIG_RISCV := y
>> +CONFIG_RISCV_64 := y
>> +CONFIG_RISCV_$(XEN_OS) := y
> I wonder whether the last one actually gets used anywhere, but I do
> realize that other architectures have similar definitions.
>
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -26,7 +26,9 @@ MAKEFLAGS += -rR
>>   EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
>>   
>>   ARCH=$(XEN_TARGET_ARCH)
>> -SRCARCH=$(shell echo $(ARCH) | sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g')
>> +SRCARCH=$(shell echo $(ARCH) | \
>> +	  sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
>> +	      -e s'/riscv.*/riscv/g')
> I think in makefiles tab indentation would better be restricted to
> rules. While here it's a minor nit, ...
>
>> @@ -35,7 +37,8 @@ include $(XEN_ROOT)/Config.mk
>>   # Set ARCH/SUBARCH appropriately.
>>   export TARGET_SUBARCH  := $(XEN_TARGET_ARCH)
>>   export TARGET_ARCH     := $(shell echo $(XEN_TARGET_ARCH) | \
>> -                            sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g')
>> +                            sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
>> +			        -e s'/riscv.*/riscv/g')
> ... here you're actually introducing locally inconsistent indentation.
>
>> --- /dev/null
>> +++ b/xen/arch/riscv/Kconfig
>> @@ -0,0 +1,52 @@
>> +config 64BIT
>> +	bool
> I'm afraid this is stale now - the option now lives in xen/arch/Kconfig,
> available to all architectures.
>
>> +config RISCV_64
>> +	bool
>> +	depends on 64BIT
> Such a "depends on" is relatively pointless - they're more important to
> have when there is a prompt.
>
>> +config ARCH_DEFCONFIG
>> +	string
>> +	default "arch/riscv/configs/riscv64_defconfig" if RISCV_64
> For the RISCV_64 dependency to be really useful (at least with the
> command line kconfig), you want its selection to live above the use.
>
>> +menu "Architecture Features"
>> +
>> +source "arch/Kconfig"
>> +
>> +endmenu
>> +
>> +menu "ISA Selection"
>> +
>> +choice
>> +	prompt "Base ISA"
>> +        default RISCV_ISA_RV64IMA
>> +        help
>> +          This selects the base ISA extensions that Xen will target.
>> +
>> +config RISCV_ISA_RV64IMA
>> +	bool "RV64IMA"
>> +        select 64BIT
>> +        select RISCV_64
> I think you want only the latter here, and the former done by RISCV_64
> (or select the former here, and have "default y if 64BIT" at RISCV_64).
> That way, not every party wanting 64-bit has to select both.
>
>> +        help
>> +           Use the RV64I base ISA, plus the "M" and "A" extensions
>> +           for integer multiply/divide and atomic instructions, respectively.
>> +
>> +endchoice
>> +
>> +config RISCV_ISA_C
>> +	bool "Compressed extension"
>> +        help
>> +           Add "C" to the ISA subsets that the toolchain is allowed
>> +           to emit when building Xen, which results in compressed
>> +           instructions in the Xen binary.
>> +
>> +           If unsure, say N.
> For all of the above, please adjust indentation to be consistent with
> (the bulk of) what we have elsewhere.
Will do
>> --- /dev/null
>> +++ b/xen/arch/riscv/arch.mk
>> @@ -0,0 +1,16 @@
>> +########################################
>> +# RISCV-specific definitions
>> +
>> +ifeq ($(CONFIG_RISCV_64),y)
>> +    CFLAGS += -mabi=lp64
>> +endif
> Wherever possible I think we should prefer the list approach:
>
> CFLAGS-$(CONFIG_RISCV_64) += -mabi=lp64
>
>> --- /dev/null
>> +++ b/xen/arch/riscv/configs/riscv64_defconfig
>> @@ -0,0 +1,12 @@
>> +# CONFIG_SCHED_CREDIT is not set
>> +# CONFIG_SCHED_RTDS is not set
>> +# CONFIG_SCHED_NULL is not set
>> +# CONFIG_SCHED_ARINC653 is not set
>> +# CONFIG_TRACEBUFFER is not set
>> +# CONFIG_DEBUG is not set
>> +# CONFIG_DEBUG_INFO is not set
>> +# CONFIG_HYPFS is not set
>> +# CONFIG_GRANT_TABLE is not set
>> +# CONFIG_SPECULATIVE_HARDEN_ARRAY is not set
>> +
>> +CONFIG_EXPERT=y
> These are rather odd defaults, more like for a special purpose
> config than a general purpose one. None of what you turn off here
> will guarantee to be off for people actually trying to build
> things, so it's not clear to me what the idea here is. As a
> specific remark, especially during bringup work I think it is
> quite important to not default DEBUG to off: You definitely want
> to see whether any assertions trigger.
The idea was to turn off as much stuff as possible to get a minimal
build (involving xen/common) working. Although now that we're focused on
only a few files at a time, they could be enabled without adding any
undue burden (at least for now).

Perhaps it would be best to rename the file to include "tiny" or something,
and then add a normal defconfig once things are actually running?

At any rate, agreed on the DEBUG setting, I will enable that.
>> --- /dev/null
>> +++ b/xen/include/asm-riscv/config.h
>> @@ -0,0 +1,110 @@
>> +/******************************************************************************
>> + * config.h
>> + *
>> + * A Linux-style configuration list.
> May I suggest to not further replicate this now inapplicable
> comment? It was already somewhat bogus for Arm to clone from x86.
Sure thing.
>
>> + */
>> +
>> +#ifndef __RISCV_CONFIG_H__
>> +#define __RISCV_CONFIG_H__
>> +
>> +#if defined(CONFIG_RISCV_64)
>> +# define LONG_BYTEORDER 3
>> +# define ELFSIZE 64
>> +#else
>> +# error "Unsupported RISCV variant"
>> +#endif
>> +
>> +#define BYTES_PER_LONG (1 << LONG_BYTEORDER)
>> +#define BITS_PER_LONG  (BYTES_PER_LONG << 3)
>> +#define POINTER_ALIGN  BYTES_PER_LONG
>> +
>> +#define BITS_PER_LLONG 64
>> +
>> +/* xen_ulong_t is always 64 bits */
>> +#define BITS_PER_XEN_ULONG 64
>> +
>> +#define CONFIG_RISCV 1
> This duplicates a "real" Kconfig setting, doesn't it?
Yes, will remove, thanks
>
>> +#define CONFIG_RISCV_L1_CACHE_SHIFT 6
>> +
>> +#define CONFIG_PAGEALLOC_MAX_ORDER 18
>> +#define CONFIG_DOMU_MAX_ORDER      9
>> +#define CONFIG_HWDOM_MAX_ORDER     10
>> +
>> +#define OPT_CONSOLE_STR "dtuart"
>> +
>> +#ifdef CONFIG_RISCV_64
>> +#define MAX_VIRT_CPUS 128u
>> +#else
>> +#error "Unsupported RISCV variant"
>> +#endif
> Could this be folded with the other similar construct further up?
Yep, will do.

Thanks,
Connor


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH
  2021-05-18  4:11       ` Connor Davis
@ 2021-05-18  6:27         ` Jan Beulich
  2021-05-18 14:06           ` Julien Grall
  0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2021-05-18  6:27 UTC (permalink / raw)
  To: Connor Davis
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Stefano Stabellini, Wei Liu, Paul Durrant,
	xen-devel, Julien Grall

On 18.05.2021 06:11, Connor Davis wrote:
> 
> On 5/17/21 9:42 AM, Julien Grall wrote:
>> Hi Jan,
>>
>> On 17/05/2021 12:16, Jan Beulich wrote:
>>> On 14.05.2021 20:53, Connor Davis wrote:
>>>> --- a/xen/common/memory.c
>>>> +++ b/xen/common/memory.c
>>>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned 
>>>> long gmfn)
>>>>       p2m_type_t p2mt;
>>>>   #endif
>>>>       mfn_t mfn;
>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>       bool *dont_flush_p, dont_flush;
>>>> +#endif
>>>>       int rc;
>>>>     #ifdef CONFIG_X86
>>>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, 
>>>> unsigned long gmfn)
>>>>        * Since we're likely to free the page below, we need to suspend
>>>>        * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes.
>>>>        */
>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>       dont_flush_p = &this_cpu(iommu_dont_flush_iotlb);
>>>>       dont_flush = *dont_flush_p;
>>>>       *dont_flush_p = false;
>>>> +#endif
>>>>         rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>>>>   +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>       *dont_flush_p = dont_flush;
>>>> +#endif
>>>>         /*
>>>>        * With the lack of an IOMMU on some platforms, domains with 
>>>> DMA-capable
>>>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, 
>>>> struct xen_add_to_physmap *xatp,
>>>>       xatp->gpfn += start;
>>>>       xatp->size -= start;
>>>>   +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>       if ( is_iommu_enabled(d) )
>>>>       {
>>>>          this_cpu(iommu_dont_flush_iotlb) = 1;
>>>>          extra.ppage = &pages[0];
>>>>       }
>>>> +#endif
>>>>         while ( xatp->size > done )
>>>>       {
>>>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, 
>>>> struct xen_add_to_physmap *xatp,
>>>>           }
>>>>       }
>>>>   +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>       if ( is_iommu_enabled(d) )
>>>>       {
>>>>           int ret;
>>>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, 
>>>> struct xen_add_to_physmap *xatp,
>>>>           if ( unlikely(ret) && rc >= 0 )
>>>>               rc = ret;
>>>>       }
>>>> +#endif
>>>>         return rc;
>>>>   }
>>>
>>> I wonder whether all of these wouldn't better become CONFIG_X86:
>>> ISTR Julien indicating that he doesn't see the override getting used
>>> on Arm. (Julien, please correct me if I'm misremembering.)
>>
>> Right, so far, I haven't been in favor to introduce it because:
>>    1) The P2M code may free some memory. So you can't always ignore 
>> the flush (I think this is wrong for the upper layer to know when this 
>> can happen).
>>    2) It is unclear what happen if the IOMMU TLBs and the PT contains 
>> different mappings (I received conflicted advice).
>>
>> So it is better to always flush and as early as possible.
> 
> So keep it as is or switch to CONFIG_X86?

Please switch, unless anyone else voices a strong opinion towards
keeping as is.

Jan


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 3/5] xen: Fix build when !CONFIG_GRANT_TABLE
  2021-05-18  3:58     ` Connor Davis
@ 2021-05-18  6:31       ` Jan Beulich
  0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2021-05-18  6:31 UTC (permalink / raw)
  To: Connor Davis
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
	xen-devel

On 18.05.2021 05:58, Connor Davis wrote:
> 
> On 5/17/21 5:22 AM, Jan Beulich wrote:
>> On 14.05.2021 20:53, Connor Davis wrote:
>>> Move struct grant_table; in grant_table.h above
>>> ifdef CONFIG_GRANT_TABLE. This fixes the following:
>>>
>>> /build/xen/include/xen/grant_table.h:84:50: error: 'struct grant_table'
>>> declared inside parameter list will not be visible outside of this
>>> definition or declaration [-Werror]
>>>     84 | static inline int mem_sharing_gref_to_gfn(struct grant_table *gt,
>>>        |
>> There must be more to this, as e.g. the PV shim does get built with
>> !GRANT_TABLE. Nevertheless, ...
>>
> Can you elaborate? I tested all defconfigs with and without grant tables
> 
> enabled on x86 and ARM and they all build fine.

I'm confused: Everything building fine supports my statement, so if
more elaboration was needed, it would be you to make more precise
the conditions upon which a build failure would occur. But this is
largely moot now, since I did commit your change yesterday already,
for, as said, the potential of breaking the build being obvious
enough.

Jan


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 4/5] xen: Add files needed for minimal riscv build
  2021-05-18  4:58     ` Connor Davis
@ 2021-05-18  6:33       ` Jan Beulich
  0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2021-05-18  6:33 UTC (permalink / raw)
  To: Connor Davis
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
	xen-devel

On 18.05.2021 06:58, Connor Davis wrote:
> On 5/17/21 5:51 AM, Jan Beulich wrote:
>> On 14.05.2021 20:53, Connor Davis wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/configs/riscv64_defconfig
>>> @@ -0,0 +1,12 @@
>>> +# CONFIG_SCHED_CREDIT is not set
>>> +# CONFIG_SCHED_RTDS is not set
>>> +# CONFIG_SCHED_NULL is not set
>>> +# CONFIG_SCHED_ARINC653 is not set
>>> +# CONFIG_TRACEBUFFER is not set
>>> +# CONFIG_DEBUG is not set
>>> +# CONFIG_DEBUG_INFO is not set
>>> +# CONFIG_HYPFS is not set
>>> +# CONFIG_GRANT_TABLE is not set
>>> +# CONFIG_SPECULATIVE_HARDEN_ARRAY is not set
>>> +
>>> +CONFIG_EXPERT=y
>> These are rather odd defaults, more like for a special purpose
>> config than a general purpose one. None of what you turn off here
>> will guarantee to be off for people actually trying to build
>> things, so it's not clear to me what the idea here is. As a
>> specific remark, especially during bringup work I think it is
>> quite important to not default DEBUG to off: You definitely want
>> to see whether any assertions trigger.
> The idea was to turn off as much stuff as possible to get a minimal
> build (involving xen/common) working. Although now that we're focused on
> only a few files at a time, they could be enabled without adding any
> undue burden (at least for now).
> 
> Perhaps it would be best to rename the file to include "tiny" or something,
> and then add a normal defconfig once things are actually running?

Yes please.

Jan


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH
  2021-05-18  6:27         ` Jan Beulich
@ 2021-05-18 14:06           ` Julien Grall
  2021-05-18 15:13             ` Jan Beulich
  0 siblings, 1 reply; 31+ messages in thread
From: Julien Grall @ 2021-05-18 14:06 UTC (permalink / raw)
  To: Jan Beulich, Connor Davis
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Stefano Stabellini, Wei Liu, Paul Durrant,
	xen-devel



On 18/05/2021 07:27, Jan Beulich wrote:
> On 18.05.2021 06:11, Connor Davis wrote:
>>
>> On 5/17/21 9:42 AM, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 17/05/2021 12:16, Jan Beulich wrote:
>>>> On 14.05.2021 20:53, Connor Davis wrote:
>>>>> --- a/xen/common/memory.c
>>>>> +++ b/xen/common/memory.c
>>>>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned
>>>>> long gmfn)
>>>>>        p2m_type_t p2mt;
>>>>>    #endif
>>>>>        mfn_t mfn;
>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>        bool *dont_flush_p, dont_flush;
>>>>> +#endif
>>>>>        int rc;
>>>>>      #ifdef CONFIG_X86
>>>>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d,
>>>>> unsigned long gmfn)
>>>>>         * Since we're likely to free the page below, we need to suspend
>>>>>         * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes.
>>>>>         */
>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>        dont_flush_p = &this_cpu(iommu_dont_flush_iotlb);
>>>>>        dont_flush = *dont_flush_p;
>>>>>        *dont_flush_p = false;
>>>>> +#endif
>>>>>          rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>>>>>    +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>        *dont_flush_p = dont_flush;
>>>>> +#endif
>>>>>          /*
>>>>>         * With the lack of an IOMMU on some platforms, domains with
>>>>> DMA-capable
>>>>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d,
>>>>> struct xen_add_to_physmap *xatp,
>>>>>        xatp->gpfn += start;
>>>>>        xatp->size -= start;
>>>>>    +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>        if ( is_iommu_enabled(d) )
>>>>>        {
>>>>>           this_cpu(iommu_dont_flush_iotlb) = 1;
>>>>>           extra.ppage = &pages[0];
>>>>>        }
>>>>> +#endif
>>>>>          while ( xatp->size > done )
>>>>>        {
>>>>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d,
>>>>> struct xen_add_to_physmap *xatp,
>>>>>            }
>>>>>        }
>>>>>    +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>        if ( is_iommu_enabled(d) )
>>>>>        {
>>>>>            int ret;
>>>>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d,
>>>>> struct xen_add_to_physmap *xatp,
>>>>>            if ( unlikely(ret) && rc >= 0 )
>>>>>                rc = ret;
>>>>>        }
>>>>> +#endif
>>>>>          return rc;
>>>>>    }
>>>>
>>>> I wonder whether all of these wouldn't better become CONFIG_X86:
>>>> ISTR Julien indicating that he doesn't see the override getting used
>>>> on Arm. (Julien, please correct me if I'm misremembering.)
>>>
>>> Right, so far, I haven't been in favor to introduce it because:
>>>     1) The P2M code may free some memory. So you can't always ignore
>>> the flush (I think this is wrong for the upper layer to know when this
>>> can happen).
>>>     2) It is unclear what happen if the IOMMU TLBs and the PT contains
>>> different mappings (I received conflicted advice).
>>>
>>> So it is better to always flush and as early as possible.
>>
>> So keep it as is or switch to CONFIG_X86?
> 
> Please switch, unless anyone else voices a strong opinion towards
> keeping as is.

I would like to avoid adding more #ifdef CONFIG_X86 in the common code. 
Can we instead provide a wrapper for them?

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH
  2021-05-18 14:06           ` Julien Grall
@ 2021-05-18 15:13             ` Jan Beulich
  2021-05-18 15:17               ` Julien Grall
  0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2021-05-18 15:13 UTC (permalink / raw)
  To: Julien Grall, Connor Davis
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Stefano Stabellini, Wei Liu, Paul Durrant,
	xen-devel

On 18.05.2021 16:06, Julien Grall wrote:
> 
> 
> On 18/05/2021 07:27, Jan Beulich wrote:
>> On 18.05.2021 06:11, Connor Davis wrote:
>>>
>>> On 5/17/21 9:42 AM, Julien Grall wrote:
>>>> Hi Jan,
>>>>
>>>> On 17/05/2021 12:16, Jan Beulich wrote:
>>>>> On 14.05.2021 20:53, Connor Davis wrote:
>>>>>> --- a/xen/common/memory.c
>>>>>> +++ b/xen/common/memory.c
>>>>>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned
>>>>>> long gmfn)
>>>>>>        p2m_type_t p2mt;
>>>>>>    #endif
>>>>>>        mfn_t mfn;
>>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>        bool *dont_flush_p, dont_flush;
>>>>>> +#endif
>>>>>>        int rc;
>>>>>>      #ifdef CONFIG_X86
>>>>>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d,
>>>>>> unsigned long gmfn)
>>>>>>         * Since we're likely to free the page below, we need to suspend
>>>>>>         * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes.
>>>>>>         */
>>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>        dont_flush_p = &this_cpu(iommu_dont_flush_iotlb);
>>>>>>        dont_flush = *dont_flush_p;
>>>>>>        *dont_flush_p = false;
>>>>>> +#endif
>>>>>>          rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>>>>>>    +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>        *dont_flush_p = dont_flush;
>>>>>> +#endif
>>>>>>          /*
>>>>>>         * With the lack of an IOMMU on some platforms, domains with
>>>>>> DMA-capable
>>>>>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d,
>>>>>> struct xen_add_to_physmap *xatp,
>>>>>>        xatp->gpfn += start;
>>>>>>        xatp->size -= start;
>>>>>>    +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>        if ( is_iommu_enabled(d) )
>>>>>>        {
>>>>>>           this_cpu(iommu_dont_flush_iotlb) = 1;
>>>>>>           extra.ppage = &pages[0];
>>>>>>        }
>>>>>> +#endif
>>>>>>          while ( xatp->size > done )
>>>>>>        {
>>>>>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d,
>>>>>> struct xen_add_to_physmap *xatp,
>>>>>>            }
>>>>>>        }
>>>>>>    +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>        if ( is_iommu_enabled(d) )
>>>>>>        {
>>>>>>            int ret;
>>>>>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d,
>>>>>> struct xen_add_to_physmap *xatp,
>>>>>>            if ( unlikely(ret) && rc >= 0 )
>>>>>>                rc = ret;
>>>>>>        }
>>>>>> +#endif
>>>>>>          return rc;
>>>>>>    }
>>>>>
>>>>> I wonder whether all of these wouldn't better become CONFIG_X86:
>>>>> ISTR Julien indicating that he doesn't see the override getting used
>>>>> on Arm. (Julien, please correct me if I'm misremembering.)
>>>>
>>>> Right, so far, I haven't been in favor to introduce it because:
>>>>     1) The P2M code may free some memory. So you can't always ignore
>>>> the flush (I think this is wrong for the upper layer to know when this
>>>> can happen).
>>>>     2) It is unclear what happen if the IOMMU TLBs and the PT contains
>>>> different mappings (I received conflicted advice).
>>>>
>>>> So it is better to always flush and as early as possible.
>>>
>>> So keep it as is or switch to CONFIG_X86?
>>
>> Please switch, unless anyone else voices a strong opinion towards
>> keeping as is.
> 
> I would like to avoid adding more #ifdef CONFIG_X86 in the common code. 
> Can we instead provide a wrapper for them?

Doable, sure, but I don't know whether Connor is up to going this
more extensive route.

Jan


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH
  2021-05-18 15:13             ` Jan Beulich
@ 2021-05-18 15:17               ` Julien Grall
  0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2021-05-18 15:17 UTC (permalink / raw)
  To: Jan Beulich, Connor Davis
  Cc: Bobby Eshleman, Alistair Francis, Andrew Cooper, George Dunlap,
	Ian Jackson, Stefano Stabellini, Wei Liu, Paul Durrant,
	xen-devel

Hi Jan,

On 18/05/2021 16:13, Jan Beulich wrote:
> On 18.05.2021 16:06, Julien Grall wrote:
>>
>>
>> On 18/05/2021 07:27, Jan Beulich wrote:
>>> On 18.05.2021 06:11, Connor Davis wrote:
>>>>
>>>> On 5/17/21 9:42 AM, Julien Grall wrote:
>>>>> Hi Jan,
>>>>>
>>>>> On 17/05/2021 12:16, Jan Beulich wrote:
>>>>>> On 14.05.2021 20:53, Connor Davis wrote:
>>>>>>> --- a/xen/common/memory.c
>>>>>>> +++ b/xen/common/memory.c
>>>>>>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned
>>>>>>> long gmfn)
>>>>>>>         p2m_type_t p2mt;
>>>>>>>     #endif
>>>>>>>         mfn_t mfn;
>>>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>>         bool *dont_flush_p, dont_flush;
>>>>>>> +#endif
>>>>>>>         int rc;
>>>>>>>       #ifdef CONFIG_X86
>>>>>>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d,
>>>>>>> unsigned long gmfn)
>>>>>>>          * Since we're likely to free the page below, we need to suspend
>>>>>>>          * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes.
>>>>>>>          */
>>>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>>         dont_flush_p = &this_cpu(iommu_dont_flush_iotlb);
>>>>>>>         dont_flush = *dont_flush_p;
>>>>>>>         *dont_flush_p = false;
>>>>>>> +#endif
>>>>>>>           rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>>>>>>>     +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>>         *dont_flush_p = dont_flush;
>>>>>>> +#endif
>>>>>>>           /*
>>>>>>>          * With the lack of an IOMMU on some platforms, domains with
>>>>>>> DMA-capable
>>>>>>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d,
>>>>>>> struct xen_add_to_physmap *xatp,
>>>>>>>         xatp->gpfn += start;
>>>>>>>         xatp->size -= start;
>>>>>>>     +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>>         if ( is_iommu_enabled(d) )
>>>>>>>         {
>>>>>>>            this_cpu(iommu_dont_flush_iotlb) = 1;
>>>>>>>            extra.ppage = &pages[0];
>>>>>>>         }
>>>>>>> +#endif
>>>>>>>           while ( xatp->size > done )
>>>>>>>         {
>>>>>>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d,
>>>>>>> struct xen_add_to_physmap *xatp,
>>>>>>>             }
>>>>>>>         }
>>>>>>>     +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>>>>         if ( is_iommu_enabled(d) )
>>>>>>>         {
>>>>>>>             int ret;
>>>>>>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d,
>>>>>>> struct xen_add_to_physmap *xatp,
>>>>>>>             if ( unlikely(ret) && rc >= 0 )
>>>>>>>                 rc = ret;
>>>>>>>         }
>>>>>>> +#endif
>>>>>>>           return rc;
>>>>>>>     }
>>>>>>
>>>>>> I wonder whether all of these wouldn't better become CONFIG_X86:
>>>>>> ISTR Julien indicating that he doesn't see the override getting used
>>>>>> on Arm. (Julien, please correct me if I'm misremembering.)
>>>>>
>>>>> Right, so far, I haven't been in favor to introduce it because:
>>>>>      1) The P2M code may free some memory. So you can't always ignore
>>>>> the flush (I think this is wrong for the upper layer to know when this
>>>>> can happen).
>>>>>      2) It is unclear what happen if the IOMMU TLBs and the PT contains
>>>>> different mappings (I received conflicted advice).
>>>>>
>>>>> So it is better to always flush and as early as possible.
>>>>
>>>> So keep it as is or switch to CONFIG_X86?
>>>
>>> Please switch, unless anyone else voices a strong opinion towards
>>> keeping as is.
>>
>> I would like to avoid adding more #ifdef CONFIG_X86 in the common code.
>> Can we instead provide a wrapper for them?
> 
> Doable, sure, but I don't know whether Connor is up to going this
> more extensive route.

That's a fair point. If that the case, then I prefer the #ifdef 
CONFIG_HAS_PASSTHROUGH version.

I can add an item in my todo list to introduce some helpers.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2021-05-18 15:17 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-14 18:53 [PATCH v3 0/5] Minimal build for RISCV Connor Davis
2021-05-14 18:53 ` [PATCH v3 1/5] xen/char: Default HAS_NS16550 to y only for X86 and ARM Connor Davis
2021-05-16 22:48   ` Alistair Francis
2021-05-17 11:56   ` Jan Beulich
2021-05-17 23:43     ` Connor Davis
2021-05-14 18:53 ` [PATCH v3 2/5] xen/common: Guard iommu symbols with CONFIG_HAS_PASSTHROUGH Connor Davis
2021-05-17 11:16   ` Jan Beulich
2021-05-17 13:52     ` Connor Davis
2021-05-17 15:42     ` Julien Grall
2021-05-18  4:11       ` Connor Davis
2021-05-18  6:27         ` Jan Beulich
2021-05-18 14:06           ` Julien Grall
2021-05-18 15:13             ` Jan Beulich
2021-05-18 15:17               ` Julien Grall
2021-05-14 18:53 ` [PATCH v3 3/5] xen: Fix build when !CONFIG_GRANT_TABLE Connor Davis
2021-05-17 11:22   ` Jan Beulich
2021-05-17 23:46     ` Connor Davis
2021-05-18  3:58     ` Connor Davis
2021-05-18  6:31       ` Jan Beulich
2021-05-14 18:53 ` [PATCH v3 4/5] xen: Add files needed for minimal riscv build Connor Davis
2021-05-14 21:53   ` Bob Eshleman
2021-05-14 23:47     ` Connor Davis
2021-05-18  1:43       ` Bob Eshleman
2021-05-18  4:05         ` Connor Davis
2021-05-17 11:51   ` Jan Beulich
2021-05-18  4:58     ` Connor Davis
2021-05-18  6:33       ` Jan Beulich
2021-05-14 18:53 ` [PATCH v3 5/5] automation: Add container for riscv64 builds Connor Davis
2021-05-14 21:01   ` Bob Eshleman
2021-05-14 23:54     ` Connor Davis
2021-05-17 23:20 ` [PATCH v3 0/5] Minimal build for RISCV Roman Shaposhnik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).