Re: [PATCH v2 07/14] KVM: arm64: Handle endianness in read helper for emulated PTW

[Marc Zyngier <maz@kernel.org>]

On Mon, 17 Nov 2025 22:43:18 +0000,
Oliver Upton <oupton@kernel.org> wrote:
> 
> Implementing FEAT_HAFDBS means adding another descriptor accessor that
> needs to deal with the guest-configured endianness. Prepare by moving
> the endianness handling into the read accessor and out of the main body
> of the S1/S2 PTWs.
> 
> Signed-off-by: Oliver Upton <oupton@kernel.org>
> ---
>  arch/arm64/kvm/at.c     | 25 +++++++++++++++++++------
>  arch/arm64/kvm/nested.c | 30 +++++++++++++++++-------------
>  2 files changed, 36 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> index be26d5aa668c..a295a37dd3b1 100644
> --- a/arch/arm64/kvm/at.c
> +++ b/arch/arm64/kvm/at.c
> @@ -362,6 +362,24 @@ static int setup_s1_walk(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  	return -EFAULT;
>  }
>  
> +static int kvm_read_s1_desc(struct kvm_vcpu *vcpu, u64 pa, u64 *desc,
> +			    struct s1_walk_info *wi)
> +{
> +	u64 val;
> +	int r;
> +
> +	r = kvm_read_guest(vcpu->kvm, pa, &val, sizeof(val));
> +	if (r)
> +		return r;
> +
> +	if (wi->be)
> +		*desc = be64_to_cpu((__force __be64)val);
> +	else
> +		*desc = le64_to_cpu((__force __le64)val);
> +
> +	return 0;
> +}
> +
>  static int walk_s1(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  		   struct s1_walk_result *wr, u64 va)
>  {
> @@ -414,17 +432,12 @@ static int walk_s1(struct kvm_vcpu *vcpu, struct s1_walk_info *wi,
>  				return ret;
>  		}
>  
> -		ret = kvm_read_guest(vcpu->kvm, ipa, &desc, sizeof(desc));
> +		ret = kvm_read_s1_desc(vcpu, ipa, &desc, wi);
>  		if (ret) {
>  			fail_s1_walk(wr, ESR_ELx_FSC_SEA_TTW(level), false);
>  			return ret;
>  		}
>  
> -		if (wi->be)
> -			desc = be64_to_cpu((__force __be64)desc);
> -		else
> -			desc = le64_to_cpu((__force __le64)desc);
> -
>  		/* Invalid descriptor */
>  		if (!(desc & BIT(0)))
>  			goto transfault;
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index cf4b24e04a1a..10e68aab3d2a 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -198,13 +198,26 @@ static int check_output_size(struct s2_walk_info *wi, phys_addr_t output)
>  	return 0;
>  }
>  
> -static int read_guest_s2_desc(phys_addr_t pa, u64 *desc, void *data)
> +static int read_guest_s2_desc(phys_addr_t pa, u64 *desc, struct s2_walk_info *wi)
>  {
> -	struct kvm_vcpu *vcpu = data;
> +	struct kvm_vcpu *vcpu = wi->data;

How about getting rid of this nonsense wi->data business? With the
reader abstraction removed, this really serves no purpose. And
aligning the read_guest_s[12]_desc() prototype. See below for an
untested suggestion.

Thanks,

	M.

diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
index dbd3900e8bff6..e063061f21aad 100644
--- a/arch/arm64/kvm/nested.c
+++ b/arch/arm64/kvm/nested.c
@@ -124,7 +124,6 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)
 }
 
 struct s2_walk_info {
-	void		*data;
 	u64		baddr;
 	unsigned int	max_oa_bits;
 	unsigned int	pgshift;
@@ -199,9 +198,9 @@ static int check_output_size(struct s2_walk_info *wi, phys_addr_t output)
 	return 0;
 }
 
-static int read_guest_s2_desc(phys_addr_t pa, u64 *desc, struct s2_walk_info *wi)
+static int read_guest_s2_desc(struct kvm_vcpu *vcpu,
+			      phys_addr_t pa, u64 *desc, struct s2_walk_info *wi)
 {
-	struct kvm_vcpu *vcpu = wi->data;
 	u64 val;
 	int r;
 
@@ -221,11 +220,10 @@ static int read_guest_s2_desc(phys_addr_t pa, u64 *desc, struct s2_walk_info *wi
 	return 0;
 }
 
-static int swap_guest_s2_desc(phys_addr_t pa, u64 old, u64 new,
+static int swap_guest_s2_desc(struct kvm_vcpu *vcpu,
+			      phys_addr_t pa, u64 old, u64 new,
 			      struct s2_walk_info *wi)
 {
-	struct kvm_vcpu *vcpu = wi->data;
-
 	if (wi->be) {
 		old = cpu_to_be64(old);
 		new = cpu_to_be64(new);
@@ -244,7 +242,7 @@ static int swap_guest_s2_desc(phys_addr_t pa, u64 old, u64 new,
  *
  * Must be called with the kvm->srcu read lock held
  */
-static int walk_nested_s2_pgd(phys_addr_t ipa,
+static int walk_nested_s2_pgd(struct kvm_vcpu *vcpu, phys_addr_t ipa,
 			      struct s2_walk_info *wi, struct kvm_s2_trans *out)
 {
 	int first_block_level, level, stride, input_size, base_lower_bound;
@@ -295,7 +293,7 @@ static int walk_nested_s2_pgd(phys_addr_t ipa,
 			>> (addr_bottom - 3);
 
 		paddr = base_addr | index;
-		ret = read_guest_s2_desc(paddr, &desc, wi);
+		ret = read_guest_s2_desc(vcpu, paddr, &desc, wi);
 		if (ret < 0)
 			return ret;
 
@@ -349,7 +347,7 @@ static int walk_nested_s2_pgd(phys_addr_t ipa,
 		new_desc |= KVM_PTE_LEAF_ATTR_LO_S2_AF;
 
 	if (new_desc != desc) {
-		ret = swap_guest_s2_desc(paddr, desc, new_desc, wi);
+		ret = swap_guest_s2_desc(vcpu, paddr, desc, new_desc, wi);
 		if (ret)
 			return ret;
 
@@ -410,14 +408,13 @@ int kvm_walk_nested_s2(struct kvm_vcpu *vcpu, phys_addr_t gipa,
 	if (!vcpu_has_nv(vcpu))
 		return 0;
 
-	wi.data = vcpu;
 	wi.baddr = vcpu_read_sys_reg(vcpu, VTTBR_EL2);
 
 	vtcr_to_walk_info(vtcr, &wi);
 
 	wi.be = vcpu_read_sys_reg(vcpu, SCTLR_EL2) & SCTLR_ELx_EE;
 
-	ret = walk_nested_s2_pgd(gipa, &wi, result);
+	ret = walk_nested_s2_pgd(vcpu, gipa, &wi, result);
 	if (ret)
 		result->esr |= (kvm_vcpu_get_esr(vcpu) & ~ESR_ELx_FSC);
 

-- 
Jazz isn't dead. It just smells funny.