Re: [RFC PATCH 05/28] arm64: RME: Define the user ABI

[Zhi Wang]

On Wed, 1 Mar 2023 11:54:34 +0000
Steven Price <steven.price@xxxxxxx> wrote:

> On 13/02/2023 16:04, Zhi Wang wrote:
> > On Fri, 27 Jan 2023 11:29:09 +0000
> > Steven Price <steven.price@xxxxxxx> wrote:
> > 
> >> There is one (multiplexed) CAP which can be used to create, populate and
> >> then activate the realm.
> >>
> >> Signed-off-by: Steven Price <steven.price@xxxxxxx>
> >> ---
> >>  Documentation/virt/kvm/api.rst    |  1 +
> >>  arch/arm64/include/uapi/asm/kvm.h | 63 +++++++++++++++++++++++++++++++
> >>  include/uapi/linux/kvm.h          |  2 +
> >>  3 files changed, 66 insertions(+)
> >>
> >> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> >> index 0dd5d8733dd5..f1a59d6fb7fc 100644
> >> --- a/Documentation/virt/kvm/api.rst
> >> +++ b/Documentation/virt/kvm/api.rst
> >> @@ -4965,6 +4965,7 @@ Recognised values for feature:
> >>  
> >>    =====      ===========================================
> >>    arm64      KVM_ARM_VCPU_SVE (requires KVM_CAP_ARM_SVE)
> >> +  arm64      KVM_ARM_VCPU_REC (requires KVM_CAP_ARM_RME)
> >>    =====      ===========================================
> >>  
> >>  Finalizes the configuration of the specified vcpu feature.
> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> >> index a7a857f1784d..fcc0b8dce29b 100644
> >> --- a/arch/arm64/include/uapi/asm/kvm.h
> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
> >> @@ -109,6 +109,7 @@ struct kvm_regs {
> >>  #define KVM_ARM_VCPU_SVE		4 /* enable SVE for this CPU */
> >>  #define KVM_ARM_VCPU_PTRAUTH_ADDRESS	5 /* VCPU uses address authentication */
> >>  #define KVM_ARM_VCPU_PTRAUTH_GENERIC	6 /* VCPU uses generic authentication */
> >> +#define KVM_ARM_VCPU_REC		7 /* VCPU REC state as part of Realm */
> >>  
> >>  struct kvm_vcpu_init {
> >>  	__u32 target;
> >> @@ -401,6 +402,68 @@ enum {
> >>  #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
> >>  #define   KVM_DEV_ARM_ITS_CTRL_RESET		4
> >>  
> >> +/* KVM_CAP_ARM_RME on VM fd */
> >> +#define KVM_CAP_ARM_RME_CONFIG_REALM		0
> >> +#define KVM_CAP_ARM_RME_CREATE_RD		1
> >> +#define KVM_CAP_ARM_RME_INIT_IPA_REALM		2
> >> +#define KVM_CAP_ARM_RME_POPULATE_REALM		3
> >> +#define KVM_CAP_ARM_RME_ACTIVATE_REALM		4
> >> +
> > 
> > It is a little bit confusing here. These seems more like 'commands' not caps.
> > Will leave more comments after reviewing the later patches.
> 
> Sorry for the slow response. Thank you for your review - I hope to post
> a new version of this series (rebased on 6.3-rc1) in the coming weeks
> with your comments addressed.
> 

Hi:

No worries. I spent most of my time on closing the review of TDX/SNP
series recently while stopped at patch 16 of this series. I will try to
finish the rest of this series this week.

I am glad if my efforts help and more reviewers can smoothly jump in
later.
 
> They are indeed commands - and using caps is a bit of a hack. The
> benefit here is that all the Realm commands are behind the one
> KVM_CAP_ARM_RME.
> 
> The options I can see are:
> 
> a) What I've got here - (ab)using KVM_ENABLE_CAP to perform commands.
> 
> b) Add new ioctls for each of the above stages (so 5 new ioctls on top
> of the CAP for discovery). With any future extensions requiring new ioctls.
> 
> c) Add a single new multiplexing ioctl (along with the CAP for discovery).
> 
> I'm not massively keen on defining a new multiplexing scheme (c), but
> equally (b) seems like it's burning through ioctl numbers. Which led me
> to stick with (a) which at least keeps the rebasing simple (there's only
> the one CAP number which could conflict) and there's already a
> multiplexing scheme.
> 
> But I'm happy to change if there's consensus a different approach would
> be preferable.
> 

