diff mbox series

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

Message ID 20240122201852.262057-11-maz@kernel.org (mailing list archive)
State New, archived
Headers show
Series KVM/arm64: VM configuration enforcement | expand

Commit Message

Marc Zyngier Jan. 22, 2024, 8:18 p.m. UTC
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(-)

Comments

Joey Gouly Jan. 23, 2024, 4:37 p.m. UTC | #1
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!

> +
>  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.

Thanks,
Joey
Marc Zyngier Jan. 23, 2024, 5:45 p.m. UTC | #2
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.
diff mbox series

Patch

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);
+}
+
 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;