| Message ID | 20241107214137.428439-3-jingzhangos@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Some fixes about vgic-its | expand |
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.
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); +} + /* * This struct provides an intermediate representation of the fields contained * in the GICH_VMCR and ICH_VMCR registers, such that code exporting the GIC
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(+)