Let's see if others have different opinions.

My coin goes to b as it is better to respect "what it is, make it explicit
and clear" when coming to define the UABI. Ioctl number is for UABI. If
it is going to burn out, IMHO, we need to find another way, perhaps another
fd to group those ioctls, like KVM.

1. a) seems abusing the usage of the cap. for sure, the benefit is obvious.
2. c) seems hiding the details, which saves the ioctl numbers, but it didn't
actually help a lot on the complexity and might end up with another bunch
of "command code".

> Thanks,
> 
> Steve
> 
> >> +#define KVM_CAP_ARM_RME_MEASUREMENT_ALGO_SHA256		0
> >> +#define KVM_CAP_ARM_RME_MEASUREMENT_ALGO_SHA512		1
> >> +
> >> +#define KVM_CAP_ARM_RME_RPV_SIZE 64
> >> +
> >> +/* List of configuration items accepted for KVM_CAP_ARM_RME_CONFIG_REALM */
> >> +#define KVM_CAP_ARM_RME_CFG_RPV			0
> >> +#define KVM_CAP_ARM_RME_CFG_HASH_ALGO		1
> >> +#define KVM_CAP_ARM_RME_CFG_SVE			2
> >> +#define KVM_CAP_ARM_RME_CFG_DBG			3
> >> +#define KVM_CAP_ARM_RME_CFG_PMU			4
> >> +
> >> +struct kvm_cap_arm_rme_config_item {
> >> +	__u32 cfg;
> >> +	union {
> >> +		/* cfg == KVM_CAP_ARM_RME_CFG_RPV */
> >> +		struct {
> >> +			__u8	rpv[KVM_CAP_ARM_RME_RPV_SIZE];
> >> +		};
> >> +
> >> +		/* cfg == KVM_CAP_ARM_RME_CFG_HASH_ALGO */
> >> +		struct {
> >> +			__u32	hash_algo;
> >> +		};
> >> +
> >> +		/* cfg == KVM_CAP_ARM_RME_CFG_SVE */
> >> +		struct {
> >> +			__u32	sve_vq;
> >> +		};
> >> +
> >> +		/* cfg == KVM_CAP_ARM_RME_CFG_DBG */
> >> +		struct {
> >> +			__u32	num_brps;
> >> +			__u32	num_wrps;
> >> +		};
> >> +
> >> +		/* cfg == KVM_CAP_ARM_RME_CFG_PMU */
> >> +		struct {
> >> +			__u32	num_pmu_cntrs;
> >> +		};
> >> +		/* Fix the size of the union */
> >> +		__u8	reserved[256];
> >> +	};
> >> +};
> >> +
> >> +struct kvm_cap_arm_rme_populate_realm_args {
> >> +	__u64 populate_ipa_base;
> >> +	__u64 populate_ipa_size;
> >> +};
> >> +
> >> +struct kvm_cap_arm_rme_init_ipa_args {
> >> +	__u64 init_ipa_base;
> >> +	__u64 init_ipa_size;
> >> +};
> >> +
> >>  /* Device Control API on vcpu fd */
> >>  #define KVM_ARM_VCPU_PMU_V3_CTRL	0
> >>  #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
> >> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> >> index 20522d4ba1e0..fec1909e8b73 100644
> >> --- a/include/uapi/linux/kvm.h
> >> +++ b/include/uapi/linux/kvm.h
> >> @@ -1176,6 +1176,8 @@ struct kvm_ppc_resize_hpt {
> >>  #define KVM_CAP_S390_PROTECTED_ASYNC_DISABLE 224
> >>  #define KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP 225
> >>  
> >> +#define KVM_CAP_ARM_RME 300 // FIXME: Large number to prevent conflicts
> >> +
> >>  #ifdef KVM_CAP_IRQ_ROUTING
> >>  
> >>  struct kvm_irq_routing_irqchip {
> > 
>