* [PATCH v4 00/12] SVE feature for arm guests
@ 2023-03-27 10:59 Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 01/12] xen/arm: enable SVE extension for Xen Luca Fancellu
` (11 more replies)
0 siblings, 12 replies; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Roger Pau Monné,
Nick Rosbrook, Anthony PERARD, Juergen Gross, Christian Lindig,
David Scott, Marek Marczykowski-Górecki, Henry Wang,
Community Manager
This serie is introducing the possibility for Dom0 and DomU guests to use
sve/sve2 instructions.
SVE feature introduces new instruction and registers to improve performances on
floating point operations.
The SVE feature is advertised using the ID_AA64PFR0_EL1 register, SVE field, and
when available the ID_AA64ZFR0_EL1 register provides additional information
about the implemented version and other SVE feature.
New registers added by the SVE feature are Z0-Z31, P0-P15, FFR, ZCR_ELx.
Z0-Z31 are scalable vector register whose size is implementation defined and
goes from 128 bits to maximum 2048, the term vector length will be used to refer
to this quantity.
P0-P15 are predicate registers and the size is the vector length divided by 8,
same size is the FFR (First Fault Register).
ZCR_ELx is a register that can control and restrict the maximum vector length
used by the <x> exception level and all the lower exception levels, so for
example EL3 can restrict the vector length usable by EL3,2,1,0.
The platform has a maximum implemented vector length, so for every value
written in ZCR register, if this value is above the implemented length, then the
lower value will be used. The RDVL instruction can be used to check what vector
length is the HW using after setting ZCR.
For an SVE guest, the V0-V31 registers are part of the Z0-Z31, so there is no
need to save them separately, saving Z0-Z31 will save implicitly also V0-V31.
SVE usage can be trapped using a flag in CPTR_EL2, hence in this serie the
register is added to the domain state, to be able to trap only the guests that
are not allowed to use SVE.
This serie is introducing a command line parameter to enable Dom0 to use SVE and
to set its maximum vector length that by default is 0 which means the guest is
not allowed to use SVE. Values from 128 to 2048 mean the guest can use SVE with
the selected value used as maximum allowed vector length (which could be lower
if the implemented one is lower).
For DomUs, an XL parameter with the same way of use is introduced and a dom0less
DTB binding is created.
The context switch is the most critical part because there can be big registers
to be saved, in this serie an easy approach is used and the context is
saved/restored every time for the guests that are allowed to use SVE.
Luca Fancellu (12):
xen/arm: enable SVE extension for Xen
xen/arm: add SVE vector length field to the domain
xen/arm: Expose SVE feature to the guest
xen/arm: add SVE exception class handling
arm/sve: save/restore SVE context switch
xen/common: add dom0 xen command line argument for Arm
xen: enable Dom0 to use SVE feature
xen/physinfo: encode Arm SVE vector length in arch_capabilities
tools: add physinfo arch_capabilities handling for Arm
xen/tools: add sve parameter in XL configuration
xen/arm: add sve property for dom0less domUs
xen/changelog: Add SVE and "dom0" options to the changelog for Arm
CHANGELOG.md | 5 +
docs/man/xl.cfg.5.pod.in | 11 ++
docs/misc/arm/device-tree/booting.txt | 9 ++
docs/misc/xen-command-line.pandoc | 16 +-
tools/golang/xenlight/helpers.gen.go | 4 +
tools/golang/xenlight/types.gen.go | 2 +
tools/include/arm-arch-capabilities.h | 33 ++++
tools/include/libxl.h | 5 +
tools/include/xen-tools/common-macros.h | 2 +
tools/libs/light/libxl.c | 1 +
tools/libs/light/libxl_arm.c | 2 +
tools/libs/light/libxl_internal.h | 1 -
tools/libs/light/libxl_types.idl | 2 +
tools/ocaml/libs/xc/xenctrl.ml | 4 +-
tools/ocaml/libs/xc/xenctrl.mli | 4 +-
tools/ocaml/libs/xc/xenctrl_stubs.c | 8 +-
tools/python/xen/lowlevel/xc/xc.c | 10 +-
tools/xl/xl_info.c | 8 +
tools/xl/xl_parse.c | 26 +++-
xen/arch/arm/Kconfig | 10 +-
xen/arch/arm/arm64/Makefile | 1 +
xen/arch/arm/arm64/cpufeature.c | 7 +-
xen/arch/arm/arm64/sve-asm.S | 189 +++++++++++++++++++++++
xen/arch/arm/arm64/sve.c | 138 +++++++++++++++++
xen/arch/arm/arm64/vfp.c | 79 ++++++----
xen/arch/arm/arm64/vsysreg.c | 39 ++++-
xen/arch/arm/cpufeature.c | 6 +-
xen/arch/arm/domain.c | 48 +++++-
xen/arch/arm/domain_build.c | 21 +++
xen/arch/arm/include/asm/arm64/sve.h | 92 +++++++++++
xen/arch/arm/include/asm/arm64/sysregs.h | 4 +
xen/arch/arm/include/asm/arm64/vfp.h | 10 ++
xen/arch/arm/include/asm/cpufeature.h | 14 ++
xen/arch/arm/include/asm/domain.h | 8 +
xen/arch/arm/include/asm/processor.h | 3 +
xen/arch/arm/setup.c | 5 +-
xen/arch/arm/sysctl.c | 3 +
xen/arch/arm/traps.c | 40 +++--
xen/arch/x86/dom0_build.c | 46 ++----
xen/common/domain.c | 23 +++
xen/common/kernel.c | 24 +++
xen/include/public/arch-arm.h | 2 +
xen/include/public/domctl.h | 2 +-
xen/include/public/sysctl.h | 4 +
xen/include/xen/domain.h | 1 +
xen/include/xen/lib.h | 10 ++
46 files changed, 874 insertions(+), 108 deletions(-)
create mode 100644 tools/include/arm-arch-capabilities.h
create mode 100644 xen/arch/arm/arm64/sve-asm.S
create mode 100644 xen/arch/arm/arm64/sve.c
create mode 100644 xen/arch/arm/include/asm/arm64/sve.h
--
2.34.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v4 01/12] xen/arm: enable SVE extension for Xen
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
@ 2023-03-27 10:59 ` Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 02/12] xen/arm: add SVE vector length field to the domain Luca Fancellu
` (10 subsequent siblings)
11 siblings, 0 replies; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk
Enable Xen to handle the SVE extension, add code in cpufeature module
to handle ZCR SVE register, disable trapping SVE feature on system
boot only when SVE resources are accessed.
While there, correct coding style for the comment on coprocessor
trapping.
Now cptr_el2 is part of the domain context and it will be restored
on context switch, this is a preparation for saving the SVE context
which will be part of VFP operations, so restore it before the call
to save VFP registers.
To save an additional isb barrier, restore cptr_el2 before an
existing isb barrier and move the call for saving VFP context after
that barrier.
Change the KConfig entry to make ARM64_SVE symbol selectable, by
default it will be not selected.
Create sve module and sve_asm.S that contains assembly routines for
the SVE feature, this code is inspired from linux and it uses
instruction encoding to be compatible with compilers that does not
support SVE.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v3:
- no changes
Changes from v2:
- renamed sve_asm.S in sve-asm.S, new files should not contain
underscore in the name (Jan)
Changes from v1:
- Add assert to vl_to_zcr, it is never called with vl==0, but just
to be sure it won't in the future.
Changes from RFC:
- Moved restoring of cptr before an existing barrier (Julien)
- Marked the feature as unsupported for now (Julien)
- Trap and un-trap only when using SVE resources in
compute_max_zcr() (Julien)
---
xen/arch/arm/Kconfig | 10 +++--
xen/arch/arm/arm64/Makefile | 1 +
xen/arch/arm/arm64/cpufeature.c | 7 ++--
xen/arch/arm/arm64/sve-asm.S | 48 +++++++++++++++++++++++
xen/arch/arm/arm64/sve.c | 50 ++++++++++++++++++++++++
xen/arch/arm/cpufeature.c | 6 ++-
xen/arch/arm/domain.c | 9 +++--
xen/arch/arm/include/asm/arm64/sve.h | 43 ++++++++++++++++++++
xen/arch/arm/include/asm/arm64/sysregs.h | 1 +
xen/arch/arm/include/asm/cpufeature.h | 14 +++++++
xen/arch/arm/include/asm/domain.h | 1 +
xen/arch/arm/include/asm/processor.h | 2 +
xen/arch/arm/setup.c | 5 ++-
xen/arch/arm/traps.c | 28 +++++++------
14 files changed, 201 insertions(+), 24 deletions(-)
create mode 100644 xen/arch/arm/arm64/sve-asm.S
create mode 100644 xen/arch/arm/arm64/sve.c
create mode 100644 xen/arch/arm/include/asm/arm64/sve.h
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 239d3aed3c7f..41f45d8d1203 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -112,11 +112,15 @@ config ARM64_PTR_AUTH
This feature is not supported in Xen.
config ARM64_SVE
- def_bool n
+ bool "Enable Scalar Vector Extension support (UNSUPPORTED)" if UNSUPPORTED
depends on ARM_64
help
- Scalar Vector Extension support.
- This feature is not supported in Xen.
+ Scalar Vector Extension (SVE/SVE2) support for guests.
+
+ Please be aware that currently, enabling this feature will add latency on
+ VM context switch between SVE enabled guests, between not-enabled SVE
+ guests and SVE enabled guests and viceversa, compared to the time
+ required to switch between not-enabled SVE guests.
config ARM64_MTE
def_bool n
diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index 6d507da0d44d..24e08fd42596 100644
--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -12,6 +12,7 @@ obj-y += insn.o
obj-$(CONFIG_LIVEPATCH) += livepatch.o
obj-y += smc.o
obj-y += smpboot.o
+obj-$(CONFIG_ARM64_SVE) += sve.o sve-asm.o
obj-y += traps.o
obj-y += vfp.o
obj-y += vsysreg.o
diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
index d9039d37b2d1..b4656ff4d80f 100644
--- a/xen/arch/arm/arm64/cpufeature.c
+++ b/xen/arch/arm/arm64/cpufeature.c
@@ -455,15 +455,11 @@ static const struct arm64_ftr_bits ftr_id_dfr1[] = {
ARM64_FTR_END,
};
-#if 0
-/* TODO: use this to sanitize SVE once we support it */
-
static const struct arm64_ftr_bits ftr_zcr[] = {
ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE,
ZCR_ELx_LEN_SHIFT, ZCR_ELx_LEN_SIZE, 0), /* LEN */
ARM64_FTR_END,
};
-#endif
/*
* Common ftr bits for a 32bit register with all hidden, strict
@@ -603,6 +599,9 @@ void update_system_features(const struct cpuinfo_arm *new)
SANITIZE_ID_REG(zfr64, 0, aa64zfr0);
+ if ( cpu_has_sve )
+ SANITIZE_REG(zcr64, 0, zcr);
+
/*
* Comment from Linux:
* Userspace may perform DC ZVA instructions. Mismatched block sizes
diff --git a/xen/arch/arm/arm64/sve-asm.S b/xen/arch/arm/arm64/sve-asm.S
new file mode 100644
index 000000000000..4d1549344733
--- /dev/null
+++ b/xen/arch/arm/arm64/sve-asm.S
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Arm SVE assembly routines
+ *
+ * Copyright (C) 2022 ARM Ltd.
+ *
+ * Some macros and instruction encoding in this file are taken from linux 6.1.1,
+ * file arch/arm64/include/asm/fpsimdmacros.h, some of them are a modified
+ * version.
+ */
+
+/* Sanity-check macros to help avoid encoding garbage instructions */
+
+.macro _check_general_reg nr
+ .if (\nr) < 0 || (\nr) > 30
+ .error "Bad register number \nr."
+ .endif
+.endm
+
+.macro _check_num n, min, max
+ .if (\n) < (\min) || (\n) > (\max)
+ .error "Number \n out of range [\min,\max]"
+ .endif
+.endm
+
+/* SVE instruction encodings for non-SVE-capable assemblers */
+/* (pre binutils 2.28, all kernel capable clang versions support SVE) */
+
+/* RDVL X\nx, #\imm */
+.macro _sve_rdvl nx, imm
+ _check_general_reg \nx
+ _check_num (\imm), -0x20, 0x1f
+ .inst 0x04bf5000 \
+ | (\nx) \
+ | (((\imm) & 0x3f) << 5)
+.endm
+
+/* Gets the current vector register size in bytes */
+GLOBAL(sve_get_hw_vl)
+ _sve_rdvl 0, 1
+ ret
+
+/*
+ * Local variables:
+ * mode: ASM
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c
new file mode 100644
index 000000000000..c466de61b47f
--- /dev/null
+++ b/xen/arch/arm/arm64/sve.c
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Arm SVE feature code
+ *
+ * Copyright (C) 2022 ARM Ltd.
+ */
+
+#include <xen/types.h>
+#include <asm/arm64/sve.h>
+#include <asm/arm64/sysregs.h>
+#include <asm/processor.h>
+#include <asm/system.h>
+
+extern unsigned int sve_get_hw_vl(void);
+
+register_t compute_max_zcr(void)
+{
+ register_t cptr_bits = get_default_cptr_flags();
+ register_t zcr = vl_to_zcr(SVE_VL_MAX_BITS);
+ unsigned int hw_vl;
+
+ /* Remove trap for SVE resources */
+ WRITE_SYSREG(cptr_bits & ~HCPTR_CP(8), CPTR_EL2);
+ isb();
+
+ /*
+ * Set the maximum SVE vector length, doing that we will know the VL
+ * supported by the platform, calling sve_get_hw_vl()
+ */
+ WRITE_SYSREG(zcr, ZCR_EL2);
+
+ /*
+ * Read the maximum VL, which could be lower than what we imposed before,
+ * hw_vl contains VL in bytes, multiply it by 8 to use vl_to_zcr() later
+ */
+ hw_vl = sve_get_hw_vl() * 8U;
+
+ /* Restore CPTR_EL2 */
+ WRITE_SYSREG(cptr_bits, CPTR_EL2);
+ isb();
+
+ return vl_to_zcr(hw_vl);
+}
+
+/* Takes a vector length in bits and returns the ZCR_ELx encoding */
+register_t vl_to_zcr(uint16_t vl)
+{
+ ASSERT(vl > 0);
+ return ((vl / SVE_VL_MULTIPLE_VAL) - 1U) & ZCR_ELx_LEN_MASK;
+}
diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
index c4ec38bb2554..83b84368f6d5 100644
--- a/xen/arch/arm/cpufeature.c
+++ b/xen/arch/arm/cpufeature.c
@@ -9,6 +9,7 @@
#include <xen/init.h>
#include <xen/smp.h>
#include <xen/stop_machine.h>
+#include <asm/arm64/sve.h>
#include <asm/cpufeature.h>
DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS);
@@ -143,6 +144,9 @@ void identify_cpu(struct cpuinfo_arm *c)
c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
+ if ( cpu_has_sve )
+ c->zcr64.bits[0] = compute_max_zcr();
+
c->dczid.bits[0] = READ_SYSREG(DCZID_EL0);
c->ctr.bits[0] = READ_SYSREG(CTR_EL0);
@@ -199,7 +203,7 @@ static int __init create_guest_cpuinfo(void)
guest_cpuinfo.pfr64.mpam = 0;
guest_cpuinfo.pfr64.mpam_frac = 0;
- /* Hide SVE as Xen does not support it */
+ /* Hide SVE by default to the guests */
guest_cpuinfo.pfr64.sve = 0;
guest_cpuinfo.zfr64.bits[0] = 0;
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 99577adb6c69..adb6ace2e24d 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -181,9 +181,6 @@ static void ctxt_switch_to(struct vcpu *n)
/* VGIC */
gic_restore_state(n);
- /* VFP */
- vfp_restore_state(n);
-
/* XXX MPU */
/* Fault Status */
@@ -234,6 +231,7 @@ static void ctxt_switch_to(struct vcpu *n)
p2m_restore_state(n);
/* Control Registers */
+ WRITE_SYSREG(n->arch.cptr_el2, CPTR_EL2);
WRITE_SYSREG(n->arch.cpacr, CPACR_EL1);
/*
@@ -258,6 +256,9 @@ static void ctxt_switch_to(struct vcpu *n)
#endif
isb();
+ /* VFP */
+ vfp_restore_state(n);
+
/* CP 15 */
WRITE_SYSREG(n->arch.csselr, CSSELR_EL1);
@@ -548,6 +549,8 @@ int arch_vcpu_create(struct vcpu *v)
v->arch.vmpidr = MPIDR_SMP | vcpuid_to_vaffinity(v->vcpu_id);
+ v->arch.cptr_el2 = get_default_cptr_flags();
+
v->arch.hcr_el2 = get_default_hcr_flags();
v->arch.mdcr_el2 = HDCR_TDRA | HDCR_TDOSA | HDCR_TDA;
diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h
new file mode 100644
index 000000000000..bd56e2f24230
--- /dev/null
+++ b/xen/arch/arm/include/asm/arm64/sve.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Arm SVE feature code
+ *
+ * Copyright (C) 2022 ARM Ltd.
+ */
+
+#ifndef _ARM_ARM64_SVE_H
+#define _ARM_ARM64_SVE_H
+
+#define SVE_VL_MAX_BITS (2048U)
+
+/* Vector length must be multiple of 128 */
+#define SVE_VL_MULTIPLE_VAL (128U)
+
+#ifdef CONFIG_ARM64_SVE
+
+register_t compute_max_zcr(void);
+register_t vl_to_zcr(uint16_t vl);
+
+#else /* !CONFIG_ARM64_SVE */
+
+static inline register_t compute_max_zcr(void)
+{
+ return 0;
+}
+
+static inline register_t vl_to_zcr(uint16_t vl)
+{
+ return 0;
+}
+
+#endif
+
+#endif /* _ARM_ARM64_SVE_H */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
index 463899951414..4cabb9eb4d5e 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -24,6 +24,7 @@
#define ICH_EISR_EL2 S3_4_C12_C11_3
#define ICH_ELSR_EL2 S3_4_C12_C11_5
#define ICH_VMCR_EL2 S3_4_C12_C11_7
+#define ZCR_EL2 S3_4_C1_C2_0
#define __LR0_EL2(x) S3_4_C12_C12_ ## x
#define __LR8_EL2(x) S3_4_C12_C13_ ## x
diff --git a/xen/arch/arm/include/asm/cpufeature.h b/xen/arch/arm/include/asm/cpufeature.h
index c62cf6293fd6..6d703e051906 100644
--- a/xen/arch/arm/include/asm/cpufeature.h
+++ b/xen/arch/arm/include/asm/cpufeature.h
@@ -32,6 +32,12 @@
#define cpu_has_thumbee (boot_cpu_feature32(thumbee) == 1)
#define cpu_has_aarch32 (cpu_has_arm || cpu_has_thumb)
+#ifdef CONFIG_ARM64_SVE
+#define cpu_has_sve (boot_cpu_feature64(sve) == 1)
+#else
+#define cpu_has_sve (0)
+#endif
+
#ifdef CONFIG_ARM_32
#define cpu_has_gicv3 (boot_cpu_feature32(gic) >= 1)
#define cpu_has_gentimer (boot_cpu_feature32(gentimer) == 1)
@@ -323,6 +329,14 @@ struct cpuinfo_arm {
};
} isa64;
+ union {
+ register_t bits[1];
+ struct {
+ unsigned long len:4;
+ unsigned long __res0:60;
+ };
+ } zcr64;
+
struct {
register_t bits[1];
} zfr64;
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index 2a51f0ca688e..e776ee704b7d 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -190,6 +190,7 @@ struct arch_vcpu
register_t tpidrro_el0;
/* HYP configuration */
+ register_t cptr_el2;
register_t hcr_el2;
register_t mdcr_el2;
diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
index 54f253087718..bc683334125c 100644
--- a/xen/arch/arm/include/asm/processor.h
+++ b/xen/arch/arm/include/asm/processor.h
@@ -582,6 +582,8 @@ void do_trap_guest_serror(struct cpu_user_regs *regs);
register_t get_default_hcr_flags(void);
+register_t get_default_cptr_flags(void);
+
/*
* Synchronize SError unless the feature is selected.
* This is relying on the SErrors are currently unmasked.
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 1f26f67b90e3..5459cc4f5e62 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -135,10 +135,11 @@ static void __init processor_id(void)
cpu_has_el2_32 ? "64+32" : cpu_has_el2_64 ? "64" : "No",
cpu_has_el1_32 ? "64+32" : cpu_has_el1_64 ? "64" : "No",
cpu_has_el0_32 ? "64+32" : cpu_has_el0_64 ? "64" : "No");
- printk(" Extensions:%s%s%s\n",
+ printk(" Extensions:%s%s%s%s\n",
cpu_has_fp ? " FloatingPoint" : "",
cpu_has_simd ? " AdvancedSIMD" : "",
- cpu_has_gicv3 ? " GICv3-SysReg" : "");
+ cpu_has_gicv3 ? " GICv3-SysReg" : "",
+ cpu_has_sve ? " SVE" : "");
/* Warn user if we find unknown floating-point features */
if ( cpu_has_fp && (boot_cpu_feature64(fp) >= 2) )
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 061c92acbd68..a78a99ddadd0 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -93,6 +93,21 @@ register_t get_default_hcr_flags(void)
HCR_TID3|HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB|HCR_TSW);
}
+register_t get_default_cptr_flags(void)
+{
+ /*
+ * Trap all coprocessor registers (0-13) except cp10 and
+ * cp11 for VFP.
+ *
+ * /!\ All coprocessors except cp10 and cp11 cannot be used in Xen.
+ *
+ * On ARM64 the TCPx bits which we set here (0..9,12,13) are all
+ * RES1, i.e. they would trap whether we did this write or not.
+ */
+ return ((HCPTR_CP_MASK & ~(HCPTR_CP(10) | HCPTR_CP(11))) |
+ HCPTR_TTA | HCPTR_TAM);
+}
+
static enum {
SERRORS_DIVERSE,
SERRORS_PANIC,
@@ -122,6 +137,7 @@ __initcall(update_serrors_cpu_caps);
void init_traps(void)
{
+ register_t cptr_bits = get_default_cptr_flags();
/*
* Setup Hyp vector base. Note they might get updated with the
* branch predictor hardening.
@@ -135,17 +151,7 @@ void init_traps(void)
/* Trap CP15 c15 used for implementation defined registers */
WRITE_SYSREG(HSTR_T(15), HSTR_EL2);
- /* Trap all coprocessor registers (0-13) except cp10 and
- * cp11 for VFP.
- *
- * /!\ All coprocessors except cp10 and cp11 cannot be used in Xen.
- *
- * On ARM64 the TCPx bits which we set here (0..9,12,13) are all
- * RES1, i.e. they would trap whether we did this write or not.
- */
- WRITE_SYSREG((HCPTR_CP_MASK & ~(HCPTR_CP(10) | HCPTR_CP(11))) |
- HCPTR_TTA | HCPTR_TAM,
- CPTR_EL2);
+ WRITE_SYSREG(cptr_bits, CPTR_EL2);
/*
* Configure HCR_EL2 with the bare minimum to run Xen until a guest
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 02/12] xen/arm: add SVE vector length field to the domain
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 01/12] xen/arm: enable SVE extension for Xen Luca Fancellu
@ 2023-03-27 10:59 ` Luca Fancellu
2023-03-28 9:36 ` Jan Beulich
2023-03-27 10:59 ` [PATCH v4 03/12] xen/arm: Expose SVE feature to the guest Luca Fancellu
` (9 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
Add sve_vl field to arch_domain and xen_arch_domainconfig struct,
to allow the domain to have an information about the SVE feature
and the number of SVE register bits that are allowed for this
domain.
sve_vl field is the vector length in bits divided by 128, this
allows to use less space in the structures.
The field is used also to allow or forbid a domain to use SVE,
because a value equal to zero means the guest is not allowed to
use the feature.
Check that the requested vector length is lower or equal to the
platform supported vector length, otherwise fail on domain
creation.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v3:
- don't use fixed types when not needed, use encoded value also in
arch_domain so rename sve_vl_bits in sve_vl. (Jan)
- rename domainconfig_decode_vl to sve_decode_vl because it will now
be used also to decode from arch_domain value
- change sve_vl from uint16_t to uint8_t and move it after "type" field
to optimize space.
Changes from v2:
- rename field in xen_arch_domainconfig from "sve_vl_bits" to
"sve_vl" and use the implicit padding after gic_version to
store it, now this field is the VL/128. (Jan)
- Created domainconfig_decode_vl() function to decode the sve_vl
field and use it as plain bits value inside arch_domain.
- Changed commit message reflecting the changes
Changes from v1:
- no changes
Changes from RFC:
- restore zcr_el2 in sve_restore_state, that will be introduced
later in this serie, so remove zcr_el2 related code from this
patch and move everything to the later patch (Julien)
- add explicit padding into struct xen_arch_domainconfig (Julien)
- Don't lower down the vector length, just fail to create the
domain. (Julien)
---
xen/arch/arm/arm64/sve.c | 11 +++++++++-
xen/arch/arm/domain.c | 32 ++++++++++++++++++++++++++++
xen/arch/arm/include/asm/arm64/sve.h | 22 +++++++++++++++++--
xen/arch/arm/include/asm/domain.h | 5 +++++
xen/include/public/arch-arm.h | 2 ++
xen/include/public/domctl.h | 2 +-
6 files changed, 70 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c
index c466de61b47f..3c3adfb5c6bd 100644
--- a/xen/arch/arm/arm64/sve.c
+++ b/xen/arch/arm/arm64/sve.c
@@ -6,6 +6,7 @@
*/
#include <xen/types.h>
+#include <asm/cpufeature.h>
#include <asm/arm64/sve.h>
#include <asm/arm64/sysregs.h>
#include <asm/processor.h>
@@ -43,8 +44,16 @@ register_t compute_max_zcr(void)
}
/* Takes a vector length in bits and returns the ZCR_ELx encoding */
-register_t vl_to_zcr(uint16_t vl)
+register_t vl_to_zcr(unsigned int vl)
{
ASSERT(vl > 0);
return ((vl / SVE_VL_MULTIPLE_VAL) - 1U) & ZCR_ELx_LEN_MASK;
}
+
+/* Get the system sanitized value for VL in bits */
+unsigned int get_sys_vl_len(void)
+{
+ /* ZCR_ELx len field is ((len+1) * 128) = vector bits length */
+ return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) *
+ SVE_VL_MULTIPLE_VAL;
+}
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index adb6ace2e24d..7182d4567bf0 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -13,6 +13,7 @@
#include <xen/wait.h>
#include <asm/alternative.h>
+#include <asm/arm64/sve.h>
#include <asm/cpuerrata.h>
#include <asm/cpufeature.h>
#include <asm/current.h>
@@ -550,6 +551,8 @@ int arch_vcpu_create(struct vcpu *v)
v->arch.vmpidr = MPIDR_SMP | vcpuid_to_vaffinity(v->vcpu_id);
v->arch.cptr_el2 = get_default_cptr_flags();
+ if ( is_sve_domain(v->domain) )
+ v->arch.cptr_el2 &= ~HCPTR_CP(8);
v->arch.hcr_el2 = get_default_hcr_flags();
@@ -594,6 +597,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
unsigned int max_vcpus;
unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap);
unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu);
+ unsigned int sve_vl_bits = sve_decode_vl(config->arch.sve_vl);
if ( (config->flags & ~flags_optional) != flags_required )
{
@@ -602,6 +606,31 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
return -EINVAL;
}
+ /* Check feature flags */
+ if ( sve_vl_bits > 0 ) {
+ unsigned int zcr_max_bits = get_sys_vl_len();
+
+ if ( !cpu_has_sve )
+ {
+ dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n");
+ return -EINVAL;
+ }
+ else if ( !is_vl_valid(sve_vl_bits) )
+ {
+ dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n",
+ sve_vl_bits);
+ return -EINVAL;
+ }
+ else if ( sve_vl_bits > zcr_max_bits )
+ {
+ dprintk(XENLOG_INFO,
+ "The requested SVE vector length (%u) must be lower or \n"
+ "equal to the platform supported vector length (%u)\n",
+ sve_vl_bits, zcr_max_bits);
+ return -EINVAL;
+ }
+ }
+
/* The P2M table must always be shared between the CPU and the IOMMU */
if ( config->iommu_opts & XEN_DOMCTL_IOMMU_no_sharept )
{
@@ -744,6 +773,9 @@ int arch_domain_create(struct domain *d,
if ( (rc = domain_vpci_init(d)) != 0 )
goto fail;
+ /* Copy the encoded vector length sve_vl from the domain configuration */
+ d->arch.sve_vl = config->arch.sve_vl;
+
return 0;
fail:
diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h
index bd56e2f24230..8037f09b5b0a 100644
--- a/xen/arch/arm/include/asm/arm64/sve.h
+++ b/xen/arch/arm/include/asm/arm64/sve.h
@@ -13,10 +13,23 @@
/* Vector length must be multiple of 128 */
#define SVE_VL_MULTIPLE_VAL (128U)
+static inline bool is_vl_valid(unsigned int vl)
+{
+ /* SVE vector length is multiple of 128 and maximum 2048 */
+ return ((vl % SVE_VL_MULTIPLE_VAL) == 0) && (vl <= SVE_VL_MAX_BITS);
+}
+
+static inline unsigned int sve_decode_vl(unsigned int sve_vl)
+{
+ /* SVE vector length is stored as VL/128 in xen_arch_domainconfig */
+ return sve_vl * SVE_VL_MULTIPLE_VAL;
+}
+
#ifdef CONFIG_ARM64_SVE
register_t compute_max_zcr(void);
-register_t vl_to_zcr(uint16_t vl);
+register_t vl_to_zcr(unsigned int vl);
+unsigned int get_sys_vl_len(void);
#else /* !CONFIG_ARM64_SVE */
@@ -25,7 +38,12 @@ static inline register_t compute_max_zcr(void)
return 0;
}
-static inline register_t vl_to_zcr(uint16_t vl)
+static inline register_t vl_to_zcr(unsigned int vl)
+{
+ return 0;
+}
+
+static inline unsigned int get_sys_vl_len(void)
{
return 0;
}
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index e776ee704b7d..78cc2da3d4e5 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -31,6 +31,8 @@ enum domain_type {
#define is_domain_direct_mapped(d) ((d)->cdf & CDF_directmap)
+#define is_sve_domain(d) ((d)->arch.sve_vl > 0)
+
/*
* Is the domain using the host memory layout?
*
@@ -67,6 +69,9 @@ struct arch_domain
enum domain_type type;
#endif
+ /* max SVE encoded vector length */
+ uint8_t sve_vl;
+
/* Virtual MMU */
struct p2m_domain p2m;
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 1528ced5097a..38311f559581 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -300,6 +300,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
struct xen_arch_domainconfig {
/* IN/OUT */
uint8_t gic_version;
+ /* IN - Contains SVE vector length divided by 128 */
+ uint8_t sve_vl;
/* IN */
uint16_t tee_type;
/* IN */
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 51be28c3de7c..616d7a1c070d 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -21,7 +21,7 @@
#include "hvm/save.h"
#include "memory.h"
-#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015
+#define XEN_DOMCTL_INTERFACE_VERSION 0x00000016
/*
* NB. xen_domctl.domain is an IN/OUT parameter for this operation.
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 03/12] xen/arm: Expose SVE feature to the guest
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 01/12] xen/arm: enable SVE extension for Xen Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 02/12] xen/arm: add SVE vector length field to the domain Luca Fancellu
@ 2023-03-27 10:59 ` Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 04/12] xen/arm: add SVE exception class handling Luca Fancellu
` (8 subsequent siblings)
11 siblings, 0 replies; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk
When a guest is allowed to use SVE, expose the SVE features through
the identification registers.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v3:
- no changes
Changes from v2:
- no changes
Changes from v1:
- No changes
Changes from RFC:
- No changes
---
xen/arch/arm/arm64/vsysreg.c | 39 ++++++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
index 758750983c11..10048bb4d221 100644
--- a/xen/arch/arm/arm64/vsysreg.c
+++ b/xen/arch/arm/arm64/vsysreg.c
@@ -18,6 +18,7 @@
#include <xen/sched.h>
+#include <asm/arm64/cpufeature.h>
#include <asm/current.h>
#include <asm/regs.h>
#include <asm/traps.h>
@@ -295,7 +296,28 @@ void do_sysreg(struct cpu_user_regs *regs,
GENERATE_TID3_INFO(MVFR0_EL1, mvfr, 0)
GENERATE_TID3_INFO(MVFR1_EL1, mvfr, 1)
GENERATE_TID3_INFO(MVFR2_EL1, mvfr, 2)
- GENERATE_TID3_INFO(ID_AA64PFR0_EL1, pfr64, 0)
+
+ case HSR_SYSREG_ID_AA64PFR0_EL1:
+ {
+ register_t guest_reg_value = guest_cpuinfo.pfr64.bits[0];
+
+ if ( is_sve_domain(v->domain) )
+ {
+ /* 4 is the SVE field width in id_aa64pfr0_el1 */
+ uint64_t mask = GENMASK(ID_AA64PFR0_SVE_SHIFT + 4 - 1,
+ ID_AA64PFR0_SVE_SHIFT);
+ /* sysval is the sve field on the system */
+ uint64_t sysval = cpuid_feature_extract_unsigned_field_width(
+ system_cpuinfo.pfr64.bits[0],
+ ID_AA64PFR0_SVE_SHIFT, 4);
+ guest_reg_value &= ~mask;
+ guest_reg_value |= (sysval << ID_AA64PFR0_SVE_SHIFT) & mask;
+ }
+
+ return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
+ guest_reg_value);
+ }
+
GENERATE_TID3_INFO(ID_AA64PFR1_EL1, pfr64, 1)
GENERATE_TID3_INFO(ID_AA64DFR0_EL1, dbg64, 0)
GENERATE_TID3_INFO(ID_AA64DFR1_EL1, dbg64, 1)
@@ -306,7 +328,20 @@ void do_sysreg(struct cpu_user_regs *regs,
GENERATE_TID3_INFO(ID_AA64MMFR2_EL1, mm64, 2)
GENERATE_TID3_INFO(ID_AA64AFR0_EL1, aux64, 0)
GENERATE_TID3_INFO(ID_AA64AFR1_EL1, aux64, 1)
- GENERATE_TID3_INFO(ID_AA64ZFR0_EL1, zfr64, 0)
+
+ case HSR_SYSREG_ID_AA64ZFR0_EL1:
+ {
+ /*
+ * When the guest has the SVE feature enabled, the whole id_aa64zfr0_el1
+ * needs to be exposed.
+ */
+ register_t guest_reg_value = guest_cpuinfo.zfr64.bits[0];
+ if ( is_sve_domain(v->domain) )
+ guest_reg_value = system_cpuinfo.zfr64.bits[0];
+
+ return handle_ro_read_val(regs, regidx, hsr.sysreg.read, hsr, 1,
+ guest_reg_value);
+ }
/*
* Those cases are catching all Reserved registers trapped by TID3 which
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 04/12] xen/arm: add SVE exception class handling
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
` (2 preceding siblings ...)
2023-03-27 10:59 ` [PATCH v4 03/12] xen/arm: Expose SVE feature to the guest Luca Fancellu
@ 2023-03-27 10:59 ` Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 05/12] arm/sve: save/restore SVE context switch Luca Fancellu
` (7 subsequent siblings)
11 siblings, 0 replies; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk
SVE has a new exception class with code 0x19, introduce the new code
and handle the exception.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v3:
- No changes
Changes from v2:
- No changes
Changes from v1:
- No changes
Changes from RFC:
- No changes
---
xen/arch/arm/include/asm/processor.h | 1 +
xen/arch/arm/traps.c | 12 ++++++++++++
2 files changed, 13 insertions(+)
diff --git a/xen/arch/arm/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h
index bc683334125c..7e42ff8811fc 100644
--- a/xen/arch/arm/include/asm/processor.h
+++ b/xen/arch/arm/include/asm/processor.h
@@ -426,6 +426,7 @@
#define HSR_EC_HVC64 0x16
#define HSR_EC_SMC64 0x17
#define HSR_EC_SYSREG 0x18
+#define HSR_EC_SVE 0x19
#endif
#define HSR_EC_INSTR_ABORT_LOWER_EL 0x20
#define HSR_EC_INSTR_ABORT_CURR_EL 0x21
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index a78a99ddadd0..c2e30feafd5a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2160,6 +2160,13 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
perfc_incr(trap_sysreg);
do_sysreg(regs, hsr);
break;
+ case HSR_EC_SVE:
+ GUEST_BUG_ON(regs_mode_is_32bit(regs));
+ gprintk(XENLOG_WARNING,
+ "Domain id %d tried to use SVE while not allowed\n",
+ current->domain->domain_id);
+ inject_undef_exception(regs, hsr);
+ break;
#endif
case HSR_EC_INSTR_ABORT_LOWER_EL:
@@ -2189,6 +2196,11 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
case HSR_EC_BRK:
do_trap_brk(regs, hsr);
break;
+ case HSR_EC_SVE:
+ /* An SVE exception is a bug somewhere in hypervisor code */
+ printk("SVE trap at EL2.\n");
+ do_unexpected_trap("Hypervisor", regs);
+ break;
#endif
case HSR_EC_DATA_ABORT_CURR_EL:
case HSR_EC_INSTR_ABORT_CURR_EL:
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 05/12] arm/sve: save/restore SVE context switch
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
` (3 preceding siblings ...)
2023-03-27 10:59 ` [PATCH v4 04/12] xen/arm: add SVE exception class handling Luca Fancellu
@ 2023-03-27 10:59 ` Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 06/12] xen/common: add dom0 xen command line argument for Arm Luca Fancellu
` (6 subsequent siblings)
11 siblings, 0 replies; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk
Save/restore context switch for SVE, allocate memory to contain
the Z0-31 registers whose length is maximum 2048 bits each and
FFR who can be maximum 256 bits, the allocated memory depends on
how many bits is the vector length for the domain and how many bits
are supported by the platform.
Save P0-15 whose length is maximum 256 bits each, in this case the
memory used is from the fpregs field in struct vfp_state,
because V0-31 are part of Z0-31 and this space would have been
unused for SVE domain otherwise.
Create zcr_el{1,2} fields in arch_vcpu, initialise zcr_el2 on vcpu
creation given the requested vector length and restore it on
context switch, save/restore ZCR_EL1 value as well.
Remove headers from sve.c that are already included using
xen/sched.h.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v3:
- don't use fixed len types when not needed (Jan)
- now VL is an encoded value, decode it before using.
Changes from v2:
- No changes
Changes from v1:
- No changes
Changes from RFC:
- Moved zcr_el2 field introduction in this patch, restore its
content inside sve_restore_state function. (Julien)
---
xen/arch/arm/arm64/sve-asm.S | 141 +++++++++++++++++++++++
xen/arch/arm/arm64/sve.c | 68 ++++++++++-
xen/arch/arm/arm64/vfp.c | 79 +++++++------
xen/arch/arm/domain.c | 7 ++
xen/arch/arm/include/asm/arm64/sve.h | 13 +++
xen/arch/arm/include/asm/arm64/sysregs.h | 3 +
xen/arch/arm/include/asm/arm64/vfp.h | 10 ++
xen/arch/arm/include/asm/domain.h | 2 +
8 files changed, 284 insertions(+), 39 deletions(-)
diff --git a/xen/arch/arm/arm64/sve-asm.S b/xen/arch/arm/arm64/sve-asm.S
index 4d1549344733..8c37d7bc95d5 100644
--- a/xen/arch/arm/arm64/sve-asm.S
+++ b/xen/arch/arm/arm64/sve-asm.S
@@ -17,6 +17,18 @@
.endif
.endm
+.macro _sve_check_zreg znr
+ .if (\znr) < 0 || (\znr) > 31
+ .error "Bad Scalable Vector Extension vector register number \znr."
+ .endif
+.endm
+
+.macro _sve_check_preg pnr
+ .if (\pnr) < 0 || (\pnr) > 15
+ .error "Bad Scalable Vector Extension predicate register number \pnr."
+ .endif
+.endm
+
.macro _check_num n, min, max
.if (\n) < (\min) || (\n) > (\max)
.error "Number \n out of range [\min,\max]"
@@ -26,6 +38,54 @@
/* SVE instruction encodings for non-SVE-capable assemblers */
/* (pre binutils 2.28, all kernel capable clang versions support SVE) */
+/* STR (vector): STR Z\nz, [X\nxbase, #\offset, MUL VL] */
+.macro _sve_str_v nz, nxbase, offset=0
+ _sve_check_zreg \nz
+ _check_general_reg \nxbase
+ _check_num (\offset), -0x100, 0xff
+ .inst 0xe5804000 \
+ | (\nz) \
+ | ((\nxbase) << 5) \
+ | (((\offset) & 7) << 10) \
+ | (((\offset) & 0x1f8) << 13)
+.endm
+
+/* LDR (vector): LDR Z\nz, [X\nxbase, #\offset, MUL VL] */
+.macro _sve_ldr_v nz, nxbase, offset=0
+ _sve_check_zreg \nz
+ _check_general_reg \nxbase
+ _check_num (\offset), -0x100, 0xff
+ .inst 0x85804000 \
+ | (\nz) \
+ | ((\nxbase) << 5) \
+ | (((\offset) & 7) << 10) \
+ | (((\offset) & 0x1f8) << 13)
+.endm
+
+/* STR (predicate): STR P\np, [X\nxbase, #\offset, MUL VL] */
+.macro _sve_str_p np, nxbase, offset=0
+ _sve_check_preg \np
+ _check_general_reg \nxbase
+ _check_num (\offset), -0x100, 0xff
+ .inst 0xe5800000 \
+ | (\np) \
+ | ((\nxbase) << 5) \
+ | (((\offset) & 7) << 10) \
+ | (((\offset) & 0x1f8) << 13)
+.endm
+
+/* LDR (predicate): LDR P\np, [X\nxbase, #\offset, MUL VL] */
+.macro _sve_ldr_p np, nxbase, offset=0
+ _sve_check_preg \np
+ _check_general_reg \nxbase
+ _check_num (\offset), -0x100, 0xff
+ .inst 0x85800000 \
+ | (\np) \
+ | ((\nxbase) << 5) \
+ | (((\offset) & 7) << 10) \
+ | (((\offset) & 0x1f8) << 13)
+.endm
+
/* RDVL X\nx, #\imm */
.macro _sve_rdvl nx, imm
_check_general_reg \nx
@@ -35,11 +95,92 @@
| (((\imm) & 0x3f) << 5)
.endm
+/* RDFFR (unpredicated): RDFFR P\np.B */
+.macro _sve_rdffr np
+ _sve_check_preg \np
+ .inst 0x2519f000 \
+ | (\np)
+.endm
+
+/* WRFFR P\np.B */
+.macro _sve_wrffr np
+ _sve_check_preg \np
+ .inst 0x25289000 \
+ | ((\np) << 5)
+.endm
+
+.macro __for from:req, to:req
+ .if (\from) == (\to)
+ _for__body %\from
+ .else
+ __for %\from, %((\from) + ((\to) - (\from)) / 2)
+ __for %((\from) + ((\to) - (\from)) / 2 + 1), %\to
+ .endif
+.endm
+
+.macro _for var:req, from:req, to:req, insn:vararg
+ .macro _for__body \var:req
+ .noaltmacro
+ \insn
+ .altmacro
+ .endm
+
+ .altmacro
+ __for \from, \to
+ .noaltmacro
+
+ .purgem _for__body
+.endm
+
+.macro sve_save nxzffrctx, nxpctx, save_ffr
+ _for n, 0, 31, _sve_str_v \n, \nxzffrctx, \n - 32
+ _for n, 0, 15, _sve_str_p \n, \nxpctx, \n
+ cbz \save_ffr, 1f
+ _sve_rdffr 0
+ _sve_str_p 0, \nxzffrctx
+ _sve_ldr_p 0, \nxpctx
+ b 2f
+1:
+ str xzr, [x\nxzffrctx] // Zero out FFR
+2:
+.endm
+
+.macro sve_load nxzffrctx, nxpctx, restore_ffr
+ _for n, 0, 31, _sve_ldr_v \n, \nxzffrctx, \n - 32
+ cbz \restore_ffr, 1f
+ _sve_ldr_p 0, \nxzffrctx
+ _sve_wrffr 0
+1:
+ _for n, 0, 15, _sve_ldr_p \n, \nxpctx, \n
+.endm
+
/* Gets the current vector register size in bytes */
GLOBAL(sve_get_hw_vl)
_sve_rdvl 0, 1
ret
+/*
+ * Save the SVE context
+ *
+ * x0 - pointer to buffer for Z0-31 + FFR
+ * x1 - pointer to buffer for P0-15
+ * x2 - Save FFR if non-zero
+ */
+GLOBAL(sve_save_ctx)
+ sve_save 0, 1, x2
+ ret
+
+/*
+ * Load the SVE context
+ *
+ * x0 - pointer to buffer for Z0-31 + FFR
+ * x1 - pointer to buffer for P0-15
+ * x2 - Restore FFR if non-zero
+ */
+GLOBAL(sve_load_ctx)
+ sve_load 0, 1, x2
+ ret
+
/*
* Local variables:
* mode: ASM
diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c
index 3c3adfb5c6bd..696a97811cac 100644
--- a/xen/arch/arm/arm64/sve.c
+++ b/xen/arch/arm/arm64/sve.c
@@ -5,14 +5,29 @@
* Copyright (C) 2022 ARM Ltd.
*/
-#include <xen/types.h>
-#include <asm/cpufeature.h>
+#include <xen/sched.h>
+#include <xen/sizes.h>
#include <asm/arm64/sve.h>
-#include <asm/arm64/sysregs.h>
-#include <asm/processor.h>
-#include <asm/system.h>
extern unsigned int sve_get_hw_vl(void);
+extern void sve_save_ctx(uint64_t *sve_ctx, uint64_t *pregs, int save_ffr);
+extern void sve_load_ctx(uint64_t const *sve_ctx, uint64_t const *pregs,
+ int restore_ffr);
+
+static inline unsigned int sve_zreg_ctx_size(unsigned int vl)
+{
+ /*
+ * Z0-31 registers size in bytes is computed from VL that is in bits, so VL
+ * in bytes is VL/8.
+ */
+ return (vl / 8U) * 32U;
+}
+
+static inline unsigned int sve_ffrreg_ctx_size(unsigned int vl)
+{
+ /* FFR register size is VL/8, which is in bytes (VL/8)/8 */
+ return (vl / 64U);
+}
register_t compute_max_zcr(void)
{
@@ -57,3 +72,46 @@ unsigned int get_sys_vl_len(void)
return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) *
SVE_VL_MULTIPLE_VAL;
}
+
+int sve_context_init(struct vcpu *v)
+{
+ unsigned int sve_vl_bits = sve_decode_vl(v->domain->arch.sve_vl);
+ uint64_t *ctx = _xzalloc(sve_zreg_ctx_size(sve_vl_bits) +
+ sve_ffrreg_ctx_size(sve_vl_bits),
+ L1_CACHE_BYTES);
+
+ if ( !ctx )
+ return -ENOMEM;
+
+ v->arch.vfp.sve_context = ctx;
+
+ return 0;
+}
+
+void sve_context_free(struct vcpu *v)
+{
+ xfree(v->arch.vfp.sve_context);
+}
+
+void sve_save_state(struct vcpu *v)
+{
+ unsigned int sve_vl_bits = sve_decode_vl(v->domain->arch.sve_vl);
+ uint64_t *sve_ctx_zreg_end = v->arch.vfp.sve_context +
+ (sve_zreg_ctx_size(sve_vl_bits) / sizeof(uint64_t));
+
+ v->arch.zcr_el1 = READ_SYSREG(ZCR_EL1);
+
+ sve_save_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1);
+}
+
+void sve_restore_state(struct vcpu *v)
+{
+ unsigned int sve_vl_bits = sve_decode_vl(v->domain->arch.sve_vl);
+ uint64_t *sve_ctx_zreg_end = v->arch.vfp.sve_context +
+ (sve_zreg_ctx_size(sve_vl_bits) / sizeof(uint64_t));
+
+ WRITE_SYSREG(v->arch.zcr_el1, ZCR_EL1);
+ WRITE_SYSREG(v->arch.zcr_el2, ZCR_EL2);
+
+ sve_load_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1);
+}
diff --git a/xen/arch/arm/arm64/vfp.c b/xen/arch/arm/arm64/vfp.c
index 47885e76baae..2d0d7c2e6ddb 100644
--- a/xen/arch/arm/arm64/vfp.c
+++ b/xen/arch/arm/arm64/vfp.c
@@ -2,29 +2,35 @@
#include <asm/processor.h>
#include <asm/cpufeature.h>
#include <asm/vfp.h>
+#include <asm/arm64/sve.h>
void vfp_save_state(struct vcpu *v)
{
if ( !cpu_has_fp )
return;
- asm volatile("stp q0, q1, [%1, #16 * 0]\n\t"
- "stp q2, q3, [%1, #16 * 2]\n\t"
- "stp q4, q5, [%1, #16 * 4]\n\t"
- "stp q6, q7, [%1, #16 * 6]\n\t"
- "stp q8, q9, [%1, #16 * 8]\n\t"
- "stp q10, q11, [%1, #16 * 10]\n\t"
- "stp q12, q13, [%1, #16 * 12]\n\t"
- "stp q14, q15, [%1, #16 * 14]\n\t"
- "stp q16, q17, [%1, #16 * 16]\n\t"
- "stp q18, q19, [%1, #16 * 18]\n\t"
- "stp q20, q21, [%1, #16 * 20]\n\t"
- "stp q22, q23, [%1, #16 * 22]\n\t"
- "stp q24, q25, [%1, #16 * 24]\n\t"
- "stp q26, q27, [%1, #16 * 26]\n\t"
- "stp q28, q29, [%1, #16 * 28]\n\t"
- "stp q30, q31, [%1, #16 * 30]\n\t"
- : "=Q" (*v->arch.vfp.fpregs) : "r" (v->arch.vfp.fpregs));
+ if ( is_sve_domain(v->domain) )
+ sve_save_state(v);
+ else
+ {
+ asm volatile("stp q0, q1, [%1, #16 * 0]\n\t"
+ "stp q2, q3, [%1, #16 * 2]\n\t"
+ "stp q4, q5, [%1, #16 * 4]\n\t"
+ "stp q6, q7, [%1, #16 * 6]\n\t"
+ "stp q8, q9, [%1, #16 * 8]\n\t"
+ "stp q10, q11, [%1, #16 * 10]\n\t"
+ "stp q12, q13, [%1, #16 * 12]\n\t"
+ "stp q14, q15, [%1, #16 * 14]\n\t"
+ "stp q16, q17, [%1, #16 * 16]\n\t"
+ "stp q18, q19, [%1, #16 * 18]\n\t"
+ "stp q20, q21, [%1, #16 * 20]\n\t"
+ "stp q22, q23, [%1, #16 * 22]\n\t"
+ "stp q24, q25, [%1, #16 * 24]\n\t"
+ "stp q26, q27, [%1, #16 * 26]\n\t"
+ "stp q28, q29, [%1, #16 * 28]\n\t"
+ "stp q30, q31, [%1, #16 * 30]\n\t"
+ : "=Q" (*v->arch.vfp.fpregs) : "r" (v->arch.vfp.fpregs));
+ }
v->arch.vfp.fpsr = READ_SYSREG(FPSR);
v->arch.vfp.fpcr = READ_SYSREG(FPCR);
@@ -37,23 +43,28 @@ void vfp_restore_state(struct vcpu *v)
if ( !cpu_has_fp )
return;
- asm volatile("ldp q0, q1, [%1, #16 * 0]\n\t"
- "ldp q2, q3, [%1, #16 * 2]\n\t"
- "ldp q4, q5, [%1, #16 * 4]\n\t"
- "ldp q6, q7, [%1, #16 * 6]\n\t"
- "ldp q8, q9, [%1, #16 * 8]\n\t"
- "ldp q10, q11, [%1, #16 * 10]\n\t"
- "ldp q12, q13, [%1, #16 * 12]\n\t"
- "ldp q14, q15, [%1, #16 * 14]\n\t"
- "ldp q16, q17, [%1, #16 * 16]\n\t"
- "ldp q18, q19, [%1, #16 * 18]\n\t"
- "ldp q20, q21, [%1, #16 * 20]\n\t"
- "ldp q22, q23, [%1, #16 * 22]\n\t"
- "ldp q24, q25, [%1, #16 * 24]\n\t"
- "ldp q26, q27, [%1, #16 * 26]\n\t"
- "ldp q28, q29, [%1, #16 * 28]\n\t"
- "ldp q30, q31, [%1, #16 * 30]\n\t"
- : : "Q" (*v->arch.vfp.fpregs), "r" (v->arch.vfp.fpregs));
+ if ( is_sve_domain(v->domain) )
+ sve_restore_state(v);
+ else
+ {
+ asm volatile("ldp q0, q1, [%1, #16 * 0]\n\t"
+ "ldp q2, q3, [%1, #16 * 2]\n\t"
+ "ldp q4, q5, [%1, #16 * 4]\n\t"
+ "ldp q6, q7, [%1, #16 * 6]\n\t"
+ "ldp q8, q9, [%1, #16 * 8]\n\t"
+ "ldp q10, q11, [%1, #16 * 10]\n\t"
+ "ldp q12, q13, [%1, #16 * 12]\n\t"
+ "ldp q14, q15, [%1, #16 * 14]\n\t"
+ "ldp q16, q17, [%1, #16 * 16]\n\t"
+ "ldp q18, q19, [%1, #16 * 18]\n\t"
+ "ldp q20, q21, [%1, #16 * 20]\n\t"
+ "ldp q22, q23, [%1, #16 * 22]\n\t"
+ "ldp q24, q25, [%1, #16 * 24]\n\t"
+ "ldp q26, q27, [%1, #16 * 26]\n\t"
+ "ldp q28, q29, [%1, #16 * 28]\n\t"
+ "ldp q30, q31, [%1, #16 * 30]\n\t"
+ : : "Q" (*v->arch.vfp.fpregs), "r" (v->arch.vfp.fpregs));
+ }
WRITE_SYSREG(v->arch.vfp.fpsr, FPSR);
WRITE_SYSREG(v->arch.vfp.fpcr, FPCR);
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 7182d4567bf0..8326ef15d159 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -552,7 +552,12 @@ int arch_vcpu_create(struct vcpu *v)
v->arch.cptr_el2 = get_default_cptr_flags();
if ( is_sve_domain(v->domain) )
+ {
+ if ( (rc = sve_context_init(v)) != 0 )
+ goto fail;
v->arch.cptr_el2 &= ~HCPTR_CP(8);
+ v->arch.zcr_el2 = vl_to_zcr(sve_decode_vl(v->domain->arch.sve_vl));
+ }
v->arch.hcr_el2 = get_default_hcr_flags();
@@ -582,6 +587,8 @@ fail:
void arch_vcpu_destroy(struct vcpu *v)
{
+ if ( is_sve_domain(v->domain) )
+ sve_context_free(v);
vcpu_timer_destroy(v);
vcpu_vgic_free(v);
free_xenheap_pages(v->arch.stack, STACK_ORDER);
diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h
index 8037f09b5b0a..d38a37408439 100644
--- a/xen/arch/arm/include/asm/arm64/sve.h
+++ b/xen/arch/arm/include/asm/arm64/sve.h
@@ -30,6 +30,10 @@ static inline unsigned int sve_decode_vl(unsigned int sve_vl)
register_t compute_max_zcr(void);
register_t vl_to_zcr(unsigned int vl);
unsigned int get_sys_vl_len(void);
+int sve_context_init(struct vcpu *v);
+void sve_context_free(struct vcpu *v);
+void sve_save_state(struct vcpu *v);
+void sve_restore_state(struct vcpu *v);
#else /* !CONFIG_ARM64_SVE */
@@ -48,6 +52,15 @@ static inline unsigned int get_sys_vl_len(void)
return 0;
}
+static inline int sve_context_init(struct vcpu *v)
+{
+ return 0;
+}
+
+static inline void sve_context_free(struct vcpu *v) {}
+static inline void sve_save_state(struct vcpu *v) {}
+static inline void sve_restore_state(struct vcpu *v) {}
+
#endif
#endif /* _ARM_ARM64_SVE_H */
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
index 4cabb9eb4d5e..3fdeb9d8cdef 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -88,6 +88,9 @@
#ifndef ID_AA64ISAR2_EL1
#define ID_AA64ISAR2_EL1 S3_0_C0_C6_2
#endif
+#ifndef ZCR_EL1
+#define ZCR_EL1 S3_0_C1_C2_0
+#endif
/* ID registers (imported from arm64/include/asm/sysreg.h in Linux) */
diff --git a/xen/arch/arm/include/asm/arm64/vfp.h b/xen/arch/arm/include/asm/arm64/vfp.h
index e6e8c363bc16..8af714cb8ecc 100644
--- a/xen/arch/arm/include/asm/arm64/vfp.h
+++ b/xen/arch/arm/include/asm/arm64/vfp.h
@@ -6,7 +6,17 @@
struct vfp_state
{
+ /*
+ * When SVE is enabled for the guest, fpregs memory will be used to
+ * save/restore P0-P15 registers, otherwise it will be used for the V0-V31
+ * registers.
+ */
uint64_t fpregs[64] __vfp_aligned;
+ /*
+ * When SVE is enabled for the guest, sve_context contains memory to
+ * save/restore Z0-Z31 registers and FFR.
+ */
+ uint64_t *sve_context;
register_t fpcr;
register_t fpexc32_el2;
register_t fpsr;
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index 78cc2da3d4e5..6b5ec3bd0680 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -195,6 +195,8 @@ struct arch_vcpu
register_t tpidrro_el0;
/* HYP configuration */
+ register_t zcr_el1;
+ register_t zcr_el2;
register_t cptr_el2;
register_t hcr_el2;
register_t mdcr_el2;
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 06/12] xen/common: add dom0 xen command line argument for Arm
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
` (4 preceding siblings ...)
2023-03-27 10:59 ` [PATCH v4 05/12] arm/sve: save/restore SVE context switch Luca Fancellu
@ 2023-03-27 10:59 ` Luca Fancellu
2023-03-28 9:44 ` Jan Beulich
2023-03-27 10:59 ` [PATCH v4 07/12] xen: enable Dom0 to use SVE feature Luca Fancellu
` (5 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu, Roger Pau Monné
Currently x86 defines a Xen command line argument dom0=<list> where
there can be specified dom0 controlling sub-options, to use it also
on Arm, move the code that loops through the list of arguments from
x86 to the common code and from there, call architecture specific
functions to handle the comma separated sub-options.
No functional changes are intended.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v3:
- new patch
---
xen/arch/arm/domain_build.c | 5 ++++
xen/arch/x86/dom0_build.c | 46 ++++++++++++++-----------------------
xen/common/domain.c | 23 +++++++++++++++++++
xen/include/xen/domain.h | 1 +
4 files changed, 46 insertions(+), 29 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9707eb7b1bb1..35dbe964fc8b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -59,6 +59,11 @@ static int __init parse_dom0_mem(const char *s)
}
custom_param("dom0_mem", parse_dom0_mem);
+int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
+{
+ return -1;
+}
+
/* Override macros from asm/page.h to make them work with mfn_t */
#undef virt_to_mfn
#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 79234f18ff01..5e143814c854 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -266,42 +266,30 @@ bool __initdata opt_dom0_pvh = !IS_ENABLED(CONFIG_PV);
bool __initdata opt_dom0_verbose = IS_ENABLED(CONFIG_VERBOSE_DEBUG);
bool __initdata opt_dom0_msr_relaxed;
-static int __init cf_check parse_dom0_param(const char *s)
+int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
{
- const char *ss;
- int rc = 0;
-
- do {
- int val;
-
- ss = strchr(s, ',');
- if ( !ss )
- ss = strchr(s, '\0');
+ int val, rc = 0;
- if ( IS_ENABLED(CONFIG_PV) && !cmdline_strcmp(s, "pv") )
- opt_dom0_pvh = false;
- else if ( IS_ENABLED(CONFIG_HVM) && !cmdline_strcmp(s, "pvh") )
- opt_dom0_pvh = true;
+ if ( IS_ENABLED(CONFIG_PV) && !cmdline_strcmp(str_begin, "pv") )
+ opt_dom0_pvh = false;
+ else if ( IS_ENABLED(CONFIG_HVM) && !cmdline_strcmp(str_begin, "pvh") )
+ opt_dom0_pvh = true;
#ifdef CONFIG_SHADOW_PAGING
- else if ( (val = parse_boolean("shadow", s, ss)) >= 0 )
- opt_dom0_shadow = val;
+ else if ( (val = parse_boolean("shadow", str_begin, str_end)) >= 0 )
+ opt_dom0_shadow = val;
#endif
- else if ( (val = parse_boolean("verbose", s, ss)) >= 0 )
- opt_dom0_verbose = val;
- else if ( IS_ENABLED(CONFIG_PV) &&
- (val = parse_boolean("cpuid-faulting", s, ss)) >= 0 )
- opt_dom0_cpuid_faulting = val;
- else if ( (val = parse_boolean("msr-relaxed", s, ss)) >= 0 )
- opt_dom0_msr_relaxed = val;
- else
- rc = -EINVAL;
-
- s = ss + 1;
- } while ( *ss );
+ else if ( (val = parse_boolean("verbose", str_begin, str_end)) >= 0 )
+ opt_dom0_verbose = val;
+ else if ( IS_ENABLED(CONFIG_PV) &&
+ (val = parse_boolean("cpuid-faulting", str_begin, str_end)) >= 0 )
+ opt_dom0_cpuid_faulting = val;
+ else if ( (val = parse_boolean("msr-relaxed", str_begin, str_end)) >= 0 )
+ opt_dom0_msr_relaxed = val;
+ else
+ rc = -EINVAL;
return rc;
}
-custom_param("dom0", parse_dom0_param);
static char __initdata opt_dom0_ioports_disable[200] = "";
string_param("dom0_ioports_disable", opt_dom0_ioports_disable);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 626debbae095..7779ba088675 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -364,6 +364,29 @@ static int __init cf_check parse_extra_guest_irqs(const char *s)
}
custom_param("extra_guest_irqs", parse_extra_guest_irqs);
+static int __init cf_check parse_dom0_param(const char *s)
+{
+ const char *ss;
+ int rc = 0;
+
+ do {
+ int ret;
+
+ ss = strchr(s, ',');
+ if ( !ss )
+ ss = strchr(s, '\0');
+
+ ret = parse_arch_dom0_param(s, ss);
+ if ( ret && !rc )
+ rc = ret;
+
+ s = ss + 1;
+ } while ( *ss );
+
+ return rc;
+}
+custom_param("dom0", parse_dom0_param);
+
/*
* Release resources held by a domain. There may or may not be live
* references to the domain, and it may or may not be fully constructed.
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 26f9c4f6dd5b..b1259a3392b2 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -16,6 +16,7 @@ typedef union {
struct vcpu *vcpu_create(struct domain *d, unsigned int vcpu_id);
unsigned int dom0_max_vcpus(void);
+int parse_arch_dom0_param(const char *str_begin, const char *str_end);
struct vcpu *alloc_dom0_vcpu0(struct domain *dom0);
int vcpu_reset(struct vcpu *);
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 07/12] xen: enable Dom0 to use SVE feature
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
` (5 preceding siblings ...)
2023-03-27 10:59 ` [PATCH v4 06/12] xen/common: add dom0 xen command line argument for Arm Luca Fancellu
@ 2023-03-27 10:59 ` Luca Fancellu
2023-03-28 10:08 ` Jan Beulich
2023-03-27 10:59 ` [PATCH v4 08/12] xen/physinfo: encode Arm SVE vector length in arch_capabilities Luca Fancellu
` (4 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu,
Volodymyr Babchuk
Add a command line parameter to allow Dom0 the use of SVE resources,
the command line parameter sve=<integer>, sub argument of dom0=,
controls the feature on this domain and sets the maximum SVE vector
length for Dom0.
Add a new function, parse_integer(), to parse an integer command
line argument.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v3:
- Don't use fixed len types when not needed (Jan)
- renamed domainconfig_encode_vl to sve_encode_vl
- Use a sub argument of dom0= to enable the feature (Jan)
- Add parse_integer() function
Changes from v2:
- xen_domctl_createdomain field has changed into sve_vl and its
value now is the VL / 128, create an helper function for that.
Changes from v1:
- No changes
Changes from RFC:
- Changed docs to explain that the domain won't be created if the
requested vector length is above the supported one from the
platform.
---
docs/misc/xen-command-line.pandoc | 16 ++++++++++++++--
xen/arch/arm/arm64/sve.c | 9 +++++++++
xen/arch/arm/domain_build.c | 11 ++++++++++-
xen/arch/arm/include/asm/arm64/sve.h | 16 ++++++++++++++++
xen/common/kernel.c | 24 ++++++++++++++++++++++++
xen/include/xen/lib.h | 10 ++++++++++
6 files changed, 83 insertions(+), 3 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index e0b89b7d3319..06c1eb4e6d6f 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -777,9 +777,9 @@ Specify the bit width of the DMA heap.
### dom0
= List of [ pv | pvh, shadow=<bool>, verbose=<bool>,
- cpuid-faulting=<bool>, msr-relaxed=<bool> ]
+ cpuid-faulting=<bool>, msr-relaxed=<bool> ] (x86)
- Applicability: x86
+ = List of [ sve=<integer> ] (Arm)
Controls for how dom0 is constructed on x86 systems.
@@ -838,6 +838,18 @@ Controls for how dom0 is constructed on x86 systems.
If using this option is necessary to fix an issue, please report a bug.
+Enables features on dom0 on Arm systems.
+
+* The `sve` integer parameter enables Arm SVE usage for Dom0 domain and sets
+ the maximum SVE vector length.
+ Values above 0 means feature is enabled for Dom0, otherwise feature is
+ disabled.
+ Possible values are from 0 to maximum 2048, being multiple of 128, that will
+ be the maximum vector length.
+ Please note that the platform can supports a lower value, if the requested
+ value is above the supported one, the domain creation will fail and the
+ system will stop.
+
### dom0-cpuid
= List of comma separated booleans
diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c
index 696a97811cac..6416403817e3 100644
--- a/xen/arch/arm/arm64/sve.c
+++ b/xen/arch/arm/arm64/sve.c
@@ -5,10 +5,14 @@
* Copyright (C) 2022 ARM Ltd.
*/
+#include <xen/param.h>
#include <xen/sched.h>
#include <xen/sizes.h>
#include <asm/arm64/sve.h>
+/* opt_dom0_sve: allow Dom0 to use SVE and set maximum vector length. */
+unsigned int __initdata opt_dom0_sve;
+
extern unsigned int sve_get_hw_vl(void);
extern void sve_save_ctx(uint64_t *sve_ctx, uint64_t *pregs, int save_ffr);
extern void sve_load_ctx(uint64_t const *sve_ctx, uint64_t const *pregs,
@@ -115,3 +119,8 @@ void sve_restore_state(struct vcpu *v)
sve_load_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1);
}
+
+int __init sve_parse_dom0_param(const char *str_begin, const char *str_end)
+{
+ return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve);
+}
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 35dbe964fc8b..f6019ce30149 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -26,6 +26,7 @@
#include <asm/platform.h>
#include <asm/psci.h>
#include <asm/setup.h>
+#include <asm/arm64/sve.h>
#include <asm/cpufeature.h>
#include <asm/domain_build.h>
#include <xen/event.h>
@@ -61,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem);
int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
{
- return -1;
+ int rc = 0;
+
+ if ( sve_parse_dom0_param(str_begin, str_end) < 0 )
+ rc = -EINVAL;
+
+ return rc;
}
/* Override macros from asm/page.h to make them work with mfn_t */
@@ -4089,6 +4095,9 @@ void __init create_dom0(void)
if ( iommu_enabled )
dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
+ if ( opt_dom0_sve > 0 )
+ dom0_cfg.arch.sve_vl = sve_encode_vl(opt_dom0_sve);
+
dom0 = domain_create(0, &dom0_cfg, CDF_privileged | CDF_directmap);
if ( IS_ERR(dom0) )
panic("Error creating domain 0 (rc = %ld)\n", PTR_ERR(dom0));
diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h
index d38a37408439..69a1044c37d9 100644
--- a/xen/arch/arm/include/asm/arm64/sve.h
+++ b/xen/arch/arm/include/asm/arm64/sve.h
@@ -25,8 +25,15 @@ static inline unsigned int sve_decode_vl(unsigned int sve_vl)
return sve_vl * SVE_VL_MULTIPLE_VAL;
}
+static inline unsigned int sve_encode_vl(unsigned int sve_vl_bits)
+{
+ return sve_vl_bits / SVE_VL_MULTIPLE_VAL;
+}
+
#ifdef CONFIG_ARM64_SVE
+extern unsigned int opt_dom0_sve;
+
register_t compute_max_zcr(void);
register_t vl_to_zcr(unsigned int vl);
unsigned int get_sys_vl_len(void);
@@ -34,9 +41,12 @@ int sve_context_init(struct vcpu *v);
void sve_context_free(struct vcpu *v);
void sve_save_state(struct vcpu *v);
void sve_restore_state(struct vcpu *v);
+int sve_parse_dom0_param(const char *str_begin, const char *str_end);
#else /* !CONFIG_ARM64_SVE */
+#define opt_dom0_sve (0)
+
static inline register_t compute_max_zcr(void)
{
return 0;
@@ -61,6 +71,12 @@ static inline void sve_context_free(struct vcpu *v) {}
static inline void sve_save_state(struct vcpu *v) {}
static inline void sve_restore_state(struct vcpu *v) {}
+static inline int sve_parse_dom0_param(const char *str_begin,
+ const char *str_end)
+{
+ return -1;
+}
+
#endif
#endif /* _ARM_ARM64_SVE_H */
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index f7b1f65f373c..97b460f5a5c2 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -314,6 +314,30 @@ int parse_boolean(const char *name, const char *s, const char *e)
return -1;
}
+int parse_integer(const char *name, const char *s, const char *e,
+ int *val)
+{
+ size_t slen, nlen;
+ const char *str;
+ long long pval;
+
+ slen = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s);
+ nlen = strlen(name);
+
+ /* Does s start with name or contains only the name? */
+ if ( (slen <= nlen) || strncmp(s, name, nlen) || (s[nlen] != '=') )
+ return -1;
+
+ pval = simple_strtoll(&s[nlen + 1], &str, 0);
+
+ if ( (str != e) || (pval < INT_MIN) || (pval > INT_MAX) )
+ return -2;
+
+ *val = pval;
+
+ return 0;
+}
+
int cmdline_strcmp(const char *frag, const char *name)
{
for ( ; ; frag++, name++ )
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 05ee1e18af6b..900f1257acb4 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -94,6 +94,16 @@ int parse_bool(const char *s, const char *e);
*/
int parse_boolean(const char *name, const char *s, const char *e);
+/**
+ * Given a specific name, parses a string of the form:
+ * $NAME[=...]
+ * returning 0 and a value in val, for a recognised integer.
+ * Returns -1 for name not found, general errors, or -2 if name is found but
+ * not recognised/overflow/underflow value.
+ */
+int parse_integer(const char *name, const char *s, const char *e,
+ int *val);
+
/**
* Very similar to strcmp(), but will declare a match if the NUL in 'name'
* lines up with comma, colon, semicolon or equals in 'frag'. Designed for
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 08/12] xen/physinfo: encode Arm SVE vector length in arch_capabilities
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
` (6 preceding siblings ...)
2023-03-27 10:59 ` [PATCH v4 07/12] xen: enable Dom0 to use SVE feature Luca Fancellu
@ 2023-03-27 10:59 ` Luca Fancellu
2023-03-28 10:14 ` Jan Beulich
2023-03-27 10:59 ` [PATCH v4 09/12] tools: add physinfo arch_capabilities handling for Arm Luca Fancellu
` (3 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
When the arm platform supports SVE, advertise the feature in the
field arch_capabilities in struct xen_sysctl_physinfo by encoding
the SVE vector length in it.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v3:
- domainconfig_encode_vl is now named sve_encode_vl
Changes from v2:
- Remove XEN_SYSCTL_PHYSCAP_ARM_SVE_SHFT, use MASK_INSR and
protect with ifdef XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK (Jan)
- Use the helper function sve_arch_cap_physinfo to encode
the VL into physinfo arch_capabilities field.
Changes from v1:
- Use only arch_capabilities and some defines to encode SVE VL
(Bertrand, Stefano, Jan)
Changes from RFC:
- new patch
---
xen/arch/arm/arm64/sve.c | 12 ++++++++++++
xen/arch/arm/include/asm/arm64/sve.h | 2 ++
xen/arch/arm/sysctl.c | 3 +++
xen/include/public/sysctl.h | 4 ++++
4 files changed, 21 insertions(+)
diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c
index 6416403817e3..7b5223117e1b 100644
--- a/xen/arch/arm/arm64/sve.c
+++ b/xen/arch/arm/arm64/sve.c
@@ -124,3 +124,15 @@ int __init sve_parse_dom0_param(const char *str_begin, const char *str_end)
{
return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve);
}
+
+void sve_arch_cap_physinfo(uint32_t *arch_capabilities)
+{
+ if ( cpu_has_sve )
+ {
+ /* Vector length is divided by 128 to save some space */
+ uint32_t sve_vl = MASK_INSR(sve_encode_vl(get_sys_vl_len()),
+ XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
+
+ *arch_capabilities |= sve_vl;
+ }
+}
diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h
index 69a1044c37d9..a6d0fc851f68 100644
--- a/xen/arch/arm/include/asm/arm64/sve.h
+++ b/xen/arch/arm/include/asm/arm64/sve.h
@@ -42,6 +42,7 @@ void sve_context_free(struct vcpu *v);
void sve_save_state(struct vcpu *v);
void sve_restore_state(struct vcpu *v);
int sve_parse_dom0_param(const char *str_begin, const char *str_end);
+void sve_arch_cap_physinfo(uint32_t *arch_capabilities);
#else /* !CONFIG_ARM64_SVE */
@@ -70,6 +71,7 @@ static inline int sve_context_init(struct vcpu *v)
static inline void sve_context_free(struct vcpu *v) {}
static inline void sve_save_state(struct vcpu *v) {}
static inline void sve_restore_state(struct vcpu *v) {}
+static inline void sve_arch_cap_physinfo(uint32_t *arch_capabilities) {}
static inline int sve_parse_dom0_param(const char *str_begin,
const char *str_end)
diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
index b0a78a8b10d0..64e4d3e06a6b 100644
--- a/xen/arch/arm/sysctl.c
+++ b/xen/arch/arm/sysctl.c
@@ -11,11 +11,14 @@
#include <xen/lib.h>
#include <xen/errno.h>
#include <xen/hypercall.h>
+#include <asm/arm64/sve.h>
#include <public/sysctl.h>
void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
{
pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
+
+ sve_arch_cap_physinfo(&pi->arch_capabilities);
}
long arch_do_sysctl(struct xen_sysctl *sysctl,
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index e8dded9fb94a..99ea3fa0740b 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -94,6 +94,10 @@ struct xen_sysctl_tbuf_op {
/* Max XEN_SYSCTL_PHYSCAP_* constant. Used for ABI checking. */
#define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_gnttab_v2
+#ifdef __aarch64__
+#define XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK (0x1FU)
+#endif
+
struct xen_sysctl_physinfo {
uint32_t threads_per_core;
uint32_t cores_per_socket;
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 09/12] tools: add physinfo arch_capabilities handling for Arm
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
` (7 preceding siblings ...)
2023-03-27 10:59 ` [PATCH v4 08/12] xen/physinfo: encode Arm SVE vector length in arch_capabilities Luca Fancellu
@ 2023-03-27 10:59 ` Luca Fancellu
2023-03-30 16:49 ` Anthony PERARD
2023-03-27 10:59 ` [PATCH v4 10/12] xen/tools: add sve parameter in XL configuration Luca Fancellu
` (2 subsequent siblings)
11 siblings, 1 reply; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, George Dunlap, Nick Rosbrook,
Wei Liu, Anthony PERARD, Juergen Gross, Christian Lindig,
David Scott, Marek Marczykowski-Górecki, Christian Lindig
On Arm, the SVE vector length is encoded in arch_capabilities field
of struct xen_sysctl_physinfo, make use of this field in the tools
when building for arm.
Create header arm-arch-capabilities.h to handle the arch_capabilities
field of physinfo for Arm.
Removed include for xen-tools/common-macros.h in
python/xen/lowlevel/xc/xc.c because it is already included by the
arm-arch-capabilities.h header.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
Acked-by: Christian Lindig <christian.lindig@cloud.com>
---
Changes from v3:
- add Ack-by for the Golang bits (George)
- add Ack-by for the OCaml tools (Christian)
- now xen-tools/libs.h is named xen-tools/common-macros.h
- changed commit message to explain why the header modification
in python/xen/lowlevel/xc/xc.c
Changes from v2:
- rename arm_arch_capabilities.h in arm-arch-capabilities.h, use
MASK_EXTR.
- Now arm-arch-capabilities.h needs MASK_EXTR macro, but it is
defined in libxl_internal.h, it doesn't feel right to include
that header so move MASK_EXTR into xen-tools/libs.h that is also
included in libxl_internal.h
Changes from v1:
- now SVE VL is encoded in arch_capabilities on Arm
Changes from RFC:
- new patch
---
tools/golang/xenlight/helpers.gen.go | 2 ++
tools/golang/xenlight/types.gen.go | 1 +
tools/include/arm-arch-capabilities.h | 33 +++++++++++++++++++++++++
tools/include/xen-tools/common-macros.h | 2 ++
tools/libs/light/libxl.c | 1 +
tools/libs/light/libxl_internal.h | 1 -
tools/libs/light/libxl_types.idl | 1 +
tools/ocaml/libs/xc/xenctrl.ml | 4 +--
tools/ocaml/libs/xc/xenctrl.mli | 4 +--
tools/ocaml/libs/xc/xenctrl_stubs.c | 8 +++---
tools/python/xen/lowlevel/xc/xc.c | 10 +++++---
tools/xl/xl_info.c | 8 ++++++
12 files changed, 61 insertions(+), 14 deletions(-)
create mode 100644 tools/include/arm-arch-capabilities.h
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index 0a203d22321f..35397be2f9e2 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -3506,6 +3506,7 @@ x.CapVmtrace = bool(xc.cap_vmtrace)
x.CapVpmu = bool(xc.cap_vpmu)
x.CapGnttabV1 = bool(xc.cap_gnttab_v1)
x.CapGnttabV2 = bool(xc.cap_gnttab_v2)
+x.ArchCapabilities = uint32(xc.arch_capabilities)
return nil}
@@ -3540,6 +3541,7 @@ xc.cap_vmtrace = C.bool(x.CapVmtrace)
xc.cap_vpmu = C.bool(x.CapVpmu)
xc.cap_gnttab_v1 = C.bool(x.CapGnttabV1)
xc.cap_gnttab_v2 = C.bool(x.CapGnttabV2)
+xc.arch_capabilities = C.uint32_t(x.ArchCapabilities)
return nil
}
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index a7c17699f80e..3d968a496744 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -1079,6 +1079,7 @@ CapVmtrace bool
CapVpmu bool
CapGnttabV1 bool
CapGnttabV2 bool
+ArchCapabilities uint32
}
type Connectorinfo struct {
diff --git a/tools/include/arm-arch-capabilities.h b/tools/include/arm-arch-capabilities.h
new file mode 100644
index 000000000000..46e876651052
--- /dev/null
+++ b/tools/include/arm-arch-capabilities.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 ARM Ltd.
+ */
+
+#ifndef ARM_ARCH_CAPABILITIES_H
+#define ARM_ARCH_CAPABILITIES_H
+
+/* Tell the Xen public headers we are a user-space tools build. */
+#ifndef __XEN_TOOLS__
+#define __XEN_TOOLS__ 1
+#endif
+
+#include <stdint.h>
+#include <xen/sysctl.h>
+
+#include <xen-tools/common-macros.h>
+
+static inline
+unsigned int arch_capabilities_arm_sve(unsigned int arch_capabilities)
+{
+#if defined(__aarch64__)
+ unsigned int sve_vl = MASK_EXTR(arch_capabilities,
+ XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
+
+ /* Vector length is divided by 128 before storing it in arch_capabilities */
+ return sve_vl * 128U;
+#else
+ return 0;
+#endif
+}
+
+#endif /* ARM_ARCH_CAPABILITIES_H */
diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
index 76b55bf62085..d53b88182560 100644
--- a/tools/include/xen-tools/common-macros.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -72,6 +72,8 @@
#define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
#endif
+#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
+
#ifndef __must_check
#define __must_check __attribute__((__warn_unused_result__))
#endif
diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
index a0bf7d186f69..175d6dde0b80 100644
--- a/tools/libs/light/libxl.c
+++ b/tools/libs/light/libxl.c
@@ -409,6 +409,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
!!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_gnttab_v1);
physinfo->cap_gnttab_v2 =
!!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_gnttab_v2);
+ physinfo->arch_capabilities = xcphysinfo.arch_capabilities;
GC_FREE;
return 0;
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 5244fde6239a..8aba3e138909 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -132,7 +132,6 @@
#define DIV_ROUNDUP(n, d) (((n) + (d) - 1) / (d))
-#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
#define LIBXL__LOGGING_ENABLED
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index c10292e0d7e3..fd31dacf7d5a 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -1133,6 +1133,7 @@ libxl_physinfo = Struct("physinfo", [
("cap_vpmu", bool),
("cap_gnttab_v1", bool),
("cap_gnttab_v2", bool),
+ ("arch_capabilities", uint32),
], dir=DIR_OUT)
libxl_connectorinfo = Struct("connectorinfo", [
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index e4096bf92c1d..bf23ca50bb15 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -128,12 +128,10 @@ type physinfo_cap_flag =
| CAP_Gnttab_v1
| CAP_Gnttab_v2
-type arm_physinfo_cap_flag
-
type x86_physinfo_cap_flag
type arch_physinfo_cap_flags =
- | ARM of arm_physinfo_cap_flag list
+ | ARM of int
| X86 of x86_physinfo_cap_flag list
type physinfo =
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index ef2254537430..ed1e28ea30a0 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -113,12 +113,10 @@ type physinfo_cap_flag =
| CAP_Gnttab_v1
| CAP_Gnttab_v2
-type arm_physinfo_cap_flag
-
type x86_physinfo_cap_flag
type arch_physinfo_cap_flags =
- | ARM of arm_physinfo_cap_flag list
+ | ARM of int
| X86 of x86_physinfo_cap_flag list
type physinfo = {
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 6ec9ed6d1e6f..526a3610fa42 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -853,13 +853,15 @@ CAMLprim value stub_xc_physinfo(value xch_val)
arch_cap_list = Tag_cons;
arch_cap_flags_tag = 1; /* tag x86 */
-#else
- caml_failwith("Unhandled architecture");
-#endif
arch_cap_flags = caml_alloc_small(1, arch_cap_flags_tag);
Store_field(arch_cap_flags, 0, arch_cap_list);
Store_field(physinfo, 10, arch_cap_flags);
+#elif defined(__aarch64__)
+ Store_field(physinfo, 10, Val_int(c_physinfo.arch_capabilities));
+#else
+ caml_failwith("Unhandled architecture");
+#endif
CAMLreturn(physinfo);
}
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index 35901c2d63b6..254d3b5dccd2 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -7,6 +7,7 @@
#define PY_SSIZE_T_CLEAN
#include <Python.h>
#define XC_WANT_COMPAT_MAP_FOREIGN_API
+#include <arm-arch-capabilities.h>
#include <xenctrl.h>
#include <xenguest.h>
#include <fcntl.h>
@@ -22,8 +23,6 @@
#include <xen/hvm/hvm_info_table.h>
#include <xen/hvm/params.h>
-#include <xen-tools/common-macros.h>
-
/* Needed for Python versions earlier than 2.3. */
#ifndef PyMODINIT_FUNC
#define PyMODINIT_FUNC DL_EXPORT(void)
@@ -897,7 +896,7 @@ static PyObject *pyxc_physinfo(XcObject *self)
if ( p != virt_caps )
*(p-1) = '\0';
- return Py_BuildValue("{s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s,s:s}",
+ return Py_BuildValue("{s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s,s:s,s:i}",
"nr_nodes", pinfo.nr_nodes,
"threads_per_core", pinfo.threads_per_core,
"cores_per_socket", pinfo.cores_per_socket,
@@ -907,7 +906,10 @@ static PyObject *pyxc_physinfo(XcObject *self)
"scrub_memory", pages_to_kib(pinfo.scrub_pages),
"cpu_khz", pinfo.cpu_khz,
"hw_caps", cpu_cap,
- "virt_caps", virt_caps);
+ "virt_caps", virt_caps,
+ "arm_sve_vl",
+ arch_capabilities_arm_sve(pinfo.arch_capabilities)
+ );
}
static PyObject *pyxc_getcpuinfo(XcObject *self, PyObject *args, PyObject *kwds)
diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
index 712b7638b013..bf18ba2449ef 100644
--- a/tools/xl/xl_info.c
+++ b/tools/xl/xl_info.c
@@ -14,6 +14,7 @@
#define _GNU_SOURCE
+#include <arm-arch-capabilities.h>
#include <fcntl.h>
#include <inttypes.h>
#include <stdlib.h>
@@ -224,6 +225,13 @@ static void output_physinfo(void)
info.cap_gnttab_v2 ? " gnttab-v2" : ""
);
+ /* Print arm SVE vector length only on ARM platforms */
+#if defined(__aarch64__)
+ maybe_printf("arm_sve_vector_length : %u\n",
+ arch_capabilities_arm_sve(info.arch_capabilities)
+ );
+#endif
+
vinfo = libxl_get_version_info(ctx);
if (vinfo) {
i = (1 << 20) / vinfo->pagesize;
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 10/12] xen/tools: add sve parameter in XL configuration
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
` (8 preceding siblings ...)
2023-03-27 10:59 ` [PATCH v4 09/12] tools: add physinfo arch_capabilities handling for Arm Luca Fancellu
@ 2023-03-27 10:59 ` Luca Fancellu
2023-03-31 14:23 ` Anthony PERARD
2023-03-27 10:59 ` [PATCH v4 11/12] xen/arm: add sve property for dom0less domUs Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 12/12] xen/changelog: Add SVE and "dom0" options to the changelog for Arm Luca Fancellu
11 siblings, 1 reply; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Wei Liu, Anthony PERARD,
George Dunlap, Nick Rosbrook, Juergen Gross, George Dunlap
Add sve parameter in XL configuration to allow guests to use
SVE feature.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Acked-by: George Dunlap <george.dunlap@cloud.com>
---
Changes from v3:
- no changes
Changes from v2:
- domain configuration field name has changed to sve_vl,
also its value now is VL/128.
- Add Ack-by George for the Golang bits
Changes from v1:
- updated to use arch_capabilities field for vector length
Changes from RFC:
- changed libxl_types.idl sve field to uint16
- now toolstack uses info from physinfo to check against the
sve XL value
- Changed documentation
---
docs/man/xl.cfg.5.pod.in | 11 +++++++++++
tools/golang/xenlight/helpers.gen.go | 2 ++
tools/golang/xenlight/types.gen.go | 1 +
tools/include/libxl.h | 5 +++++
tools/libs/light/libxl_arm.c | 2 ++
tools/libs/light/libxl_types.idl | 1 +
tools/xl/xl_parse.c | 26 ++++++++++++++++++++++++--
7 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 10f37990be57..adf48fe8ac1d 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2952,6 +2952,17 @@ Currently, only the "sbsa_uart" model is supported for ARM.
=back
+=item B<sve="NUMBER">
+
+To enable SVE, user must specify a number different from zero, maximum 2048 and
+multiple of 128. That value will be the maximum number of SVE registers bits
+that the hypervisor will impose to this guest. If the platform has a lower
+supported bits value, then the domain creation will fail.
+A value equal to zero is the default and it means this guest is not allowed to
+use SVE.
+
+=back
+
=head3 x86
=over 4
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index 35397be2f9e2..72a3a12a6065 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1149,6 +1149,7 @@ default:
return fmt.Errorf("invalid union key '%v'", x.Type)}
x.ArchArm.GicVersion = GicVersion(xc.arch_arm.gic_version)
x.ArchArm.Vuart = VuartType(xc.arch_arm.vuart)
+x.ArchArm.Sve = uint16(xc.arch_arm.sve)
if err := x.ArchX86.MsrRelaxed.fromC(&xc.arch_x86.msr_relaxed);err != nil {
return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
}
@@ -1653,6 +1654,7 @@ default:
return fmt.Errorf("invalid union key '%v'", x.Type)}
xc.arch_arm.gic_version = C.libxl_gic_version(x.ArchArm.GicVersion)
xc.arch_arm.vuart = C.libxl_vuart_type(x.ArchArm.Vuart)
+xc.arch_arm.sve = C.uint16_t(x.ArchArm.Sve)
if err := x.ArchX86.MsrRelaxed.toC(&xc.arch_x86.msr_relaxed); err != nil {
return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
}
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index 3d968a496744..3dc292b5f1be 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -564,6 +564,7 @@ TypeUnion DomainBuildInfoTypeUnion
ArchArm struct {
GicVersion GicVersion
Vuart VuartType
+Sve uint16
}
ArchX86 struct {
MsrRelaxed Defbool
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index cfa1a191318c..9f040316ad80 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -283,6 +283,11 @@
*/
#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1
+/*
+ * libxl_domain_build_info has the arch_arm.sve field.
+ */
+#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_SVE 1
+
/*
* LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing
* 'soft reset' for domains and there is 'soft_reset' shutdown reason
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index ddc7b2a15975..16a49031fd51 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -211,6 +211,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
return ERROR_FAIL;
}
+ config->arch.sve_vl = d_config->b_info.arch_arm.sve;
+
return 0;
}
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index fd31dacf7d5a..ef4a8358e54e 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -690,6 +690,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
("vuart", libxl_vuart_type),
+ ("sve", uint16),
])),
("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
])),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 1f6f47daf4e1..3cbc23b36952 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -12,6 +12,7 @@
* GNU Lesser General Public License for more details.
*/
+#include <arm-arch-capabilities.h>
#include <ctype.h>
#include <inttypes.h>
#include <limits.h>
@@ -1312,8 +1313,6 @@ void parse_config_data(const char *config_source,
exit(EXIT_FAILURE);
}
- libxl_physinfo_dispose(&physinfo);
-
config= xlu_cfg_init(stderr, config_source);
if (!config) {
fprintf(stderr, "Failed to allocate for configuration\n");
@@ -2887,6 +2886,29 @@ skip_usbdev:
}
}
+ if (!xlu_cfg_get_long (config, "sve", &l, 0)) {
+ unsigned int arm_sve_vl =
+ arch_capabilities_arm_sve(physinfo.arch_capabilities);
+ if (!arm_sve_vl) {
+ fprintf(stderr, "SVE is not supported by the platform\n");
+ exit(-ERROR_FAIL);
+ } else if (((l % 128) != 0) || (l > 2048)) {
+ fprintf(stderr,
+ "Invalid sve value: %ld. Needs to be <= 2048 and multiple"
+ " of 128\n", l);
+ exit(-ERROR_FAIL);
+ } else if (l > arm_sve_vl) {
+ fprintf(stderr,
+ "Invalid sve value: %ld. Platform supports up to %u bits\n",
+ l, arm_sve_vl);
+ exit(-ERROR_FAIL);
+ }
+ /* Vector length is divided by 128 in domain configuration struct */
+ b_info->arch_arm.sve = l / 128U;
+ }
+
+ libxl_physinfo_dispose(&physinfo);
+
parse_vkb_list(config, d_config);
d_config->virtios = NULL;
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 11/12] xen/arm: add sve property for dom0less domUs
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
` (9 preceding siblings ...)
2023-03-27 10:59 ` [PATCH v4 10/12] xen/tools: add sve parameter in XL configuration Luca Fancellu
@ 2023-03-27 10:59 ` Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 12/12] xen/changelog: Add SVE and "dom0" options to the changelog for Arm Luca Fancellu
11 siblings, 0 replies; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk
Add a device tree property in the dom0less domU configuration
to enable the guest to use SVE.
Update documentation.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Changes from v3:
- Now domainconfig_encode_vl is named sve_encode_vl
Changes from v2:
- xen_domctl_createdomain field name has changed into sve_vl
and its value is the VL/128, use domainconfig_encode_vl
to encode a plain VL in bits.
Changes from v1:
- No changes
Changes from RFC:
- Changed documentation
---
docs/misc/arm/device-tree/booting.txt | 9 +++++++++
xen/arch/arm/domain_build.c | 7 +++++++
2 files changed, 16 insertions(+)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 3879340b5e0a..d74bf9ab1c8b 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -193,6 +193,15 @@ with the following properties:
Optional. Handle to a xen,cpupool device tree node that identifies the
cpupool where the guest will be started at boot.
+- sve
+
+ Optional. A number that, when above 0, enables SVE for this guest and sets
+ its maximum SVE vector length. The default value is 0, that means this
+ guest is not allowed to use SVE, the maximum value allowed is 2048, any
+ other value must be multiple of 128.
+ Please note that if the platform supports a lower value of bits, then the
+ domain creation will fail.
+
- xen,enhanced
A string property. Possible property values are:
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f6019ce30149..45263aea7e95 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3974,6 +3974,13 @@ void __init create_domUs(void)
d_cfg.max_maptrack_frames = val;
}
+ if ( dt_property_read_u32(node, "sve", &val) )
+ {
+ if ( val > UINT16_MAX )
+ panic("sve property value (%"PRIu32") overflow\n", val);
+ d_cfg.arch.sve_vl = sve_encode_vl(val);
+ }
+
/*
* The variable max_init_domid is initialized with zero, so here it's
* very important to use the pre-increment operator to call
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v4 12/12] xen/changelog: Add SVE and "dom0" options to the changelog for Arm
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
` (10 preceding siblings ...)
2023-03-27 10:59 ` [PATCH v4 11/12] xen/arm: add sve property for dom0less domUs Luca Fancellu
@ 2023-03-27 10:59 ` Luca Fancellu
11 siblings, 0 replies; 33+ messages in thread
From: Luca Fancellu @ 2023-03-27 10:59 UTC (permalink / raw)
To: xen-devel; +Cc: bertrand.marquis, wei.chen, Henry Wang, Community Manager
Arm now can use the "dom0=" Xen command line option and the support
for guests running SVE instructions is added, put entries in the
changelog.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
Change from v3:
- new patch
---
CHANGELOG.md | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c978cfd9b68f..a24951603359 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
## [unstable UNRELEASED](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=staging) - TBD
+### Changed
+- The "dom0" option is now supported on Arm and "sve=" sub-option can be used
+ to enable dom0 guest to use SVE/SVE2 instructions.
+
### Added
- On x86, support for features new in Intel Sapphire Rapids CPUs:
- PKS (Protection Key Supervisor) available to HVM/PVH guests.
@@ -14,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Bus-lock detection, used by Xen to mitigate (by rate-limiting) the system
wide impact of a guest misusing atomic instructions.
- xl/libxl can customize SMBIOS strings for HVM guests.
+ - On Arm, Xen supports guests running SVE/SVE2 instructions.
## [4.17.0](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.17.0) - 2022-12-12
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v4 02/12] xen/arm: add SVE vector length field to the domain
2023-03-27 10:59 ` [PATCH v4 02/12] xen/arm: add SVE vector length field to the domain Luca Fancellu
@ 2023-03-28 9:36 ` Jan Beulich
2023-03-29 10:01 ` Luca Fancellu
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2023-03-28 9:36 UTC (permalink / raw)
To: Luca Fancellu
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
xen-devel
On 27.03.2023 12:59, Luca Fancellu wrote:
> @@ -43,8 +44,16 @@ register_t compute_max_zcr(void)
> }
>
> /* Takes a vector length in bits and returns the ZCR_ELx encoding */
> -register_t vl_to_zcr(uint16_t vl)
> +register_t vl_to_zcr(unsigned int vl)
> {
> ASSERT(vl > 0);
> return ((vl / SVE_VL_MULTIPLE_VAL) - 1U) & ZCR_ELx_LEN_MASK;
> }
> +
> +/* Get the system sanitized value for VL in bits */
> +unsigned int get_sys_vl_len(void)
> +{
> + /* ZCR_ELx len field is ((len+1) * 128) = vector bits length */
> + return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) *
> + SVE_VL_MULTIPLE_VAL;
> +}
Wouldn't this function better return 0 when !cpu_has_sve?
> @@ -602,6 +606,31 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> return -EINVAL;
> }
>
> + /* Check feature flags */
> + if ( sve_vl_bits > 0 ) {
Nit: Style (brace placement).
> + unsigned int zcr_max_bits = get_sys_vl_len();
> +
> + if ( !cpu_has_sve )
> + {
> + dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n");
> + return -EINVAL;
> + }
> + else if ( !is_vl_valid(sve_vl_bits) )
If this was code I'm the maintainer for, I'd ask for the "else" to be
dropped. Please consider doing so, as it makes more visible that the
earlier if() cannot "fall through". (This could then be further
supported by inserting blank lines, here and again right below.)
As to the check - this being the only user of is_vl_valid() (at this
point) I'd like to mention that half of what that function checks is
now pointless, as we're dealing with a decoded value. Future further
users may need the full value checked, but given that all interfaces
are now using encoded values this doesn't seem very likely. Hence the
respective part of the condition there may want to become an
assertion instead (or be dropped).
Yet another question is whether both range checks (against
SVE_VL_MAX_BITS and zcr_max_bits) are actually necessary / useful.
Iirc 2048 is a hard upper bound, so zcr_max_bits being higher than
that value should likely lead to not using SVE at all (if it doesn't
already; didn't check).
> + {
> + dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n",
> + sve_vl_bits);
> + return -EINVAL;
> + }
> + else if ( sve_vl_bits > zcr_max_bits )
> + {
> + dprintk(XENLOG_INFO,
> + "The requested SVE vector length (%u) must be lower or \n"
> + "equal to the platform supported vector length (%u)\n",
> + sve_vl_bits, zcr_max_bits);
Again, if I was the maintainer of this code, I'd ask for the message
to be shortened, such that it easily fits on one line. E.g.
"requested SVE vector length (%u) > supported length (%u)\n". This
would then also avoid the apparent grammar issue of "lower" not fitting
with "to" (i.e. a "than" needing inserting, or "below" being used
instead).
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 06/12] xen/common: add dom0 xen command line argument for Arm
2023-03-27 10:59 ` [PATCH v4 06/12] xen/common: add dom0 xen command line argument for Arm Luca Fancellu
@ 2023-03-28 9:44 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2023-03-28 9:44 UTC (permalink / raw)
To: Luca Fancellu
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
Roger Pau Monné,
xen-devel
On 27.03.2023 12:59, Luca Fancellu wrote:
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -59,6 +59,11 @@ static int __init parse_dom0_mem(const char *s)
> }
> custom_param("dom0_mem", parse_dom0_mem);
>
> +int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
> +{
> + return -1;
> +}
Please also use -E... here, like x86 does.
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -266,42 +266,30 @@ bool __initdata opt_dom0_pvh = !IS_ENABLED(CONFIG_PV);
> bool __initdata opt_dom0_verbose = IS_ENABLED(CONFIG_VERBOSE_DEBUG);
> bool __initdata opt_dom0_msr_relaxed;
>
> -static int __init cf_check parse_dom0_param(const char *s)
> +int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
Is there any reason you couldn't stick to the original variable names (s
and ss) or use other meaningful by shorter names like s and e or str and
end (my personal preference among the three in the order given)? That'll
help with line length and hence readability.
> {
> - const char *ss;
> - int rc = 0;
> -
> - do {
> - int val;
> -
> - ss = strchr(s, ',');
> - if ( !ss )
> - ss = strchr(s, '\0');
> + int val, rc = 0;
>
> - if ( IS_ENABLED(CONFIG_PV) && !cmdline_strcmp(s, "pv") )
> - opt_dom0_pvh = false;
> - else if ( IS_ENABLED(CONFIG_HVM) && !cmdline_strcmp(s, "pvh") )
> - opt_dom0_pvh = true;
> + if ( IS_ENABLED(CONFIG_PV) && !cmdline_strcmp(str_begin, "pv") )
> + opt_dom0_pvh = false;
> + else if ( IS_ENABLED(CONFIG_HVM) && !cmdline_strcmp(str_begin, "pvh") )
> + opt_dom0_pvh = true;
> #ifdef CONFIG_SHADOW_PAGING
> - else if ( (val = parse_boolean("shadow", s, ss)) >= 0 )
> - opt_dom0_shadow = val;
> + else if ( (val = parse_boolean("shadow", str_begin, str_end)) >= 0 )
> + opt_dom0_shadow = val;
> #endif
> - else if ( (val = parse_boolean("verbose", s, ss)) >= 0 )
> - opt_dom0_verbose = val;
> - else if ( IS_ENABLED(CONFIG_PV) &&
> - (val = parse_boolean("cpuid-faulting", s, ss)) >= 0 )
> - opt_dom0_cpuid_faulting = val;
> - else if ( (val = parse_boolean("msr-relaxed", s, ss)) >= 0 )
> - opt_dom0_msr_relaxed = val;
> - else
> - rc = -EINVAL;
> -
> - s = ss + 1;
> - } while ( *ss );
> + else if ( (val = parse_boolean("verbose", str_begin, str_end)) >= 0 )
> + opt_dom0_verbose = val;
> + else if ( IS_ENABLED(CONFIG_PV) &&
> + (val = parse_boolean("cpuid-faulting", str_begin, str_end)) >= 0 )
> + opt_dom0_cpuid_faulting = val;
> + else if ( (val = parse_boolean("msr-relaxed", str_begin, str_end)) >= 0 )
> + opt_dom0_msr_relaxed = val;
> + else
> + rc = -EINVAL;
>
> return rc;
> }
While largely a style aspect (and hence I'm not going to insist), I don't
see the value of the "rc" local variable with the changed logic. With at
least the other aspects addressed
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 07/12] xen: enable Dom0 to use SVE feature
2023-03-27 10:59 ` [PATCH v4 07/12] xen: enable Dom0 to use SVE feature Luca Fancellu
@ 2023-03-28 10:08 ` Jan Beulich
2023-03-29 11:48 ` Luca Fancellu
2023-03-29 12:06 ` Luca Fancellu
0 siblings, 2 replies; 33+ messages in thread
From: Jan Beulich @ 2023-03-28 10:08 UTC (permalink / raw)
To: Luca Fancellu
Cc: bertrand.marquis, wei.chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, Volodymyr Babchuk,
xen-devel
On 27.03.2023 12:59, Luca Fancellu wrote:
> @@ -838,6 +838,18 @@ Controls for how dom0 is constructed on x86 systems.
>
> If using this option is necessary to fix an issue, please report a bug.
>
> +Enables features on dom0 on Arm systems.
> +
> +* The `sve` integer parameter enables Arm SVE usage for Dom0 domain and sets
> + the maximum SVE vector length.
> + Values above 0 means feature is enabled for Dom0, otherwise feature is
> + disabled.
Nit: "above" suggests negative values may also enable the feature, which
I'm sure isn't intended. You may want to consider using negative values
to signal "use length supported by hardware".
> + Possible values are from 0 to maximum 2048, being multiple of 128, that will
> + be the maximum vector length.
It may be advisable to also state the default here.
> + Please note that the platform can supports a lower value, if the requested
Maybe better "... may only support ..."?
> + value is above the supported one, the domain creation will fail and the
> + system will stop.
Such behavior may be acceptable for DomU-s which aren't essential for the
system (i.e. possibly excluding ones in dom0less scenarios), but I don't
think that's very nice for Dom0. I'd rather suggest falling back to no
SVE in such an event.
> @@ -115,3 +119,8 @@ void sve_restore_state(struct vcpu *v)
>
> sve_load_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1);
> }
> +
> +int __init sve_parse_dom0_param(const char *str_begin, const char *str_end)
> +{
> + return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve);
Please can you avoid introducing casts like this? If you're after an unsigned
value, make a function which only parses (and returns) an unsigned one. Also
why ...
> @@ -61,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem);
>
> int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
> {
> - return -1;
> + int rc = 0;
> +
> + if ( sve_parse_dom0_param(str_begin, str_end) < 0 )
> + rc = -EINVAL;
... can't you call parse_integer() right here? opt_dom0_sve isn't static,
so ought to be accessible here (provided the necessary header was included).
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -314,6 +314,30 @@ int parse_boolean(const char *name, const char *s, const char *e)
> return -1;
> }
>
> +int parse_integer(const char *name, const char *s, const char *e,
> + int *val)
> +{
> + size_t slen, nlen;
> + const char *str;
> + long long pval;
> +
> + slen = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s);
> + nlen = strlen(name);
> +
> + /* Does s start with name or contains only the name? */
> + if ( (slen <= nlen) || strncmp(s, name, nlen) || (s[nlen] != '=') )
> + return -1;
> +
> + pval = simple_strtoll(&s[nlen + 1], &str, 0);
> +
> + if ( (str != e) || (pval < INT_MIN) || (pval > INT_MAX) )
> + return -2;
Like its counterpart in parse_boolean() (which I understand you've
derived parts of the function from) this if+return wants a comment.
Also - why strtoll() when you're only after an int? Yet then another
question is whether we really want to gain parse_long() and
parse_longlong() functions subsequently, or whether instead we
limit ourselves to (e.g.) parse_signed_integer() and
parse_unsigned_integer(), taking long long * and unsigned long long *
respectively to store their outputs. (Of course right now you'd
implement only one of the two.)
Finally, for the purposes right now the function can (and should) be
__init.
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -94,6 +94,16 @@ int parse_bool(const char *s, const char *e);
> */
> int parse_boolean(const char *name, const char *s, const char *e);
>
> +/**
> + * Given a specific name, parses a string of the form:
> + * $NAME[=...]
> + * returning 0 and a value in val, for a recognised integer.
> + * Returns -1 for name not found, general errors, or -2 if name is found but
> + * not recognised/overflow/underflow value.
> + */
> +int parse_integer(const char *name, const char *s, const char *e,
> + int *val);
The comment wants to match function behavior: The '=' and the value
aren't optional as per the implementation, unlike for parse_boolean().
Also please be precise and say "... and a value in *val, ..."
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 08/12] xen/physinfo: encode Arm SVE vector length in arch_capabilities
2023-03-27 10:59 ` [PATCH v4 08/12] xen/physinfo: encode Arm SVE vector length in arch_capabilities Luca Fancellu
@ 2023-03-28 10:14 ` Jan Beulich
2023-03-30 10:41 ` Luca Fancellu
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2023-03-28 10:14 UTC (permalink / raw)
To: Luca Fancellu
Cc: bertrand.marquis, wei.chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
xen-devel
On 27.03.2023 12:59, Luca Fancellu wrote:
> --- a/xen/arch/arm/arm64/sve.c
> +++ b/xen/arch/arm/arm64/sve.c
> @@ -124,3 +124,15 @@ int __init sve_parse_dom0_param(const char *str_begin, const char *str_end)
> {
> return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve);
> }
> +
> +void sve_arch_cap_physinfo(uint32_t *arch_capabilities)
> +{
> + if ( cpu_has_sve )
> + {
> + /* Vector length is divided by 128 to save some space */
> + uint32_t sve_vl = MASK_INSR(sve_encode_vl(get_sys_vl_len()),
> + XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
> +
> + *arch_capabilities |= sve_vl;
> + }
> +}
I'm again wondering why a separate function is needed, when everything
that's needed is ...
> --- a/xen/arch/arm/sysctl.c
> +++ b/xen/arch/arm/sysctl.c
> @@ -11,11 +11,14 @@
> #include <xen/lib.h>
> #include <xen/errno.h>
> #include <xen/hypercall.h>
> +#include <asm/arm64/sve.h>
... becoming available here for use ...
> #include <public/sysctl.h>
>
> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> {
> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
> +
> + sve_arch_cap_physinfo(&pi->arch_capabilities);
... here. That would be even more so if, like suggested before,
get_sys_vl_len() returned 0 when !cpu_has_sve.
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 02/12] xen/arm: add SVE vector length field to the domain
2023-03-28 9:36 ` Jan Beulich
@ 2023-03-29 10:01 ` Luca Fancellu
2023-03-29 10:32 ` Jan Beulich
0 siblings, 1 reply; 33+ messages in thread
From: Luca Fancellu @ 2023-03-29 10:01 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Wei Chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
xen-devel
Hi Jan,
Thank you for your review, very appreciated,
> On 28 Mar 2023, at 10:36, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 27.03.2023 12:59, Luca Fancellu wrote:
>> @@ -43,8 +44,16 @@ register_t compute_max_zcr(void)
>> }
>>
>> /* Takes a vector length in bits and returns the ZCR_ELx encoding */
>> -register_t vl_to_zcr(uint16_t vl)
>> +register_t vl_to_zcr(unsigned int vl)
>> {
>> ASSERT(vl > 0);
>> return ((vl / SVE_VL_MULTIPLE_VAL) - 1U) & ZCR_ELx_LEN_MASK;
>> }
>> +
>> +/* Get the system sanitized value for VL in bits */
>> +unsigned int get_sys_vl_len(void)
>> +{
>> + /* ZCR_ELx len field is ((len+1) * 128) = vector bits length */
>> + return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) *
>> + SVE_VL_MULTIPLE_VAL;
>> +}
>
> Wouldn't this function better return 0 when !cpu_has_sve?
Yes I agree
>
>> @@ -602,6 +606,31 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>> return -EINVAL;
>> }
>>
>> + /* Check feature flags */
>> + if ( sve_vl_bits > 0 ) {
>
> Nit: Style (brace placement).
Will fix
>
>> + unsigned int zcr_max_bits = get_sys_vl_len();
>> +
>> + if ( !cpu_has_sve )
>> + {
>> + dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n");
>> + return -EINVAL;
>> + }
>> + else if ( !is_vl_valid(sve_vl_bits) )
>
> If this was code I'm the maintainer for, I'd ask for the "else" to be
> dropped. Please consider doing so, as it makes more visible that the
> earlier if() cannot "fall through". (This could then be further
> supported by inserting blank lines, here and again right below.)
>
> As to the check - this being the only user of is_vl_valid() (at this
> point) I'd like to mention that half of what that function checks is
> now pointless, as we're dealing with a decoded value. Future further
> users may need the full value checked, but given that all interfaces
> are now using encoded values this doesn't seem very likely. Hence the
> respective part of the condition there may want to become an
> assertion instead (or be dropped).
Yes I can do that
>
> Yet another question is whether both range checks (against
> SVE_VL_MAX_BITS and zcr_max_bits) are actually necessary / useful.
> Iirc 2048 is a hard upper bound, so zcr_max_bits being higher than
> that value should likely lead to not using SVE at all (if it doesn't
> already; didn't check).
I think the check sve_vl_bits > zcr_max_bits is needed because from
sve_vl_bits = sve_decode_vl(config->arch.sve_vl); I can get values above the
maximum supported bits (zcr_max_bits), later on I will use the struct arch_domain
field sve_vl to compute the size of the registers to be saved/restore
Is there something I’ve missed from your comment?
>
>> + {
>> + dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n",
>> + sve_vl_bits);
>> + return -EINVAL;
>> + }
>> + else if ( sve_vl_bits > zcr_max_bits )
>> + {
>> + dprintk(XENLOG_INFO,
>> + "The requested SVE vector length (%u) must be lower or \n"
>> + "equal to the platform supported vector length (%u)\n",
>> + sve_vl_bits, zcr_max_bits);
>
> Again, if I was the maintainer of this code, I'd ask for the message
> to be shortened, such that it easily fits on one line. E.g.
> "requested SVE vector length (%u) > supported length (%u)\n". This
> would then also avoid the apparent grammar issue of "lower" not fitting
> with "to" (i.e. a "than" needing inserting, or "below" being used
> instead).
Yes this suggestion makes sense to me.
To address your comments I’m doing these modifications to the patch, I hope
they caption all your feedbacks:
diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c
index 3c3adfb5c6bd..78f7482619da 100644
--- a/xen/arch/arm/arm64/sve.c
+++ b/xen/arch/arm/arm64/sve.c
@@ -53,6 +53,9 @@ register_t vl_to_zcr(unsigned int vl)
/* Get the system sanitized value for VL in bits */
unsigned int get_sys_vl_len(void)
{
+ if ( !cpu_has_sve )
+ return 0;
+
/* ZCR_ELx len field is ((len+1) * 128) = vector bits length */
return ((system_cpuinfo.zcr64.bits[0] & ZCR_ELx_LEN_MASK) + 1U) *
SVE_VL_MULTIPLE_VAL;
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 7182d4567bf0..c1fb30dfc5ef 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -607,7 +607,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
}
/* Check feature flags */
- if ( sve_vl_bits > 0 ) {
+ if ( sve_vl_bits > 0 )
+ {
unsigned int zcr_max_bits = get_sys_vl_len();
if ( !cpu_has_sve )
@@ -615,17 +616,18 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
dprintk(XENLOG_INFO, "SVE is unsupported on this machine.\n");
return -EINVAL;
}
- else if ( !is_vl_valid(sve_vl_bits) )
+
+ if ( !is_vl_valid(sve_vl_bits) )
{
dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n",
sve_vl_bits);
return -EINVAL;
}
- else if ( sve_vl_bits > zcr_max_bits )
+
+ if ( sve_vl_bits > zcr_max_bits )
{
dprintk(XENLOG_INFO,
- "The requested SVE vector length (%u) must be lower or \n"
- "equal to the platform supported vector length (%u)\n",
+ "Requested SVE vector length (%u) > supported length (%u)\n",
sve_vl_bits, zcr_max_bits);
return -EINVAL;
}
diff --git a/xen/arch/arm/include/asm/arm64/sve.h b/xen/arch/arm/include/asm/arm64/sve.h
index 8037f09b5b0a..e8c01a24e6da 100644
--- a/xen/arch/arm/include/asm/arm64/sve.h
+++ b/xen/arch/arm/include/asm/arm64/sve.h
@@ -16,7 +16,9 @@
static inline bool is_vl_valid(unsigned int vl)
{
/* SVE vector length is multiple of 128 and maximum 2048 */
- return ((vl % SVE_VL_MULTIPLE_VAL) == 0) && (vl <= SVE_VL_MAX_BITS);
+ ASSERT((vl % SVE_VL_MULTIPLE_VAL) == 0);
+
+ return vl <= SVE_VL_MAX_BITS;
}
static inline unsigned int sve_decode_vl(unsigned int sve_vl)
>
> Jan
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v4 02/12] xen/arm: add SVE vector length field to the domain
2023-03-29 10:01 ` Luca Fancellu
@ 2023-03-29 10:32 ` Jan Beulich
2023-03-29 10:45 ` Luca Fancellu
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2023-03-29 10:32 UTC (permalink / raw)
To: Luca Fancellu
Cc: Bertrand Marquis, Wei Chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
xen-devel
On 29.03.2023 12:01, Luca Fancellu wrote:
>> On 28 Mar 2023, at 10:36, Jan Beulich <jbeulich@suse.com> wrote:
>> Yet another question is whether both range checks (against
>> SVE_VL_MAX_BITS and zcr_max_bits) are actually necessary / useful.
>> Iirc 2048 is a hard upper bound, so zcr_max_bits being higher than
>> that value should likely lead to not using SVE at all (if it doesn't
>> already; didn't check).
>
> I think the check sve_vl_bits > zcr_max_bits is needed because from
> sve_vl_bits = sve_decode_vl(config->arch.sve_vl); I can get values above the
> maximum supported bits (zcr_max_bits), later on I will use the struct arch_domain
> field sve_vl to compute the size of the registers to be saved/restore
>
> Is there something I’ve missed from your comment?
Hmm, I realize my earlier response may have been ambiguous: I didn't
mean to question the presence of both checks individually. I merely
meant to question whether in addition to the zcr_max_bits check you
really also need the SVE_VL_MAX_BITS one.
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 02/12] xen/arm: add SVE vector length field to the domain
2023-03-29 10:32 ` Jan Beulich
@ 2023-03-29 10:45 ` Luca Fancellu
0 siblings, 0 replies; 33+ messages in thread
From: Luca Fancellu @ 2023-03-29 10:45 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Wei Chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
xen-devel
> On 29 Mar 2023, at 11:32, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 29.03.2023 12:01, Luca Fancellu wrote:
>>> On 28 Mar 2023, at 10:36, Jan Beulich <jbeulich@suse.com> wrote:
>>> Yet another question is whether both range checks (against
>>> SVE_VL_MAX_BITS and zcr_max_bits) are actually necessary / useful.
>>> Iirc 2048 is a hard upper bound, so zcr_max_bits being higher than
>>> that value should likely lead to not using SVE at all (if it doesn't
>>> already; didn't check).
>>
>> I think the check sve_vl_bits > zcr_max_bits is needed because from
>> sve_vl_bits = sve_decode_vl(config->arch.sve_vl); I can get values above the
>> maximum supported bits (zcr_max_bits), later on I will use the struct arch_domain
>> field sve_vl to compute the size of the registers to be saved/restore
>>
>> Is there something I’ve missed from your comment?
>
> Hmm, I realize my earlier response may have been ambiguous: I didn't
> mean to question the presence of both checks individually. I merely
> meant to question whether in addition to the zcr_max_bits check you
> really also need the SVE_VL_MAX_BITS one.
Oh ok now I see what you mean, this check
if ( !is_vl_valid(sve_vl_bits) )
{
dprintk(XENLOG_INFO, "Unsupported SVE vector length (%u)\n”,
sve_vl_bits);
return -EINVAL;
}
Can be removed because if ( sve_vl_bits > zcr_max_bits ) is enough,
I agree and so is_vl_valid is not required anymore
>
> Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 07/12] xen: enable Dom0 to use SVE feature
2023-03-28 10:08 ` Jan Beulich
@ 2023-03-29 11:48 ` Luca Fancellu
2023-03-29 11:54 ` Jan Beulich
2023-03-29 12:06 ` Luca Fancellu
1 sibling, 1 reply; 33+ messages in thread
From: Luca Fancellu @ 2023-03-29 11:48 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, Volodymyr Babchuk,
xen-devel
> On 28 Mar 2023, at 11:08, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 27.03.2023 12:59, Luca Fancellu wrote:
>> @@ -838,6 +838,18 @@ Controls for how dom0 is constructed on x86 systems.
>>
>> If using this option is necessary to fix an issue, please report a bug.
>>
>> +Enables features on dom0 on Arm systems.
>> +
>> +* The `sve` integer parameter enables Arm SVE usage for Dom0 domain and sets
>> + the maximum SVE vector length.
>> + Values above 0 means feature is enabled for Dom0, otherwise feature is
>> + disabled.
>
> Nit: "above" suggests negative values may also enable the feature, which
> I'm sure isn't intended. You may want to consider using negative values
> to signal "use length supported by hardware".
This is a very good suggestion, do you think I should restrict only to one negative value,
for example -1, instead of every negative value?
>
>> + Possible values are from 0 to maximum 2048, being multiple of 128, that will
>> + be the maximum vector length.
>
> It may be advisable to also state the default here.
I will add it
>
>> + Please note that the platform can supports a lower value, if the requested
>
> Maybe better "... may only support ..."?
ok
>
>> + value is above the supported one, the domain creation will fail and the
>> + system will stop.
>
> Such behavior may be acceptable for DomU-s which aren't essential for the
> system (i.e. possibly excluding ones in dom0less scenarios), but I don't
> think that's very nice for Dom0. I'd rather suggest falling back to no
> SVE in such an event.
I guess that with the introduction of a negative value meaning max supported
VL, it is ok to stop the system if the user provides a custom VL value that is
not OK. I remember Julien asked to stop the creation of Dom0 in such a case on
the RFC serie.
>
>> @@ -115,3 +119,8 @@ void sve_restore_state(struct vcpu *v)
>>
>> sve_load_ctx(sve_ctx_zreg_end, v->arch.vfp.fpregs, 1);
>> }
>> +
>> +int __init sve_parse_dom0_param(const char *str_begin, const char *str_end)
>> +{
>> + return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve);
>
> Please can you avoid introducing casts like this? If you're after an unsigned
> value, make a function which only parses (and returns) an unsigned one. Also
> why ...
>
>> @@ -61,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem);
>>
>> int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
>> {
>> - return -1;
>> + int rc = 0;
>> +
>> + if ( sve_parse_dom0_param(str_begin, str_end) < 0 )
>> + rc = -EINVAL;
>
> ... can't you call parse_integer() right here? opt_dom0_sve isn't static,
> so ought to be accessible here (provided the necessary header was included).
>
>> --- a/xen/common/kernel.c
>> +++ b/xen/common/kernel.c
>> @@ -314,6 +314,30 @@ int parse_boolean(const char *name, const char *s, const char *e)
>> return -1;
>> }
>>
>> +int parse_integer(const char *name, const char *s, const char *e,
>> + int *val)
>> +{
>> + size_t slen, nlen;
>> + const char *str;
>> + long long pval;
>> +
>> + slen = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s);
>> + nlen = strlen(name);
>> +
>> + /* Does s start with name or contains only the name? */
>> + if ( (slen <= nlen) || strncmp(s, name, nlen) || (s[nlen] != '=') )
>> + return -1;
>> +
>> + pval = simple_strtoll(&s[nlen + 1], &str, 0);
>> +
>> + if ( (str != e) || (pval < INT_MIN) || (pval > INT_MAX) )
>> + return -2;
>
> Like its counterpart in parse_boolean() (which I understand you've
> derived parts of the function from) this if+return wants a comment.
> Also - why strtoll() when you're only after an int? Yet then another
> question is whether we really want to gain parse_long() and
> parse_longlong() functions subsequently, or whether instead we
> limit ourselves to (e.g.) parse_signed_integer() and
> parse_unsigned_integer(), taking long long * and unsigned long long *
> respectively to store their outputs. (Of course right now you'd
> implement only one of the two.)
>
> Finally, for the purposes right now the function can (and should) be
> __init.
>
>> --- a/xen/include/xen/lib.h
>> +++ b/xen/include/xen/lib.h
>> @@ -94,6 +94,16 @@ int parse_bool(const char *s, const char *e);
>> */
>> int parse_boolean(const char *name, const char *s, const char *e);
>>
>> +/**
>> + * Given a specific name, parses a string of the form:
>> + * $NAME[=...]
>> + * returning 0 and a value in val, for a recognised integer.
>> + * Returns -1 for name not found, general errors, or -2 if name is found but
>> + * not recognised/overflow/underflow value.
>> + */
>> +int parse_integer(const char *name, const char *s, const char *e,
>> + int *val);
>
> The comment wants to match function behavior: The '=' and the value
> aren't optional as per the implementation, unlike for parse_boolean().
> Also please be precise and say "... and a value in *val, ..."
Ok I will address all the comments above
>
> Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 07/12] xen: enable Dom0 to use SVE feature
2023-03-29 11:48 ` Luca Fancellu
@ 2023-03-29 11:54 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2023-03-29 11:54 UTC (permalink / raw)
To: Luca Fancellu
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, Volodymyr Babchuk,
xen-devel
On 29.03.2023 13:48, Luca Fancellu wrote:
>> On 28 Mar 2023, at 11:08, Jan Beulich <jbeulich@suse.com> wrote:
>> On 27.03.2023 12:59, Luca Fancellu wrote:
>>> @@ -838,6 +838,18 @@ Controls for how dom0 is constructed on x86 systems.
>>>
>>> If using this option is necessary to fix an issue, please report a bug.
>>>
>>> +Enables features on dom0 on Arm systems.
>>> +
>>> +* The `sve` integer parameter enables Arm SVE usage for Dom0 domain and sets
>>> + the maximum SVE vector length.
>>> + Values above 0 means feature is enabled for Dom0, otherwise feature is
>>> + disabled.
>>
>> Nit: "above" suggests negative values may also enable the feature, which
>> I'm sure isn't intended. You may want to consider using negative values
>> to signal "use length supported by hardware".
>
> This is a very good suggestion, do you think I should restrict only to one negative value,
> for example -1, instead of every negative value?
That highly depends on whether there's any foreseeable use for other negative
values. I can't imagine such, so I'm inclined to say that "just negative" is
all that matters.
>>> + Please note that the platform can supports a lower value, if the requested
>>
>> Maybe better "... may only support ..."?
>
> ok
>
>>
>>> + value is above the supported one, the domain creation will fail and the
>>> + system will stop.
>>
>> Such behavior may be acceptable for DomU-s which aren't essential for the
>> system (i.e. possibly excluding ones in dom0less scenarios), but I don't
>> think that's very nice for Dom0. I'd rather suggest falling back to no
>> SVE in such an event.
>
> I guess that with the introduction of a negative value meaning max supported
> VL, it is ok to stop the system if the user provides a custom VL value that is
> not OK. I remember Julien asked to stop the creation of Dom0 in such a case on
> the RFC serie.
Oh, okay. I don't mean to override a maintainer's view. I don't see the
connection to assigning meaning to negative values though - preventing
successful (even if functionally restricted) boot is imo never a good
thing, when it can easily be avoided.
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 07/12] xen: enable Dom0 to use SVE feature
2023-03-28 10:08 ` Jan Beulich
2023-03-29 11:48 ` Luca Fancellu
@ 2023-03-29 12:06 ` Luca Fancellu
2023-03-29 12:24 ` Jan Beulich
1 sibling, 1 reply; 33+ messages in thread
From: Luca Fancellu @ 2023-03-29 12:06 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, Volodymyr Babchuk,
xen-devel
>
>> @@ -61,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem);
>>
>> int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
>> {
>> - return -1;
>> + int rc = 0;
>> +
>> + if ( sve_parse_dom0_param(str_begin, str_end) < 0 )
>> + rc = -EINVAL;
>
> ... can't you call parse_integer() right here? opt_dom0_sve isn't static,
> so ought to be accessible here (provided the necessary header was included).
>
Oh ok now I’ve seen why I’m doing this, because ops_dom0_sve is compiled only
when CONFIG_ARM64_SVE is enabled, so I’m using sve_parse_dom0_param()
that returns negative if that option is not enabled.
Otherwise I should declare ops_dom0_sve anyway, but I should not accept user
customization of it if the option is not enabled.
So I thought the use of sve_parse_dom0_param() was the best way to handle that
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 07/12] xen: enable Dom0 to use SVE feature
2023-03-29 12:06 ` Luca Fancellu
@ 2023-03-29 12:24 ` Jan Beulich
2023-03-29 14:33 ` Luca Fancellu
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2023-03-29 12:24 UTC (permalink / raw)
To: Luca Fancellu
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, Volodymyr Babchuk,
xen-devel
On 29.03.2023 14:06, Luca Fancellu wrote:
>>> @@ -61,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem);
>>>
>>> int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
>>> {
>>> - return -1;
>>> + int rc = 0;
>>> +
>>> + if ( sve_parse_dom0_param(str_begin, str_end) < 0 )
>>> + rc = -EINVAL;
>>
>> ... can't you call parse_integer() right here? opt_dom0_sve isn't static,
>> so ought to be accessible here (provided the necessary header was included).
>>
>
> Oh ok now I’ve seen why I’m doing this, because ops_dom0_sve is compiled only
> when CONFIG_ARM64_SVE is enabled, so I’m using sve_parse_dom0_param()
> that returns negative if that option is not enabled.
>
> Otherwise I should declare ops_dom0_sve anyway, but I should not accept user
> customization of it if the option is not enabled.
>
> So I thought the use of sve_parse_dom0_param() was the best way to handle that
Maybe. But please also pay attention to the existence of no_config_param()
(as in: consider using it here, which would require the code to live outside
of sve.c).
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 07/12] xen: enable Dom0 to use SVE feature
2023-03-29 12:24 ` Jan Beulich
@ 2023-03-29 14:33 ` Luca Fancellu
0 siblings, 0 replies; 33+ messages in thread
From: Luca Fancellu @ 2023-03-29 14:33 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Wei Chen, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Wei Liu, Volodymyr Babchuk,
xen-devel
> On 29 Mar 2023, at 13:24, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 29.03.2023 14:06, Luca Fancellu wrote:
>>>> @@ -61,7 +62,12 @@ custom_param("dom0_mem", parse_dom0_mem);
>>>>
>>>> int __init parse_arch_dom0_param(const char *str_begin, const char *str_end)
>>>> {
>>>> - return -1;
>>>> + int rc = 0;
>>>> +
>>>> + if ( sve_parse_dom0_param(str_begin, str_end) < 0 )
>>>> + rc = -EINVAL;
>>>
>>> ... can't you call parse_integer() right here? opt_dom0_sve isn't static,
>>> so ought to be accessible here (provided the necessary header was included).
>>>
>>
>> Oh ok now I’ve seen why I’m doing this, because ops_dom0_sve is compiled only
>> when CONFIG_ARM64_SVE is enabled, so I’m using sve_parse_dom0_param()
>> that returns negative if that option is not enabled.
>>
>> Otherwise I should declare ops_dom0_sve anyway, but I should not accept user
>> customization of it if the option is not enabled.
>>
>> So I thought the use of sve_parse_dom0_param() was the best way to handle that
>
> Maybe. But please also pay attention to the existence of no_config_param()
> (as in: consider using it here, which would require the code to live outside
> of sve.c).
Thank you, I didn’t know the existence of no_config_param(), I’ve had a look on the
approach, for example in static int __init cf_check parse_cet(const char *s), and I’ll do
something similar
>
> Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 08/12] xen/physinfo: encode Arm SVE vector length in arch_capabilities
2023-03-28 10:14 ` Jan Beulich
@ 2023-03-30 10:41 ` Luca Fancellu
2023-03-30 10:49 ` Jan Beulich
0 siblings, 1 reply; 33+ messages in thread
From: Luca Fancellu @ 2023-03-30 10:41 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Wei Chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
xen-devel
> On 28 Mar 2023, at 11:14, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 27.03.2023 12:59, Luca Fancellu wrote:
>> --- a/xen/arch/arm/arm64/sve.c
>> +++ b/xen/arch/arm/arm64/sve.c
>> @@ -124,3 +124,15 @@ int __init sve_parse_dom0_param(const char *str_begin, const char *str_end)
>> {
>> return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve);
>> }
>> +
>> +void sve_arch_cap_physinfo(uint32_t *arch_capabilities)
>> +{
>> + if ( cpu_has_sve )
>> + {
>> + /* Vector length is divided by 128 to save some space */
>> + uint32_t sve_vl = MASK_INSR(sve_encode_vl(get_sys_vl_len()),
>> + XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
>> +
>> + *arch_capabilities |= sve_vl;
>> + }
>> +}
>
> I'm again wondering why a separate function is needed, when everything
> that's needed is ...
>
>> --- a/xen/arch/arm/sysctl.c
>> +++ b/xen/arch/arm/sysctl.c
>> @@ -11,11 +11,14 @@
>> #include <xen/lib.h>
>> #include <xen/errno.h>
>> #include <xen/hypercall.h>
>> +#include <asm/arm64/sve.h>
>
> ... becoming available here for use ...
>
>> #include <public/sysctl.h>
>>
>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>> {
>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
>> +
>> + sve_arch_cap_physinfo(&pi->arch_capabilities);
>
> ... here. That would be even more so if, like suggested before,
> get_sys_vl_len() returned 0 when !cpu_has_sve.
I’ve had a look on this, I can do everything In arch_do_physinfo if in xen/include/public/sysctl.h
the XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK is protected by __aarch64__ or __arm__ .
Do you agree on that?
>
> Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 08/12] xen/physinfo: encode Arm SVE vector length in arch_capabilities
2023-03-30 10:41 ` Luca Fancellu
@ 2023-03-30 10:49 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2023-03-30 10:49 UTC (permalink / raw)
To: Luca Fancellu
Cc: Bertrand Marquis, Wei Chen, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
xen-devel
On 30.03.2023 12:41, Luca Fancellu wrote:
>
>
>> On 28 Mar 2023, at 11:14, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 27.03.2023 12:59, Luca Fancellu wrote:
>>> --- a/xen/arch/arm/arm64/sve.c
>>> +++ b/xen/arch/arm/arm64/sve.c
>>> @@ -124,3 +124,15 @@ int __init sve_parse_dom0_param(const char *str_begin, const char *str_end)
>>> {
>>> return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve);
>>> }
>>> +
>>> +void sve_arch_cap_physinfo(uint32_t *arch_capabilities)
>>> +{
>>> + if ( cpu_has_sve )
>>> + {
>>> + /* Vector length is divided by 128 to save some space */
>>> + uint32_t sve_vl = MASK_INSR(sve_encode_vl(get_sys_vl_len()),
>>> + XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
>>> +
>>> + *arch_capabilities |= sve_vl;
>>> + }
>>> +}
>>
>> I'm again wondering why a separate function is needed, when everything
>> that's needed is ...
>>
>>> --- a/xen/arch/arm/sysctl.c
>>> +++ b/xen/arch/arm/sysctl.c
>>> @@ -11,11 +11,14 @@
>>> #include <xen/lib.h>
>>> #include <xen/errno.h>
>>> #include <xen/hypercall.h>
>>> +#include <asm/arm64/sve.h>
>>
>> ... becoming available here for use ...
>>
>>> #include <public/sysctl.h>
>>>
>>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>>> {
>>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
>>> +
>>> + sve_arch_cap_physinfo(&pi->arch_capabilities);
>>
>> ... here. That would be even more so if, like suggested before,
>> get_sys_vl_len() returned 0 when !cpu_has_sve.
>
> I’ve had a look on this, I can do everything In arch_do_physinfo if in xen/include/public/sysctl.h
> the XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK is protected by __aarch64__ or __arm__ .
>
> Do you agree on that?
I don't see the connection, but guarding the #define in the public header
looks not only okay, but even desirable to me.
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 09/12] tools: add physinfo arch_capabilities handling for Arm
2023-03-27 10:59 ` [PATCH v4 09/12] tools: add physinfo arch_capabilities handling for Arm Luca Fancellu
@ 2023-03-30 16:49 ` Anthony PERARD
2023-04-04 7:25 ` Luca Fancellu
0 siblings, 1 reply; 33+ messages in thread
From: Anthony PERARD @ 2023-03-30 16:49 UTC (permalink / raw)
To: Luca Fancellu
Cc: xen-devel, bertrand.marquis, wei.chen, George Dunlap,
Nick Rosbrook, Wei Liu, Juergen Gross, Christian Lindig,
David Scott, Marek Marczykowski-Górecki, Christian Lindig
On Mon, Mar 27, 2023 at 11:59:41AM +0100, Luca Fancellu wrote:
> ---
> tools/golang/xenlight/helpers.gen.go | 2 ++
> tools/golang/xenlight/types.gen.go | 1 +
> tools/include/arm-arch-capabilities.h | 33 +++++++++++++++++++++++++
Could you move that new file into "tools/include/xen-tools/", where
"common-macros.h" is. The top-dir "tools/include" already has a mixture
of installed and internal headers, but most of them are installed. So
the fairly recent "xen-tools" dir which have been introduced to share
macros at build time seems more appropriate for another built-time
macro.
> tools/include/xen-tools/common-macros.h | 2 ++
>
> diff --git a/tools/include/arm-arch-capabilities.h b/tools/include/arm-arch-capabilities.h
> new file mode 100644
> index 000000000000..46e876651052
> --- /dev/null
> +++ b/tools/include/arm-arch-capabilities.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2023 ARM Ltd.
> + */
> +
> +#ifndef ARM_ARCH_CAPABILITIES_H
> +#define ARM_ARCH_CAPABILITIES_H
> +
> +/* Tell the Xen public headers we are a user-space tools build. */
> +#ifndef __XEN_TOOLS__
> +#define __XEN_TOOLS__ 1
Somehow, this doesn't seems appropriate in this header. This macro
should instead be set on the command line. Any reason you've added this
in the header?
> +#endif
> +
> +#include <stdint.h>
> +#include <xen/sysctl.h>
> +
> +#include <xen-tools/common-macros.h>
> +
> +static inline
> +unsigned int arch_capabilities_arm_sve(unsigned int arch_capabilities)
> +{
> +#if defined(__aarch64__)
> + unsigned int sve_vl = MASK_EXTR(arch_capabilities,
> + XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
> +
> + /* Vector length is divided by 128 before storing it in arch_capabilities */
> + return sve_vl * 128U;
> +#else
> + return 0;
> +#endif
> +}
> +
> +#endif /* ARM_ARCH_CAPABILITIES_H */
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index c10292e0d7e3..fd31dacf7d5a 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -1133,6 +1133,7 @@ libxl_physinfo = Struct("physinfo", [
> ("cap_vpmu", bool),
> ("cap_gnttab_v1", bool),
> ("cap_gnttab_v2", bool),
> + ("arch_capabilities", uint32),
This additional field needs a new LIBXL_HAVE_ macro in "libxl.h".
> ], dir=DIR_OUT)
>
> libxl_connectorinfo = Struct("connectorinfo", [
> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
> index 35901c2d63b6..254d3b5dccd2 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -7,6 +7,7 @@
> #define PY_SSIZE_T_CLEAN
> #include <Python.h>
> #define XC_WANT_COMPAT_MAP_FOREIGN_API
> +#include <arm-arch-capabilities.h>
Could you add this header ...
> #include <xenctrl.h>
> #include <xenguest.h>
> #include <fcntl.h>
> @@ -22,8 +23,6 @@
> #include <xen/hvm/hvm_info_table.h>
> #include <xen/hvm/params.h>
>
> -#include <xen-tools/common-macros.h>
> -
... here, instead?
Also, I think #include common-macros, can stay.
> /* Needed for Python versions earlier than 2.3. */
> #ifndef PyMODINIT_FUNC
> #define PyMODINIT_FUNC DL_EXPORT(void)
> @@ -897,7 +896,7 @@ static PyObject *pyxc_physinfo(XcObject *self)
> if ( p != virt_caps )
> *(p-1) = '\0';
>
> - return Py_BuildValue("{s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s,s:s}",
> + return Py_BuildValue("{s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s,s:s,s:i}",
> "nr_nodes", pinfo.nr_nodes,
> "threads_per_core", pinfo.threads_per_core,
> "cores_per_socket", pinfo.cores_per_socket,
> @@ -907,7 +906,10 @@ static PyObject *pyxc_physinfo(XcObject *self)
> "scrub_memory", pages_to_kib(pinfo.scrub_pages),
> "cpu_khz", pinfo.cpu_khz,
> "hw_caps", cpu_cap,
> - "virt_caps", virt_caps);
> + "virt_caps", virt_caps,
> + "arm_sve_vl",
> + arch_capabilities_arm_sve(pinfo.arch_capabilities)
arch_capabilities_arm_sve() returns an "unsigned int", but the format is
"i", which seems to be an "int". Shouldn't the format be "I" instead?
https://docs.python.org/3/c-api/arg.html#c.Py_BuildValue
> + );
> }
>
> static PyObject *pyxc_getcpuinfo(XcObject *self, PyObject *args, PyObject *kwds)
> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
> index 712b7638b013..bf18ba2449ef 100644
> --- a/tools/xl/xl_info.c
> +++ b/tools/xl/xl_info.c
> @@ -14,6 +14,7 @@
>
> #define _GNU_SOURCE
>
> +#include <arm-arch-capabilities.h>
Any reason reason to have this header first?
I feel like private headers should come after public ones, so here, this
include would be added between <libxlutil.h> and "xl.h".
> #include <fcntl.h>
> #include <inttypes.h>
> #include <stdlib.h>
> @@ -224,6 +225,13 @@ static void output_physinfo(void)
> info.cap_gnttab_v2 ? " gnttab-v2" : ""
> );
>
> + /* Print arm SVE vector length only on ARM platforms */
> +#if defined(__aarch64__)
> + maybe_printf("arm_sve_vector_length : %u\n",
> + arch_capabilities_arm_sve(info.arch_capabilities)
> + );
> +#endif
> +
> vinfo = libxl_get_version_info(ctx);
> if (vinfo) {
> i = (1 << 20) / vinfo->pagesize;
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 10/12] xen/tools: add sve parameter in XL configuration
2023-03-27 10:59 ` [PATCH v4 10/12] xen/tools: add sve parameter in XL configuration Luca Fancellu
@ 2023-03-31 14:23 ` Anthony PERARD
2023-04-04 13:48 ` Luca Fancellu
0 siblings, 1 reply; 33+ messages in thread
From: Anthony PERARD @ 2023-03-31 14:23 UTC (permalink / raw)
To: Luca Fancellu
Cc: xen-devel, bertrand.marquis, wei.chen, Wei Liu, George Dunlap,
Nick Rosbrook, Juergen Gross, George Dunlap
On Mon, Mar 27, 2023 at 11:59:42AM +0100, Luca Fancellu wrote:
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 10f37990be57..adf48fe8ac1d 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -2952,6 +2952,17 @@ Currently, only the "sbsa_uart" model is supported for ARM.
>
> =back
>
> +=item B<sve="NUMBER">
> +
> +To enable SVE, user must specify a number different from zero, maximum 2048 and
> +multiple of 128. That value will be the maximum number of SVE registers bits
> +that the hypervisor will impose to this guest. If the platform has a lower
Maybe start by describing what the "sve" value is before imposing
limits. Maybe something like:
Set the maximum vector length that a guest's Scalable Vector
Extension (SVE) can use. Or disable it by specifying 0, the default.
Value needs to be a multiple of 128, with a maximum of 2048 or the
maximum supported by the platform.
Would this, or something like that be a good explanation of the "sve"
configuration option?
> +supported bits value, then the domain creation will fail.
> +A value equal to zero is the default and it means this guest is not allowed to
> +use SVE.
> +
> +=back
> +
> =head3 x86
>
> =over 4
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index ddc7b2a15975..16a49031fd51 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -211,6 +211,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> return ERROR_FAIL;
> }
>
> + config->arch.sve_vl = d_config->b_info.arch_arm.sve;
This truncate a 16bit value into an 8bit value, I think you should check
that the value can actually fit.
And maybe check `d_config->b_info.arch_arm.sve` value here instead of
`xl` as commented later.
> +
> return 0;
> }
>
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index fd31dacf7d5a..ef4a8358e54e 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -690,6 +690,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>
> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> ("vuart", libxl_vuart_type),
> + ("sve", uint16),
I wonder if renaming "sve" to "sve_vl" here would make sense, seeing
that "sve_vl" is actually used in other places.
> ])),
> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> ])),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 1f6f47daf4e1..3cbc23b36952 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -12,6 +12,7 @@
> * GNU Lesser General Public License for more details.
> */
>
> +#include <arm-arch-capabilities.h>
Could you add this headers after public ones?
> #include <ctype.h>
> #include <inttypes.h>
> #include <limits.h>
> @@ -1312,8 +1313,6 @@ void parse_config_data(const char *config_source,
> exit(EXIT_FAILURE);
> }
>
> - libxl_physinfo_dispose(&physinfo);
> -
> config= xlu_cfg_init(stderr, config_source);
> if (!config) {
> fprintf(stderr, "Failed to allocate for configuration\n");
> @@ -2887,6 +2886,29 @@ skip_usbdev:
> }
> }
>
> + if (!xlu_cfg_get_long (config, "sve", &l, 0)) {
> + unsigned int arm_sve_vl =
> + arch_capabilities_arm_sve(physinfo.arch_capabilities);
> + if (!arm_sve_vl) {
> + fprintf(stderr, "SVE is not supported by the platform\n");
> + exit(-ERROR_FAIL);
"ERROR_FAIL" is a "libxl_error", instead exit with "EXIT_FAILURE".
> + } else if (((l % 128) != 0) || (l > 2048)) {
> + fprintf(stderr,
> + "Invalid sve value: %ld. Needs to be <= 2048 and multiple"
> + " of 128\n", l);
> + exit(-ERROR_FAIL);
> + } else if (l > arm_sve_vl) {
> + fprintf(stderr,
> + "Invalid sve value: %ld. Platform supports up to %u bits\n",
> + l, arm_sve_vl);
> + exit(-ERROR_FAIL);
> + }
> + /* Vector length is divided by 128 in domain configuration struct */
That's wrong, beside this comment there's nothing that say that
`b_info->arch_arm.sve` needs to have a value divided by 128.
`b_info->arch_arm.sve` is just of type uint16_t (libxl_type.idl).
BTW, "tools/xl" (xl) use just one user of "tools/libs/light" (libxl), so
it's possible that other users would set `sve` to a value that haven't
been checked. So I think all the check that the `sve` value is correct
could be done in libxl instead.
> + b_info->arch_arm.sve = l / 128U;
> + }
> +
> + libxl_physinfo_dispose(&physinfo);
> +
> parse_vkb_list(config, d_config);
>
> d_config->virtios = NULL;
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 09/12] tools: add physinfo arch_capabilities handling for Arm
2023-03-30 16:49 ` Anthony PERARD
@ 2023-04-04 7:25 ` Luca Fancellu
0 siblings, 0 replies; 33+ messages in thread
From: Luca Fancellu @ 2023-04-04 7:25 UTC (permalink / raw)
To: Anthony PERARD
Cc: Xen-devel, Bertrand Marquis, Wei Chen, George Dunlap,
Nick Rosbrook, Wei Liu, Juergen Gross, Christian Lindig,
David Scott, Marek Marczykowski-Górecki, Christian Lindig
Hi Anthony,
Thank you for your review
> On 30 Mar 2023, at 17:49, Anthony PERARD <anthony.perard@citrix.com> wrote:
>
> On Mon, Mar 27, 2023 at 11:59:41AM +0100, Luca Fancellu wrote:
>> ---
>> tools/golang/xenlight/helpers.gen.go | 2 ++
>> tools/golang/xenlight/types.gen.go | 1 +
>> tools/include/arm-arch-capabilities.h | 33 +++++++++++++++++++++++++
>
> Could you move that new file into "tools/include/xen-tools/", where
> "common-macros.h" is. The top-dir "tools/include" already has a mixture
> of installed and internal headers, but most of them are installed. So
> the fairly recent "xen-tools" dir which have been introduced to share
> macros at build time seems more appropriate for another built-time
> macro.
Yes I’ll do
>
>> tools/include/xen-tools/common-macros.h | 2 ++
>>
>> diff --git a/tools/include/arm-arch-capabilities.h b/tools/include/arm-arch-capabilities.h
>> new file mode 100644
>> index 000000000000..46e876651052
>> --- /dev/null
>> +++ b/tools/include/arm-arch-capabilities.h
>> @@ -0,0 +1,33 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2023 ARM Ltd.
>> + */
>> +
>> +#ifndef ARM_ARCH_CAPABILITIES_H
>> +#define ARM_ARCH_CAPABILITIES_H
>> +
>> +/* Tell the Xen public headers we are a user-space tools build. */
>> +#ifndef __XEN_TOOLS__
>> +#define __XEN_TOOLS__ 1
>
> Somehow, this doesn't seems appropriate in this header. This macro
> should instead be set on the command line. Any reason you've added this
> in the header?
I’ve added that because sysctl.h is doing this:
#if !defined(__XEN__) && !defined(__XEN_TOOLS__)
#error "sysctl operations are intended for use by node control tools only"
#endif
But I’ve not checked if the macro is already passed through the build system, I’ll
try and I’ll remove it if it’s the case
>
>> +#endif
>> +
>> +#include <stdint.h>
>> +#include <xen/sysctl.h>
>> +
>> +#include <xen-tools/common-macros.h>
>> +
>> +static inline
>> +unsigned int arch_capabilities_arm_sve(unsigned int arch_capabilities)
>> +{
>> +#if defined(__aarch64__)
>> + unsigned int sve_vl = MASK_EXTR(arch_capabilities,
>> + XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
>> +
>> + /* Vector length is divided by 128 before storing it in arch_capabilities */
>> + return sve_vl * 128U;
>> +#else
>> + return 0;
>> +#endif
>> +}
>> +
>> +#endif /* ARM_ARCH_CAPABILITIES_H */
>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>> index c10292e0d7e3..fd31dacf7d5a 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -1133,6 +1133,7 @@ libxl_physinfo = Struct("physinfo", [
>> ("cap_vpmu", bool),
>> ("cap_gnttab_v1", bool),
>> ("cap_gnttab_v2", bool),
>> + ("arch_capabilities", uint32),
>
> This additional field needs a new LIBXL_HAVE_ macro in "libxl.h".
I’ll add
>
>> ], dir=DIR_OUT)
>>
>> libxl_connectorinfo = Struct("connectorinfo", [
>> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
>> index 35901c2d63b6..254d3b5dccd2 100644
>> --- a/tools/python/xen/lowlevel/xc/xc.c
>> +++ b/tools/python/xen/lowlevel/xc/xc.c
>> @@ -7,6 +7,7 @@
>> #define PY_SSIZE_T_CLEAN
>> #include <Python.h>
>> #define XC_WANT_COMPAT_MAP_FOREIGN_API
>> +#include <arm-arch-capabilities.h>
>
> Could you add this header ...
>
>> #include <xenctrl.h>
>> #include <xenguest.h>
>> #include <fcntl.h>
>> @@ -22,8 +23,6 @@
>> #include <xen/hvm/hvm_info_table.h>
>> #include <xen/hvm/params.h>
>>
>> -#include <xen-tools/common-macros.h>
>> -
>
> ... here, instead?
>
> Also, I think #include common-macros, can stay.
Ok I’ll do the modifications
>
>> /* Needed for Python versions earlier than 2.3. */
>> #ifndef PyMODINIT_FUNC
>> #define PyMODINIT_FUNC DL_EXPORT(void)
>> @@ -897,7 +896,7 @@ static PyObject *pyxc_physinfo(XcObject *self)
>> if ( p != virt_caps )
>> *(p-1) = '\0';
>>
>> - return Py_BuildValue("{s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s,s:s}",
>> + return Py_BuildValue("{s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s,s:s,s:i}",
>> "nr_nodes", pinfo.nr_nodes,
>> "threads_per_core", pinfo.threads_per_core,
>> "cores_per_socket", pinfo.cores_per_socket,
>> @@ -907,7 +906,10 @@ static PyObject *pyxc_physinfo(XcObject *self)
>> "scrub_memory", pages_to_kib(pinfo.scrub_pages),
>> "cpu_khz", pinfo.cpu_khz,
>> "hw_caps", cpu_cap,
>> - "virt_caps", virt_caps);
>> + "virt_caps", virt_caps,
>> + "arm_sve_vl",
>> + arch_capabilities_arm_sve(pinfo.arch_capabilities)
>
> arch_capabilities_arm_sve() returns an "unsigned int", but the format is
> "i", which seems to be an "int". Shouldn't the format be "I" instead?
>
> https://docs.python.org/3/c-api/arg.html#c.Py_BuildValue
Yes you are right, I’ll change it
>
>> + );
>> }
>>
>> static PyObject *pyxc_getcpuinfo(XcObject *self, PyObject *args, PyObject *kwds)
>> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
>> index 712b7638b013..bf18ba2449ef 100644
>> --- a/tools/xl/xl_info.c
>> +++ b/tools/xl/xl_info.c
>> @@ -14,6 +14,7 @@
>>
>> #define _GNU_SOURCE
>>
>> +#include <arm-arch-capabilities.h>
>
> Any reason reason to have this header first?
> I feel like private headers should come after public ones, so here, this
> include would be added between <libxlutil.h> and "xl.h".
Ok I’ll move it
>
>> #include <fcntl.h>
>> #include <inttypes.h>
>> #include <stdlib.h>
>> @@ -224,6 +225,13 @@ static void output_physinfo(void)
>> info.cap_gnttab_v2 ? " gnttab-v2" : ""
>> );
>>
>> + /* Print arm SVE vector length only on ARM platforms */
>> +#if defined(__aarch64__)
>> + maybe_printf("arm_sve_vector_length : %u\n",
>> + arch_capabilities_arm_sve(info.arch_capabilities)
>> + );
>> +#endif
>> +
>> vinfo = libxl_get_version_info(ctx);
>> if (vinfo) {
>> i = (1 << 20) / vinfo->pagesize;
>
> Thanks,
>
> --
> Anthony PERARD
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 10/12] xen/tools: add sve parameter in XL configuration
2023-03-31 14:23 ` Anthony PERARD
@ 2023-04-04 13:48 ` Luca Fancellu
2023-04-05 15:14 ` Anthony PERARD
0 siblings, 1 reply; 33+ messages in thread
From: Luca Fancellu @ 2023-04-04 13:48 UTC (permalink / raw)
To: Anthony PERARD
Cc: Xen-devel, Bertrand Marquis, Wei Chen, Wei Liu, George Dunlap,
Nick Rosbrook, Juergen Gross, George Dunlap
Hi Anthony,
Thanks for your review
> On 31 Mar 2023, at 15:23, Anthony PERARD <anthony.perard@citrix.com> wrote:
>
> On Mon, Mar 27, 2023 at 11:59:42AM +0100, Luca Fancellu wrote:
>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
>> index 10f37990be57..adf48fe8ac1d 100644
>> --- a/docs/man/xl.cfg.5.pod.in
>> +++ b/docs/man/xl.cfg.5.pod.in
>> @@ -2952,6 +2952,17 @@ Currently, only the "sbsa_uart" model is supported for ARM.
>>
>> =back
>>
>> +=item B<sve="NUMBER">
>> +
>> +To enable SVE, user must specify a number different from zero, maximum 2048 and
>> +multiple of 128. That value will be the maximum number of SVE registers bits
>> +that the hypervisor will impose to this guest. If the platform has a lower
>
> Maybe start by describing what the "sve" value is before imposing
> limits. Maybe something like:
>
> Set the maximum vector length that a guest's Scalable Vector
> Extension (SVE) can use. Or disable it by specifying 0, the default.
>
> Value needs to be a multiple of 128, with a maximum of 2048 or the
> maximum supported by the platform.
>
> Would this, or something like that be a good explanation of the "sve"
> configuration option?
Yes I can change it, a need to do it anyway because I think also here, the suggestion
From Jan can apply and we could pass a negative value that means “max VL supported
by the platform"
>
>> +supported bits value, then the domain creation will fail.
>> +A value equal to zero is the default and it means this guest is not allowed to
>> +use SVE.
>> +
>> +=back
>> +
>> =head3 x86
>>
>> =over 4
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index ddc7b2a15975..16a49031fd51 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -211,6 +211,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>> return ERROR_FAIL;
>> }
>>
>> + config->arch.sve_vl = d_config->b_info.arch_arm.sve;
>
> This truncate a 16bit value into an 8bit value, I think you should check
> that the value can actually fit.
>
> And maybe check `d_config->b_info.arch_arm.sve` value here instead of
> `xl` as commented later.
Yes I can do it, one question, can I use here xc_physinfo to retrieve the maximum
Vector length from arch_capabilities?
I mean, is there a better way or I can go for that?
>
>> +
>> return 0;
>> }
>>
>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>> index fd31dacf7d5a..ef4a8358e54e 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -690,6 +690,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>
>> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>> ("vuart", libxl_vuart_type),
>> + ("sve", uint16),
>
> I wonder if renaming "sve" to "sve_vl" here would make sense, seeing
> that "sve_vl" is actually used in other places.
Yes I can rename it as sve_vl, I will also change the type to “integer"
>
>> ])),
>> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>> ])),
>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>> index 1f6f47daf4e1..3cbc23b36952 100644
>> --- a/tools/xl/xl_parse.c
>> +++ b/tools/xl/xl_parse.c
>> @@ -12,6 +12,7 @@
>> * GNU Lesser General Public License for more details.
>> */
>>
>> +#include <arm-arch-capabilities.h>
>
> Could you add this headers after public ones?
Yes
>
>> #include <ctype.h>
>> #include <inttypes.h>
>> #include <limits.h>
>> @@ -1312,8 +1313,6 @@ void parse_config_data(const char *config_source,
>> exit(EXIT_FAILURE);
>> }
>>
>> - libxl_physinfo_dispose(&physinfo);
>> -
>> config= xlu_cfg_init(stderr, config_source);
>> if (!config) {
>> fprintf(stderr, "Failed to allocate for configuration\n");
>> @@ -2887,6 +2886,29 @@ skip_usbdev:
>> }
>> }
>>
>> + if (!xlu_cfg_get_long (config, "sve", &l, 0)) {
>> + unsigned int arm_sve_vl =
>> + arch_capabilities_arm_sve(physinfo.arch_capabilities);
>> + if (!arm_sve_vl) {
>> + fprintf(stderr, "SVE is not supported by the platform\n");
>> + exit(-ERROR_FAIL);
>
> "ERROR_FAIL" is a "libxl_error", instead exit with "EXIT_FAILURE".
Ok I will use the right type
>
>> + } else if (((l % 128) != 0) || (l > 2048)) {
>> + fprintf(stderr,
>> + "Invalid sve value: %ld. Needs to be <= 2048 and multiple"
>> + " of 128\n", l);
>> + exit(-ERROR_FAIL);
>> + } else if (l > arm_sve_vl) {
>> + fprintf(stderr,
>> + "Invalid sve value: %ld. Platform supports up to %u bits\n",
>> + l, arm_sve_vl);
>> + exit(-ERROR_FAIL);
>> + }
>> + /* Vector length is divided by 128 in domain configuration struct */
>
> That's wrong, beside this comment there's nothing that say that
> `b_info->arch_arm.sve` needs to have a value divided by 128.
> `b_info->arch_arm.sve` is just of type uint16_t (libxl_type.idl).
>
> BTW, "tools/xl" (xl) use just one user of "tools/libs/light" (libxl), so
> it's possible that other users would set `sve` to a value that haven't
> been checked. So I think all the check that the `sve` value is correct
> could be done in libxl instead.
Sure I will do that
>
>
>> + b_info->arch_arm.sve = l / 128U;
>> + }
>> +
>> + libxl_physinfo_dispose(&physinfo);
>> +
>> parse_vkb_list(config, d_config);
>>
>> d_config->virtios = NULL;
>
> Thanks,
>
> --
> Anthony PERARD
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 10/12] xen/tools: add sve parameter in XL configuration
2023-04-04 13:48 ` Luca Fancellu
@ 2023-04-05 15:14 ` Anthony PERARD
2023-04-06 5:53 ` Luca Fancellu
0 siblings, 1 reply; 33+ messages in thread
From: Anthony PERARD @ 2023-04-05 15:14 UTC (permalink / raw)
To: Luca Fancellu
Cc: Xen-devel, Bertrand Marquis, Wei Chen, Wei Liu, George Dunlap,
Nick Rosbrook, Juergen Gross, George Dunlap
On Tue, Apr 04, 2023 at 01:48:34PM +0000, Luca Fancellu wrote:
> > On 31 Mar 2023, at 15:23, Anthony PERARD <anthony.perard@citrix.com> wrote:
> >
> > On Mon, Mar 27, 2023 at 11:59:42AM +0100, Luca Fancellu wrote:
> >> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> >> index 10f37990be57..adf48fe8ac1d 100644
> >> --- a/docs/man/xl.cfg.5.pod.in
> >> +++ b/docs/man/xl.cfg.5.pod.in
> >> @@ -2952,6 +2952,17 @@ Currently, only the "sbsa_uart" model is supported for ARM.
> >>
> >> =back
> >>
> >> +=item B<sve="NUMBER">
> >> +
> >> +To enable SVE, user must specify a number different from zero, maximum 2048 and
> >> +multiple of 128. That value will be the maximum number of SVE registers bits
> >> +that the hypervisor will impose to this guest. If the platform has a lower
> >
> > Maybe start by describing what the "sve" value is before imposing
> > limits. Maybe something like:
> >
> > Set the maximum vector length that a guest's Scalable Vector
> > Extension (SVE) can use. Or disable it by specifying 0, the default.
> >
> > Value needs to be a multiple of 128, with a maximum of 2048 or the
> > maximum supported by the platform.
> >
> > Would this, or something like that be a good explanation of the "sve"
> > configuration option?
>
> Yes I can change it, a need to do it anyway because I think also here, the suggestion
> From Jan can apply and we could pass a negative value that means “max VL supported
> by the platform"
Well, it's a config file, not a C ABI, so max allowed here doesn't have to be
spelled "-1", it could also be "max", "max-allowed",
"max-size-supported", ... So fill free deviate from the restricted C
ABI. But "-1" works as long as it's the only allowed negative number.
> >
> >> +supported bits value, then the domain creation will fail.
> >> +A value equal to zero is the default and it means this guest is not allowed to
> >> +use SVE.
> >> +
> >> +=back
> >> +
> >> =head3 x86
> >>
> >> =over 4
> >> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> >> index ddc7b2a15975..16a49031fd51 100644
> >> --- a/tools/libs/light/libxl_arm.c
> >> +++ b/tools/libs/light/libxl_arm.c
> >> @@ -211,6 +211,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >> return ERROR_FAIL;
> >> }
> >>
> >> + config->arch.sve_vl = d_config->b_info.arch_arm.sve;
> >
> > This truncate a 16bit value into an 8bit value, I think you should check
> > that the value can actually fit.
> >
> > And maybe check `d_config->b_info.arch_arm.sve` value here instead of
> > `xl` as commented later.
>
> Yes I can do it, one question, can I use here xc_physinfo to retrieve the maximum
> Vector length from arch_capabilities?
> I mean, is there a better way or I can go for that?
Yeah, there might be a "better" way. I think me suggestion to check the
sve value here was wrong. I still want to have it checked in libxl, but
it might be better to do that in the previous step, that is
"libxl__domain_config_setdefault". libxl__arch_domain_build_info_setdefault()
will have `physinfo` so you won't have to call xc_physinfo().
Regarding the default, libxl is capable of selecting a good set of
option, depending on the underling hardware. So is it possible that in
the future we would want to enable SVE by default? If that's even a
remote possibility, the current API wouldn't allow it because we have
"default" and "disable" been the same.
Since we want to add a max VL supported option, maybe we want to
separate the default and disable options. So we could keep the
single field `libxl_domain_build_info.arch_arm.sve` and have for example
"-1" for max-supported and "-2" for disabled, while "0" mean default.
Or alternatively, add an extra field libxl_defbool (can be
default/true/false) and keep the second one for VL. (That extra
"disabled" option would only be for libxl, for xl we can keep "sve=0"
mean disable, and the missing option just mean default.)
In any case, libxl__arch_domain_build_info_setdefault() will check
`b_info.arch_arm.sve` and set it to the value that can given to Xen. And
as of now, libxl__arch_domain_prepare_config() will just copy the value
from `b_info` to `config`. (I guess that last step _prepare_config()
could still do the division by 128.)
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 10/12] xen/tools: add sve parameter in XL configuration
2023-04-05 15:14 ` Anthony PERARD
@ 2023-04-06 5:53 ` Luca Fancellu
0 siblings, 0 replies; 33+ messages in thread
From: Luca Fancellu @ 2023-04-06 5:53 UTC (permalink / raw)
To: Anthony PERARD
Cc: Xen-devel, Bertrand Marquis, Wei Chen, Wei Liu, George Dunlap,
Nick Rosbrook, Juergen Gross, George Dunlap
Hi Anthony,
>>
>> Yes I can change it, a need to do it anyway because I think also here, the suggestion
>> From Jan can apply and we could pass a negative value that means “max VL supported
>> by the platform"
>
> Well, it's a config file, not a C ABI, so max allowed here doesn't have to be
> spelled "-1", it could also be "max", "max-allowed",
> "max-size-supported", ... So fill free deviate from the restricted C
> ABI. But "-1" works as long as it's the only allowed negative number.
Yes while working on the patch I’ve found that I could declare this type in Libxl:
libxl_sve_type = Enumeration("sve_type", [
(0, "disabled"),
(128, "128"),
(256, "256"),
(384, "384"),
(512, "512"),
(640, "640"),
(768, "768"),
(896, "896"),
(1024, "1024"),
(1152, "1152"),
(1280, "1280"),
(1408, "1408"),
(1536, "1536"),
(1664, "1664"),
(1792, "1792"),
(1920, "1920"),
(2048, "2048"),
(-1, "hw")
], init_val = "LIBXL_SVE_TYPE_DISABLED”)
So that in xl I can just use libxl_sve_type_from_string
>
>>>
>>>> +supported bits value, then the domain creation will fail.
>>>> +A value equal to zero is the default and it means this guest is not allowed to
>>>> +use SVE.
>>>> +
>>>> +=back
>>>> +
>>>> =head3 x86
>>>>
>>>> =over 4
>>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>>> index ddc7b2a15975..16a49031fd51 100644
>>>> --- a/tools/libs/light/libxl_arm.c
>>>> +++ b/tools/libs/light/libxl_arm.c
>>>> @@ -211,6 +211,8 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>>> return ERROR_FAIL;
>>>> }
>>>>
>>>> + config->arch.sve_vl = d_config->b_info.arch_arm.sve;
>>>
>>> This truncate a 16bit value into an 8bit value, I think you should check
>>> that the value can actually fit.
>>>
>>> And maybe check `d_config->b_info.arch_arm.sve` value here instead of
>>> `xl` as commented later.
>>
>> Yes I can do it, one question, can I use here xc_physinfo to retrieve the maximum
>> Vector length from arch_capabilities?
>> I mean, is there a better way or I can go for that?
>
> Yeah, there might be a "better" way. I think me suggestion to check the
> sve value here was wrong. I still want to have it checked in libxl, but
> it might be better to do that in the previous step, that is
> "libxl__domain_config_setdefault". libxl__arch_domain_build_info_setdefault()
> will have `physinfo` so you won't have to call xc_physinfo().
Right, I’ve seen it before but I was unsure if it was the right way, now that you
suggested it, I will go for that.
Thank you.
Cheers,
Luca
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2023-04-06 5:54 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-27 10:59 [PATCH v4 00/12] SVE feature for arm guests Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 01/12] xen/arm: enable SVE extension for Xen Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 02/12] xen/arm: add SVE vector length field to the domain Luca Fancellu
2023-03-28 9:36 ` Jan Beulich
2023-03-29 10:01 ` Luca Fancellu
2023-03-29 10:32 ` Jan Beulich
2023-03-29 10:45 ` Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 03/12] xen/arm: Expose SVE feature to the guest Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 04/12] xen/arm: add SVE exception class handling Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 05/12] arm/sve: save/restore SVE context switch Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 06/12] xen/common: add dom0 xen command line argument for Arm Luca Fancellu
2023-03-28 9:44 ` Jan Beulich
2023-03-27 10:59 ` [PATCH v4 07/12] xen: enable Dom0 to use SVE feature Luca Fancellu
2023-03-28 10:08 ` Jan Beulich
2023-03-29 11:48 ` Luca Fancellu
2023-03-29 11:54 ` Jan Beulich
2023-03-29 12:06 ` Luca Fancellu
2023-03-29 12:24 ` Jan Beulich
2023-03-29 14:33 ` Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 08/12] xen/physinfo: encode Arm SVE vector length in arch_capabilities Luca Fancellu
2023-03-28 10:14 ` Jan Beulich
2023-03-30 10:41 ` Luca Fancellu
2023-03-30 10:49 ` Jan Beulich
2023-03-27 10:59 ` [PATCH v4 09/12] tools: add physinfo arch_capabilities handling for Arm Luca Fancellu
2023-03-30 16:49 ` Anthony PERARD
2023-04-04 7:25 ` Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 10/12] xen/tools: add sve parameter in XL configuration Luca Fancellu
2023-03-31 14:23 ` Anthony PERARD
2023-04-04 13:48 ` Luca Fancellu
2023-04-05 15:14 ` Anthony PERARD
2023-04-06 5:53 ` Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 11/12] xen/arm: add sve property for dom0less domUs Luca Fancellu
2023-03-27 10:59 ` [PATCH v4 12/12] xen/changelog: Add SVE and "dom0" options to the changelog for Arm Luca Fancellu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.