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

[Joey Gouly]

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 at 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