[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 2/8] xen/arm: add sve_vl_bits field to domain
- To: Luca Fancellu <Luca.Fancellu@xxxxxxx>
- From: Julien Grall <julien@xxxxxxx>
- Date: Fri, 13 Jan 2023 09:06:38 +0000
- Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Wei Chen <Wei.Chen@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>
- Delivery-date: Fri, 13 Jan 2023 09:06:44 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
Hi Luca,
On 12/01/2023 10:54, Luca Fancellu wrote:
On 11 Jan 2023, at 17:27, Julien Grall <julien@xxxxxxx> wrote:
Hi Luca,
On 11/01/2023 14:38, Luca Fancellu wrote:
Add sve_vl_bits field to arch_domain and xen_arch_domainconfig
structure, 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.
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.
When the guest is allowed to use SVE, the zcr_el2 register is
updated on context switch to restict the domain on the allowed
number of bits chosen, this value is the minimum among the chosen
value and the platform supported value.
Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
---
xen/arch/arm/arm64/sve.c | 9 ++++++
xen/arch/arm/domain.c | 45 ++++++++++++++++++++++++++++
xen/arch/arm/include/asm/arm64/sve.h | 12 ++++++++
xen/arch/arm/include/asm/domain.h | 6 ++++
xen/include/public/arch-arm.h | 2 ++
xen/include/public/domctl.h | 2 +-
6 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/arm64/sve.c b/xen/arch/arm/arm64/sve.c
index 326389278292..b7695834f4ba 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>
@@ -36,3 +37,11 @@ register_t vl_to_zcr(uint16_t vl)
{
return ((vl / SVE_VL_MULTIPLE_VAL) - 1U) & ZCR_ELx_LEN_MASK;
}
+
+/* Get the system sanitized value for VL in bits */
+uint16_t 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 8ea3843ea8e8..27f38729302b 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>
@@ -183,6 +184,11 @@ static void ctxt_switch_to(struct vcpu *n)
WRITE_SYSREG(n->arch.cptr_el2, CPTR_EL2);
+#ifdef CONFIG_ARM64_SVE
+ if ( is_sve_domain(n->domain) )
+ WRITE_SYSREG(n->arch.zcr_el2, ZCR_EL2);
+#endif
I would actually expect that is_sve_domain() returns false when the SVE is not
enabled. So can we simply remove the #ifdef?
I was tricked by it too, the arm32 build will fail because it doesn’t know
ZCR_EL2
Ok. In which case, I think we should move the call to sve_restore_state().
+
/* VFP */
vfp_restore_state(n);
@@ -551,6 +557,11 @@ 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.zcr_el2 = vl_to_zcr(v->domain->arch.sve_vl_bits);
+ }
v->arch.hcr_el2 = get_default_hcr_flags();
@@ -595,6 +606,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 = config->arch.sve_vl_bits;
if ( (config->flags & ~flags_optional) != flags_required )
{
@@ -603,6 +615,36 @@ 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;
+
+ 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;
+ }
+ /*
+ * get_sys_vl_len() is the common safe value among all cpus, so if the
+ * value specified by the user is above that value, use the safe value
+ * instead.
+ */
+ zcr_max_bits = get_sys_vl_len();
+ if ( sve_vl_bits > zcr_max_bits )
+ {
+ config->arch.sve_vl_bits = zcr_max_bits;
+ dprintk(XENLOG_INFO,
+ "SVE vector length lowered to %u, safe value among CPUs\n",
+ zcr_max_bits);
+ }
I don't think Xen should "downgrade" the value. Instead, this should be the
decision from the tools. So we want to find a different way to reproduce the value
(Andrew may have some ideas here as he was looking at it).
Can you explain this in more details?
I would extend XEN_SYSCTL_physinfo to return the maximum SVE vecto
length supported by the Hardware.
Libxl would then read the value and compare to what the user requested.
This would then be up to the toolstack to decide what to do.
By the tools you mean xl?
I think libxl should do strict checking. If we also want to reduce what
the user requested, then this part should be in xl.
This would be ok for DomUs, but how to do it for Dom0?
Then we should fail to create dom0 and say the vector-length requested
is not supported.
Cheers,
--
Julien Grall
|
