Message-ID: <aRu2uC4VVazB_SfV@skinsburskii.localdomain>
Date: Mon, 17 Nov 2025 15:58:48 -0800
From: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
To: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
Cc: linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
	wei.liu@...nel.org, mhklinux@...look.com, kys@...rosoft.com,
	haiyangz@...rosoft.com, decui@...rosoft.com, longli@...rosoft.com,
	prapal@...ux.microsoft.com, mrathor@...ux.microsoft.com,
	muislam@...rosoft.com, anrayabh@...ux.microsoft.com,
	Jinank Jain <jinankjain@...rosoft.com>
Subject: Re: [PATCH v4] mshv: Extend create partition ioctl to support cpu
 features

On Tue, Nov 11, 2025 at 03:19:54PM -0800, Nuno Das Neves wrote:
> From: Muminul Islam <muislam@...rosoft.com>
> 
> The existing mshv create partition ioctl does not provide a way to
> specify which cpu features are enabled in the guest. Instead, it
> attempts to enable all features and those that are not supported are
> silently disabled by the hypervisor.
> 
> This was done to reduce unnecessary complexity and is sufficient for
> many cases. However, new scenarios require fine-grained control over
> these features.
> 
> Define a new mshv_create_partition_v2 structure which supports
> passing the disabled processor and xsave feature bits through to the
> create partition hypercall directly.
> 
> Introduce a new flag MSHV_PT_BIT_CPU_AND_XSAVE_FEATURES which enables
> the new structure. If unset, the original mshv_create_partition struct
> is used, with the old behavior of enabling all features.
> 
> Co-developed-by: Jinank Jain <jinankjain@...rosoft.com>
> Signed-off-by: Jinank Jain <jinankjain@...rosoft.com>
> Signed-off-by: Muminul Islam <muislam@...rosoft.com>
> Signed-off-by: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
> ---
> Changes in v4:
> - Change BIT() to BIT_ULL() [Michael Kelley]
> - Enforce pt_num_cpu_fbanks == MSHV_NUM_CPU_FEATURES_BANKS and expect
>   that number to never change. In future, additional processor banks
>   will be settable as 'early' partition properties. Remove redundant
>   code that set default values for unspecified banks [Michael Kelley]
> - Set xsave features to 0 on arm64 [Michael Kelley]
> - Add clarifying comments in a few places
> 
> Changes in v3:
> - Remove the new cpu features definitions in hvhdk.h, and retain the
>   old behavior of enabling all features for the old struct. For the v2
>   struct, still disable unspecified feature banks, since that makes it
>   robust to future extensions.
> - Amend comments and commit message to reflect the above
> - Fix unused variable on arm64 [kernel test robot]
> 
> Changes in v2:
> - Fix exposure of CONFIG_X86_64 to uapi [kernel test robot]
> - Fix compilation issue on arm64 [kernel test robot]
> ---
>  drivers/hv/mshv_root_main.c | 113 +++++++++++++++++++++++++++++-------
>  include/uapi/linux/mshv.h   |  34 +++++++++++
>  2 files changed, 126 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index d542a0143bb8..9f9438289b60 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -1900,43 +1900,114 @@ add_partition(struct mshv_partition *partition)
>  	return 0;
>  }
>  
> -static long
> -mshv_ioctl_create_partition(void __user *user_arg, struct device *module_dev)
> +static_assert(MSHV_NUM_CPU_FEATURES_BANKS ==
> +	      HV_PARTITION_PROCESSOR_FEATURES_BANKS);
> +
> +static long mshv_ioctl_process_pt_flags(void __user *user_arg, u64 *pt_flags,
> +					struct hv_partition_creation_properties *cr_props,
> +					union hv_partition_isolation_properties *isol_props)
>  {
> -	struct mshv_create_partition args;
> -	u64 creation_flags;
> -	struct hv_partition_creation_properties creation_properties = {};
> -	union hv_partition_isolation_properties isolation_properties = {};
> -	struct mshv_partition *partition;
> -	struct file *file;
> -	int fd;
> -	long ret;
> +	int i;
> +	struct mshv_create_partition_v2 args;
> +	union hv_partition_processor_features *disabled_procs;
> +	union hv_partition_processor_xsave_features *disabled_xsave;
>  
> -	if (copy_from_user(&args, user_arg, sizeof(args)))
> +	/* First, copy v1 struct in case user is on previous versions */
> +	if (copy_from_user(&args, user_arg,
> +			   sizeof(struct mshv_create_partition)))
>  		return -EFAULT;
>  
>  	if ((args.pt_flags & ~MSHV_PT_FLAGS_MASK) ||
>  	    args.pt_isolation >= MSHV_PT_ISOLATION_COUNT)
>  		return -EINVAL;
>  
> +	disabled_procs = &cr_props->disabled_processor_features;
> +	disabled_xsave = &cr_props->disabled_processor_xsave_features;
> +
> +	/* Check if user provided newer struct with feature fields */
> +	if (args.pt_flags & BIT_ULL(MSHV_PT_BIT_CPU_AND_XSAVE_FEATURES)) {
> +		if (copy_from_user(&args, user_arg, sizeof(args)))
> +			return -EFAULT;
> +
> +		if (args.pt_num_cpu_fbanks != MSHV_NUM_CPU_FEATURES_BANKS ||
> +		    mshv_field_nonzero(args, pt_rsvd) ||
> +		    mshv_field_nonzero(args, pt_rsvd1))
> +			return -EINVAL;
> +
> +		/*
> +		 * Note this assumes MSHV_NUM_CPU_FEATURES_BANKS will never
> +		 * change and equals HV_PARTITION_PROCESSOR_FEATURES_BANKS
> +		 * (i.e. 2).
> +		 *
> +		 * Further banks (index >= 2) will be modifiable as 'early'
> +		 * properties via the set partition property hypercall.
> +		 */
> +		for (i = 0; i < HV_PARTITION_PROCESSOR_FEATURES_BANKS; i++)
> +			disabled_procs->as_uint64[i] = args.pt_cpu_fbanks[i];
> +
> +#if IS_ENABLED(CONFIG_X86_64)
> +		disabled_xsave->as_uint64 = args.pt_disabled_xsave;
> +#else
> +		/*
> +		 * In practice this field is ignored on arm64, but safer to
> +		 * zero it in case it is ever used.
> +		 */
> +		disabled_xsave->as_uint64 = 0;

Why not explicitly treat non-zero value as invalid here instead?
Isn't it always better than implicitly (and silently) zeroing it?

> +
> +		if (mshv_field_nonzero(args, pt_rsvd2))
> +			return -EINVAL;
> +#endif
> +	} else {
> +		/*
> +		 * v1 behavior: try to enable everything. The hypervisor will
> +		 * disable features that are not supported. The banks can be
> +		 * queried via the get partition property hypercall.
> +		 */
> +		for (i = 0; i < HV_PARTITION_PROCESSOR_FEATURES_BANKS; i++)
> +			disabled_procs->as_uint64[i] = 0;
> +
> +		disabled_xsave->as_uint64 = 0;
> +	}
> +
>  	/* Only support EXO partitions */
> -	creation_flags = HV_PARTITION_CREATION_FLAG_EXO_PARTITION |
> -			 HV_PARTITION_CREATION_FLAG_INTERCEPT_MESSAGE_PAGE_ENABLED;
> +	*pt_flags = HV_PARTITION_CREATION_FLAG_EXO_PARTITION |
> +		    HV_PARTITION_CREATION_FLAG_INTERCEPT_MESSAGE_PAGE_ENABLED;
> +
> +	if (args.pt_flags & BIT_ULL(MSHV_PT_BIT_LAPIC))
> +		*pt_flags |= HV_PARTITION_CREATION_FLAG_LAPIC_ENABLED;
> +	if (args.pt_flags & BIT_ULL(MSHV_PT_BIT_X2APIC))
> +		*pt_flags |= HV_PARTITION_CREATION_FLAG_X2APIC_CAPABLE;
> +	if (args.pt_flags & BIT_ULL(MSHV_PT_BIT_GPA_SUPER_PAGES))
> +		*pt_flags |= HV_PARTITION_CREATION_FLAG_GPA_SUPER_PAGES_ENABLED;
>  
> -	if (args.pt_flags & BIT(MSHV_PT_BIT_LAPIC))
> -		creation_flags |= HV_PARTITION_CREATION_FLAG_LAPIC_ENABLED;
> -	if (args.pt_flags & BIT(MSHV_PT_BIT_X2APIC))
> -		creation_flags |= HV_PARTITION_CREATION_FLAG_X2APIC_CAPABLE;
> -	if (args.pt_flags & BIT(MSHV_PT_BIT_GPA_SUPER_PAGES))
> -		creation_flags |= HV_PARTITION_CREATION_FLAG_GPA_SUPER_PAGES_ENABLED;
> +	isol_props->as_uint64 = 0;

These properties are missing the nested one.
I'd recommend squeezing that change in this patch

Thanks,
Stanislav

>  
>  	switch (args.pt_isolation) {
>  	case MSHV_PT_ISOLATION_NONE:
> -		isolation_properties.isolation_type =
> -			HV_PARTITION_ISOLATION_TYPE_NONE;
> +		isol_props->isolation_type = HV_PARTITION_ISOLATION_TYPE_NONE;
>  		break;
>  	}
>  
> +	return 0;
> +}
> +
> +static long
> +mshv_ioctl_create_partition(void __user *user_arg, struct device *module_dev)
> +{
> +	u64 creation_flags;
> +	struct hv_partition_creation_properties creation_properties;
> +	union hv_partition_isolation_properties isolation_properties;
> +	struct mshv_partition *partition;
> +	struct file *file;
> +	int fd;
> +	long ret;
> +
> +	ret = mshv_ioctl_process_pt_flags(user_arg, &creation_flags,
> +					  &creation_properties,
> +					  &isolation_properties);
> +	if (ret)
> +		return ret;
> +
>  	partition = kzalloc(sizeof(*partition), GFP_KERNEL);
>  	if (!partition)
>  		return -ENOMEM;
> diff --git a/include/uapi/linux/mshv.h b/include/uapi/linux/mshv.h
> index 876bfe4e4227..cf904f3aa201 100644
> --- a/include/uapi/linux/mshv.h
> +++ b/include/uapi/linux/mshv.h
> @@ -26,6 +26,7 @@ enum {
>  	MSHV_PT_BIT_LAPIC,
>  	MSHV_PT_BIT_X2APIC,
>  	MSHV_PT_BIT_GPA_SUPER_PAGES,
> +	MSHV_PT_BIT_CPU_AND_XSAVE_FEATURES,
>  	MSHV_PT_BIT_COUNT,
>  };
>  
> @@ -41,6 +42,8 @@ enum {
>   * @pt_flags: Bitmask of 1 << MSHV_PT_BIT_*
>   * @pt_isolation: MSHV_PT_ISOLATION_*
>   *
> + * This is the initial/v1 version for backward compatibility.
> + *
>   * Returns a file descriptor to act as a handle to a guest partition.
>   * At this point the partition is not yet initialized in the hypervisor.
>   * Some operations must be done with the partition in this state, e.g. setting
> @@ -52,6 +55,37 @@ struct mshv_create_partition {
>  	__u64 pt_isolation;
>  };
>  
> +#define MSHV_NUM_CPU_FEATURES_BANKS 2
> +
> +/**
> + * struct mshv_create_partition_v2
> + *
> + * This is extended version of the above initial MSHV_CREATE_PARTITION
> + * ioctl and allows for following additional parameters:
> + *
> + * @pt_num_cpu_fbanks: Must be set to MSHV_NUM_CPU_FEATURES_BANKS.
> + * @pt_cpu_fbanks: Disabled processor feature banks array.
> + * @pt_disabled_xsave: Disabled xsave feature bits.
> + *
> + * pt_cpu_fbanks and pt_disabled_xsave are passed through as-is to the create
> + * partition hypercall.
> + *
> + * Returns : same as above original mshv_create_partition
> + */
> +struct mshv_create_partition_v2 {
> +	__u64 pt_flags;
> +	__u64 pt_isolation;
> +	__u16 pt_num_cpu_fbanks;
> +	__u8  pt_rsvd[6];		/* MBZ */
> +	__u64 pt_cpu_fbanks[MSHV_NUM_CPU_FEATURES_BANKS];
> +	__u64 pt_rsvd1[2];		/* MBZ */
> +#if defined(__x86_64__)
> +	__u64 pt_disabled_xsave;
> +#else
> +	__u64 pt_rsvd2;			/* MBZ */
> +#endif
> +} __packed;
> +
>  /* /dev/mshv */
>  #define MSHV_CREATE_PARTITION	_IOW(MSHV_IOCTL, 0x00, struct mshv_create_partition)
>  
> -- 
> 2.34.1

Powered by blists - more mailing lists