Re: [PATCH v4 2/5] KVM: arm64: vgic-its: Add read/write helpers on ITS table entries.

[Marc Zyngier <maz@kernel.org>]

On Thu, 07 Nov 2024 21:41:34 +0000,
Jing Zhang <jingzhangos@google.com> wrote:
> 
> To simplify read/write operations on ITS table entries, two helper
> functions have been implemented. These functions incorporate the
> necessary entry size validation.
> 
> Signed-off-by: Jing Zhang <jingzhangos@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic.h | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
> index f2486b4d9f95..309295f5e1b0 100644
> --- a/arch/arm64/kvm/vgic/vgic.h
> +++ b/arch/arm64/kvm/vgic/vgic.h
> @@ -146,6 +146,29 @@ static inline int vgic_write_guest_lock(struct kvm *kvm, gpa_t gpa,
>  	return ret;
>  }
>  
> +static inline int vgic_its_read_entry_lock(struct vgic_its *its, gpa_t eaddr,
> +					   u64 *eval, unsigned long esize)
> +{
> +	struct kvm *kvm = its->dev->kvm;
> +
> +	if (KVM_BUG_ON(esize != sizeof(*eval), kvm))
> +		return -EINVAL;
> +
> +	return kvm_read_guest_lock(kvm, eaddr, eval, esize);
> +
> +}
> +
> +static inline int vgic_its_write_entry_lock(struct vgic_its *its, gpa_t eaddr,
> +					    u64 eval, unsigned long esize)
> +{
> +	struct kvm *kvm = its->dev->kvm;
> +
> +	if (KVM_BUG_ON(esize != sizeof(eval), kvm))
> +		return -EINVAL;
> +
> +	return vgic_write_guest_lock(kvm, eaddr, &eval, esize);
> +}
> +

I don't think this is right. Or at least not what I had in mind.

What I wanted was to abstract both the element size and the ABI, and
check for the type of 'eval'.

Here, you implicitly case the caller's data on a u64, which C will
happily do without warnings. So this KVM_BUG_ON() checks very little
on writes.

Also, you force the caller to explicitly extract the element-size from
the ABI. Yes, it is available most of the time. But this is about
checking consistency, and you are missing this opportunity.

So while this code isn't wrong, it really doesn't make anything more
robust. It's just another indirection.

	M.

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