lkml.org 
[lkml]   [2023]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v6 08/14] KVM: s390: Move common code of mem_op functions into functions
    On 25/01/2023 22.26, Janis Schoetterl-Glausch wrote:
    > The vcpu and vm mem_op ioctl implementations share some functionality.
    > Move argument checking and buffer allocation into functions and call
    > them from both implementations.
    > This allows code reuse in case of additional future mem_op operations.
    >
    > Suggested-by: Janosch Frank <frankja@linux.ibm.com>
    > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
    > ---
    > arch/s390/kvm/kvm-s390.c | 80 +++++++++++++++++++++-------------------
    > 1 file changed, 42 insertions(+), 38 deletions(-)
    >
    > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
    > index e4890e04b210..e0dfaa195949 100644
    > --- a/arch/s390/kvm/kvm-s390.c
    > +++ b/arch/s390/kvm/kvm-s390.c
    > @@ -2764,24 +2764,44 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
    > return r;
    > }
    >
    > -static bool access_key_invalid(u8 access_key)
    > +static int mem_op_validate_common(struct kvm_s390_mem_op *mop, u64 supported_flags)
    > {
    > - return access_key > 0xf;
    > + if (mop->flags & ~supported_flags || !mop->size)
    > + return -EINVAL;
    > + if (mop->size > MEM_OP_MAX_SIZE)
    > + return -E2BIG;
    > + if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
    > + if (mop->key > 0xf)
    > + return -EINVAL;
    > + } else {
    > + mop->key = 0;
    > + }
    > + return 0;
    > +}
    > +
    > +static void *mem_op_alloc_buf(struct kvm_s390_mem_op *mop)
    > +{
    > + void *buf;
    > +
    > + if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)
    > + return NULL;
    > + buf = vmalloc(mop->size);
    > + if (!buf)
    > + return ERR_PTR(-ENOMEM);
    > + return buf;
    > }
    >
    > static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
    > {
    > void __user *uaddr = (void __user *)mop->buf;
    > - u64 supported_flags;
    > void *tmpbuf = NULL;

    You likely can now remove the "= NULL" here, I guess?

    > int r, srcu_idx;
    >
    > - supported_flags = KVM_S390_MEMOP_F_SKEY_PROTECTION
    > - | KVM_S390_MEMOP_F_CHECK_ONLY;
    > - if (mop->flags & ~supported_flags || !mop->size)
    > - return -EINVAL;
    > - if (mop->size > MEM_OP_MAX_SIZE)
    > - return -E2BIG;
    > + r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_SKEY_PROTECTION |
    > + KVM_S390_MEMOP_F_CHECK_ONLY);
    > + if (r)
    > + return r;
    > +
    > /*
    > * This is technically a heuristic only, if the kvm->lock is not
    > * taken, it is not guaranteed that the vm is/remains non-protected.
    > @@ -2793,17 +2813,9 @@ static int kvm_s390_vm_mem_op(struct kvm *kvm, struct kvm_s390_mem_op *mop)
    > */
    > if (kvm_s390_pv_get_handle(kvm))
    > return -EINVAL;
    > - if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
    > - if (access_key_invalid(mop->key))
    > - return -EINVAL;
    > - } else {
    > - mop->key = 0;
    > - }
    > - if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
    > - tmpbuf = vmalloc(mop->size);
    > - if (!tmpbuf)
    > - return -ENOMEM;
    > - }
    > + tmpbuf = mem_op_alloc_buf(mop);
    > + if (IS_ERR(tmpbuf))
    > + return PTR_ERR(tmpbuf);
    >
    > srcu_idx = srcu_read_lock(&kvm->srcu);
    >
    > @@ -5250,28 +5262,20 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
    > {
    > void __user *uaddr = (void __user *)mop->buf;
    > void *tmpbuf = NULL;

    ... and here, too.

    But I have to admit that I'm also not sure whether I like the
    mem_op_alloc_buf() part or not (the mem_op_validate_common() part looks fine
    to me) : mem_op_alloc_buf() is a new function with 11 lines of code, and the
    old spots that allocate memory were only 5 lines of code each, so you now
    increased the LoC count and additionally have to fiddly with IS_ERR and
    PTR_ERR which is always a little bit ugly in my eyes ... IMHO I'd rather
    keep the old code here. But that's just my 0.02 €, if you think it's nicer
    with mem_op_alloc_buf(), I won't insist on keeping the old code.

    Thomas


    > - int r = 0;
    > - const u64 supported_flags = KVM_S390_MEMOP_F_INJECT_EXCEPTION
    > - | KVM_S390_MEMOP_F_CHECK_ONLY
    > - | KVM_S390_MEMOP_F_SKEY_PROTECTION;
    > + int r;
    >
    > - if (mop->flags & ~supported_flags || mop->ar >= NUM_ACRS || !mop->size)
    > + r = mem_op_validate_common(mop, KVM_S390_MEMOP_F_INJECT_EXCEPTION |
    > + KVM_S390_MEMOP_F_CHECK_ONLY |
    > + KVM_S390_MEMOP_F_SKEY_PROTECTION);
    > + if (r)
    > + return r;
    > + if (mop->ar >= NUM_ACRS)
    > return -EINVAL;
    > - if (mop->size > MEM_OP_MAX_SIZE)
    > - return -E2BIG;
    > if (kvm_s390_pv_cpu_is_protected(vcpu))
    > return -EINVAL;
    > - if (mop->flags & KVM_S390_MEMOP_F_SKEY_PROTECTION) {
    > - if (access_key_invalid(mop->key))
    > - return -EINVAL;
    > - } else {
    > - mop->key = 0;
    > - }
    > - if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
    > - tmpbuf = vmalloc(mop->size);
    > - if (!tmpbuf)
    > - return -ENOMEM;
    > - }
    > + tmpbuf = mem_op_alloc_buf(mop);
    > + if (IS_ERR(tmpbuf))
    > + return PTR_ERR(tmpbuf);
    >
    > switch (mop->op) {
    > case KVM_S390_MEMOP_LOGICAL_READ:

    \
     
     \ /
      Last update: 2023-03-26 23:56    [W:4.661 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site