Re: [PATCH 10/25] KVM: arm64: nv: Turn encoding ranges into discrete XArray stores

[Marc Zyngier <maz@kernel.org>]

On Tue, 23 Jan 2024 16:37:25 +0000,
Joey Gouly <joey.gouly@arm.com> wrote:
> 
> On Mon, Jan 22, 2024 at 08:18:37PM +0000, Marc Zyngier wrote:
> > In order to be able to store different values for member of an
> > encoding range, replace xa_store_range() calls with discrete
> > xa_store() calls and an encoding iterator.
> > 
> > We end-up using a bit more memory, but we gain some flexibility
> > that we will make use of shortly.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/emulate-nested.c | 31 ++++++++++++++++++++++++-------
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
> > index ef46c2e45307..59622636b723 100644
> > --- a/arch/arm64/kvm/emulate-nested.c
> > +++ b/arch/arm64/kvm/emulate-nested.c
> > @@ -1757,6 +1757,28 @@ static __init void print_nv_trap_error(const struct encoding_to_trap_config *tc,
> >  		err);
> >  }
> >  
> > +static u32 encoding_next(u32 encoding)
> > +{
> > +	u8 op0, op1, crn, crm, op2;
> > +
> > +	op0 = sys_reg_Op0(encoding);
> > +	op1 = sys_reg_Op1(encoding);
> > +	crn = sys_reg_CRn(encoding);
> > +	crm = sys_reg_CRm(encoding);
> > +	op2 = sys_reg_Op2(encoding);
> > +
> > +	if (op2 < Op2_mask)
> > +		return sys_reg(op0, op1, crn, crm, op2 + 1);
> > +	if (crm < CRm_mask)
> > +		return sys_reg(op0, op1, crn, crm + 1, 0);
> > +	if (crn < CRn_mask)
> > +		return sys_reg(op0, op1, crn + 1, 0, 0);
> > +	if (op1 < Op1_mask)
> > +		return sys_reg(op0, op1 + 1, 0, 0, 0);
> > +
> > +	return sys_reg(op0 + 1, 0, 0, 0, 0);
> > +}
> 
> I like this function, aesthetically pleasing!

Glad you like the colour! :D

> 
> > +
> >  int __init populate_nv_trap_config(void)
> >  {
> >  	int ret = 0;
> > @@ -1775,13 +1797,8 @@ int __init populate_nv_trap_config(void)
> >  			ret = -EINVAL;
> >  		}
> >  
> > -		if (cgt->encoding != cgt->end) {
> > -			prev = xa_store_range(&sr_forward_xa,
> > -					      cgt->encoding, cgt->end,
> > -					      xa_mk_value(cgt->tc.val),
> > -					      GFP_KERNEL);
> > -		} else {
> > -			prev = xa_store(&sr_forward_xa, cgt->encoding,
> > +		for (u32 enc = cgt->encoding; enc <= cgt->end; enc = encoding_next(enc)) {
> > +			prev = xa_store(&sr_forward_xa, enc,
> >  					xa_mk_value(cgt->tc.val), GFP_KERNEL);
> >  			if (prev && !xa_is_err(prev)) {
> >  				ret = -EINVAL;
> 
> The error handling looks a bit weird here, the full context:
> 
>                 for (u32 enc = cgt->encoding; enc <= cgt->end; enc = encoding_next(enc)) {
>                         prev = xa_store(&sr_forward_xa, enc,
>                                         xa_mk_value(cgt->tc.val), GFP_KERNEL);
>                         if (prev && !xa_is_err(prev)) {
>                                 ret = -EINVAL;
>                                 print_nv_trap_error(cgt, "Duplicate CGT", ret);
>                         }
>                 }
>
>                 if (xa_is_err(prev)) {
>                         ret = xa_err(prev);
>                         print_nv_trap_error(cgt, "Failed CGT insertion", ret);
>                 }  
> 
> I would maybe expect some 'goto's after setting ret? It looks like the ret
> would still be returned properly at the end of the function at least.  We also
> don't check the return value of xa_store() in the encoding_to_fgt loop further
> down, which seems worse as that could affect VMs if some encodings failed to be
> stored for some reason.

The lack of goto is on purpose. Getting the tables right is tedious
when you can't collect multiple errors at once and stop on the first
one. Which is why I opted for this scheme where 'ret' only gets
written on error.

However, the error handling is pretty lax indeed. I have this in
store, on top of the current patch:

diff --git a/arch/arm64/kvm/emulate-nested.c b/arch/arm64/kvm/emulate-nested.c
index 5c0f81b6e55c..f2cf0fbf27eb 100644
--- a/arch/arm64/kvm/emulate-nested.c
+++ b/arch/arm64/kvm/emulate-nested.c
@@ -1795,11 +1795,11 @@ int __init populate_nv_trap_config(void)
 				ret = -EINVAL;
 				print_nv_trap_error(cgt, "Duplicate CGT", ret);
 			}
-		}
 
-		if (xa_is_err(prev)) {
-			ret = xa_err(prev);
-			print_nv_trap_error(cgt, "Failed CGT insertion", ret);
+			if (xa_is_err(prev)) {
+				ret = xa_err(prev);
+				print_nv_trap_error(cgt, "Failed CGT insertion", ret);
+			}
 		}
 	}
 
@@ -1812,6 +1812,7 @@ int __init populate_nv_trap_config(void)
 	for (int i = 0; i < ARRAY_SIZE(encoding_to_fgt); i++) {
 		const struct encoding_to_trap_config *fgt = &encoding_to_fgt[i];
 		union trap_config tc;
+		void *prev;
 
 		if (fgt->tc.fgt >= __NR_FGT_GROUP_IDS__) {
 			ret = -EINVAL;
@@ -1826,8 +1827,13 @@ int __init populate_nv_trap_config(void)
 		}
 
 		tc.val |= fgt->tc.val;
-		xa_store(&sr_forward_xa, fgt->encoding,
-			 xa_mk_value(tc.val), GFP_KERNEL);
+		prev = xa_store(&sr_forward_xa, fgt->encoding,
+				xa_mk_value(tc.val), GFP_KERNEL);
+
+		if (xa_is_err(prev)) {
+			ret = xa_err(prev);
+			print_nv_trap_error(cgt, "Failed FGT insertion", ret);
+		}
 	}
 
 	kvm_info("nv: %ld fine grained trap handlers\n",

Completely untested, of course.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel