The hypercalls test currently defines its own *_BMAP_BIT_MAX macros to
define the last valid feature bit for each bitmap firmware register.
However, since these definitions are already present in the uapi header,
kvm.h, as *_BMAP_BIT_COUNT, and would help to keep the test updated as
features grow, use these instead.
No functional change intended.
Signed-off-by: Raghavendra Rao Ananta <[email protected]>
---
tools/testing/selftests/kvm/aarch64/hypercalls.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/hypercalls.c b/tools/testing/selftests/kvm/aarch64/hypercalls.c
index 41e0210b7a5e..dea019ec4dd9 100644
--- a/tools/testing/selftests/kvm/aarch64/hypercalls.c
+++ b/tools/testing/selftests/kvm/aarch64/hypercalls.c
@@ -18,20 +18,15 @@
#define FW_REG_ULIMIT_VAL(max_feat_bit) (GENMASK(max_feat_bit, 0))
-/* Last valid bits of the bitmapped firmware registers */
-#define KVM_REG_ARM_STD_BMAP_BIT_MAX 0
-#define KVM_REG_ARM_STD_HYP_BMAP_BIT_MAX 0
-#define KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_MAX 1
-
struct kvm_fw_reg_info {
uint64_t reg; /* Register definition */
uint64_t max_feat_bit; /* Bit that represents the upper limit of the feature-map */
};
-#define FW_REG_INFO(r) \
- { \
- .reg = r, \
- .max_feat_bit = r##_BIT_MAX, \
+#define FW_REG_INFO(r) \
+ { \
+ .reg = r, \
+ .max_feat_bit = r##_BIT_COUNT - 1, \
}
static const struct kvm_fw_reg_info fw_reg_info[] = {
--
2.36.0.512.ge40c2bad7a-goog
Hi Raghavendra,
On Wed, May 04, 2022 at 06:44:15PM +0000, Raghavendra Rao Ananta wrote:
> The hypercalls test currently defines its own *_BMAP_BIT_MAX macros to
> define the last valid feature bit for each bitmap firmware register.
> However, since these definitions are already present in the uapi header,
> kvm.h, as *_BMAP_BIT_COUNT, and would help to keep the test updated as
> features grow, use these instead.
LOL, looks like I lost that one in the end! Still, the fact that you're
patching the selftest highlights the fact that there is a nonzero chance
of userspace using this value incorrectly expecting it to hold true
across all kernels.
Since this is the route going forward can we please consider documenting
the fact that _BIT_COUNT *will* change and is not stable between kernel
versions. Bad UAPI expectations could throw a wrench into this entire
plan we've hatched for preserving hypercall ABI.
Just a warning at the end of the register documentation would suffice.
> No functional change intended.
>
> Signed-off-by: Raghavendra Rao Ananta <[email protected]>
Besides considering the above hazard:
Reviewed-by: Oliver Upton <[email protected]>
On 5/5/22 14:04, Marc Zyngier wrote:
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index e523bb6eac67..3cde9f958eee 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -342,6 +342,10 @@ struct kvm_arm_copy_mte_tags {
>
> enum {
> KVM_REG_ARM_STD_BIT_TRNG_V1_0 = 0,
> + /*
> + * KVM_REG_ARM_STD_BMAP_BIT_COUNT will vary as new services
> + * are added, and is explicitely not part of the ABI.
> + */
> KVM_REG_ARM_STD_BMAP_BIT_COUNT,
> };
That seems like a bad idea. Perhaps you can wrap it in #ifdef
__KERNEL_OR_SELFTESTS__? I can't find any prior art.
Paolo
On Wed, 04 May 2022 20:58:42 +0100,
Oliver Upton <[email protected]> wrote:
>
> Hi Raghavendra,
>
> On Wed, May 04, 2022 at 06:44:15PM +0000, Raghavendra Rao Ananta wrote:
> > The hypercalls test currently defines its own *_BMAP_BIT_MAX macros to
> > define the last valid feature bit for each bitmap firmware register.
> > However, since these definitions are already present in the uapi header,
> > kvm.h, as *_BMAP_BIT_COUNT, and would help to keep the test updated as
> > features grow, use these instead.
>
> LOL, looks like I lost that one in the end! Still, the fact that you're
> patching the selftest highlights the fact that there is a nonzero chance
> of userspace using this value incorrectly expecting it to hold true
> across all kernels.
>
> Since this is the route going forward can we please consider documenting
> the fact that _BIT_COUNT *will* change and is not stable between kernel
> versions. Bad UAPI expectations could throw a wrench into this entire
> plan we've hatched for preserving hypercall ABI.
>
> Just a warning at the end of the register documentation would suffice.
Maybe something in the kvm.h file as well, as the includes are often
distributed without the kernel documentation. Something like:
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index e523bb6eac67..3cde9f958eee 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -342,6 +342,10 @@ struct kvm_arm_copy_mte_tags {
enum {
KVM_REG_ARM_STD_BIT_TRNG_V1_0 = 0,
+ /*
+ * KVM_REG_ARM_STD_BMAP_BIT_COUNT will vary as new services
+ * are added, and is explicitely not part of the ABI.
+ */
KVM_REG_ARM_STD_BMAP_BIT_COUNT,
};
@@ -349,6 +353,10 @@ enum {
enum {
KVM_REG_ARM_STD_HYP_BIT_PV_TIME = 0,
+ /*
+ * KVM_REG_ARM_STD_HYP_BMAP_BIT_COUNT will vary as new
+ * services are added, and is explicitely not part of the ABI.
+ */
KVM_REG_ARM_STD_HYP_BMAP_BIT_COUNT,
};
@@ -357,6 +365,10 @@ enum {
enum {
KVM_REG_ARM_VENDOR_HYP_BIT_FUNC_FEAT = 0,
KVM_REG_ARM_VENDOR_HYP_BIT_PTP = 1,
+ /*
+ * KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_COUNT will vary as new
+ * services are added, and is explicitely not part of the ABI.
+ */
KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_COUNT,
};
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On 2022-05-05 14:32, Paolo Bonzini wrote:
> On 5/5/22 14:04, Marc Zyngier wrote:
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>> b/arch/arm64/include/uapi/asm/kvm.h
>> index e523bb6eac67..3cde9f958eee 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -342,6 +342,10 @@ struct kvm_arm_copy_mte_tags {
>> enum {
>> KVM_REG_ARM_STD_BIT_TRNG_V1_0 = 0,
>> + /*
>> + * KVM_REG_ARM_STD_BMAP_BIT_COUNT will vary as new services
>> + * are added, and is explicitely not part of the ABI.
>> + */
>> KVM_REG_ARM_STD_BMAP_BIT_COUNT,
>> };
>
> That seems like a bad idea. Perhaps you can wrap it in #ifdef
> __KERNEL_OR_SELFTESTS__? I can't find any prior art.
Yeah. I've dropped this patch and sprinkled a bunch of
'#ifdef __KERNEL__' instead. We can revisit this later,
or hack the test to temporarily define __KERNEL__, which
is just as bad...
M.
--
Jazz is not dead. It just smells funny...