Hi Marc,
On Mon, Jul 11, 2022 at 11:53 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> Hi Reiji,
>
> On Tue, 12 Jul 2022 07:11:39 +0100,
> Reiji Watanabe <reijiw@xxxxxxxxxx> wrote:
> >
> > Hi Marc,
> >
> > On Wed, Jul 6, 2022 at 9:43 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > In order to start making the vgic sysreg access from userspace
> > > similar to all the other sysregs, push the userspace memory
> > > access one level down into vgic_v3_cpu_sysregs_uaccess().
> > >
> > > The next step will be to rely on the sysreg infrastructure
> > > to perform this task.
> > >
> > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > > ---
> > > arch/arm64/kvm/vgic-sys-reg-v3.c | 22 +++++++++++++------
> > > arch/arm64/kvm/vgic/vgic-kvm-device.c | 31 ++++++---------------------
> > > arch/arm64/kvm/vgic/vgic.h | 4 ++--
> > > 3 files changed, 23 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c
> > > index 85a5e1d15e9f..8c56e285fde9 100644
> > > --- a/arch/arm64/kvm/vgic-sys-reg-v3.c
> > > +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c
> > > @@ -278,15 +278,21 @@ int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *
> > > return -ENXIO;
> > > }
> > >
> > > -int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> > > - u64 *reg)
> > > +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu,
> > > + struct kvm_device_attr *attr,
> > > + bool is_write)
> > > {
> > > + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> > > struct sys_reg_params params;
> > > const struct sys_reg_desc *r;
> > > - u64 sysreg = (id & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64;
> > > + u64 sysreg;
> > >
> > > - if (is_write)
> > > - params.regval = *reg;
> > > + sysreg = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_MASK) | KVM_REG_SIZE_U64;
> >
> > Why don't you use attr_to_id() here ?
>
> This actually happens in the following patch. Happy to move the change
> here though.
IMHO it makes more sense to have the change in this patch
because vgic_v3_cpu_sysregs_uaccess in this patch already
takes "attr" as the arguments.
Having said that, I'm fine with keeping the current code:)
> >
> >
> > > +
> > > + if (is_write) {
> > > + if (get_user(params.regval, uaddr))
> > > + return -EFAULT;
> > > + }
> > > params.is_write = is_write;
> > >
> > > r = find_reg_by_id(sysreg, ¶ms, gic_v3_icc_reg_descs,
> > > @@ -297,8 +303,10 @@ int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> > > if (!r->access(vcpu, ¶ms, r))
> > > return -EINVAL;
> > >
> > > - if (!is_write)
> > > - *reg = params.regval;
> > > + if (!is_write) {
> > > + if (put_user(params.regval, uaddr))
> > > + return -EFAULT;
> > > + }
> > >
> > > return 0;
> > > }
> > > diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > > index c6d52a1fd9c8..d8269300632d 100644
> > > --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > > +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > > @@ -561,14 +561,9 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> > > if (!is_write)
> > > *reg = tmp32;
> > > break;
> > > - case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS: {
> > > - u64 regid;
> > > -
> > > - regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
> > > - ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
> > > - regid, reg);
> > > + case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS:
> > > + ret = vgic_v3_cpu_sysregs_uaccess(vcpu, attr, is_write);
> >
> > Nit: Since @reg that is passed to vgic_v3_attr_regs_access() will be NULL
> > for KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS, I think it would be more clear
> > if you could update the comment for vgic_v3_attr_regs_access accordingly.
> >
> > ----
> > /*
> > * vgic_v3_attr_regs_access - allows user space to access VGIC v3 state
> > *
> > * @dev: kvm device handle
> > * @attr: kvm device attribute
> > * @reg: address the value is read or written
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > * @is_write: true if userspace is writing a register
> > */
> > static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> > struct kvm_device_attr *attr,
> > u64 *reg, bool is_write)
>
> @reg disappears completely in patch #12. Do you see value in rewriting
> this comment even if I end-up removing it 4 patches down the line?
Oh, I haven't seen PATCH#12 yet. Then let me withdraw my comment.
Thank you,
Reiji
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
