Re: [Xen-devel] [PATCH v9 04/27] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Apr 26, 2016 at 04:21:10AM -0600, Jan Beulich wrote:
> >>> On 25.04.16 at 17:34, <konrad.wilk@xxxxxxxxxx> wrote:
> > The implementation does not actually do any patching.
> > 
> > It just adds the framework for doing the hypercalls,
> > keeping track of ELF payloads, and the basic operations:
> >  - query which payloads exist,
> >  - query for specific payloads,
> >  - check*1, apply*1, replace*1, and unload payloads.
> > 
> > *1: Which of course in this patch are nops.
> > 
> > The functionality is disabled on ARM until all arch
> > components are implemented.
> > 
> > Also by default it is disabled until the implementation
> > is in place.
> > 
> > We also use recursive spinlocks to so that the find_payload
> > function does not need to have a 'lock' and 'non-lock' variant.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Acked-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> 
> I'm hesitant to say that, but with all of this:
> 
> > v9:
> >     s/find_name/get_name/, drop locks when allocating data.
> >     Drop conditional expression on copyback
> >     Move the allocation on upload outside the spinlock.
> >     Add (TECH PREVIEW) to the Kconfig help
> >     Return -EINVAL if the CHECK or UNLOAD action is to be performed and the 
> > payload
> >     state is not in expected state.
> >     Print 'c' not 'u' when invoking the keyhandler.
> 
> ... I'm not sure the earlier R-b can still be considered valid. Andrew?

I don't know what the criteria is for dropping an Reviewed-by.
I am happy to drop it if you would like - but it may be that Andrew
is OK with the way he had his review?

Or is this more of your view as maintainer - that is the patch
changed considerably (and what is that? percentage of the patch?
small amount of the patch? Trivial changes? Dropping code?)?
> 
> > +static int get_name(const xen_xsplice_name_t *name, char *n)
> > +{
> > +    if ( !name->size || name->size > XEN_XSPLICE_NAME_SIZE )
> > +        return -EINVAL;
> > +
> > +    if ( name->pad[0] || name->pad[1] || name->pad[2] )
> > +        return -EINVAL;
> > +
> > +    if ( !guest_handle_okay(name->name, name->size) )
> > +        return -EINVAL;
> > +
> > +    if ( __copy_from_guest(n, name->name, name->size) )
> > +        return -EFAULT;
> 
> Quoting part of my v8.1 reply:
> "Is there a particular reason why you open code copy_from_guest() here?"

You mean why I use guest_handle_okay and __copy_from_guest instead of
say copy_from_guest?

I think it is an artificat of earlier changes - in which the find_name
would only check 'name-size' and then in another function we would
just do '__copy_from_guest'. But that is not needed anymore - so let
me change it to 'copy_from_guest'
I thought at some point you asked for that as the check was done for
it once and there was no point
> 
> > +static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)
> > +{
> > +    struct payload *data, *found;
> > +    char n[XEN_XSPLICE_NAME_SIZE];
> > +    int rc;
> > +
> > +    rc = verify_payload(upload, n);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    data = xzalloc(struct payload);
> > +
> > +    spin_lock(&payload_lock);
> > +
> > +    found = find_payload(n);
> > +    if ( IS_ERR(found) )
> > +    {
> > +        rc = PTR_ERR(found);
> > +        goto out;
> > +    }
> > +    else if ( found )
> > +    {
> > +        rc = -EEXIST;
> > +        goto out;
> > +    }
> > +
> > +    if ( !data )
> > +    {
> > +        rc = -ENOMEM;
> > +        goto out;
> > +    }
> > +
> > +    rc = 0;
> 
> rc is already zero by the time we get here.
> 
> I also wonder whether the code wouldn't be easier to read if you
> used just a sequence of if()/else if() here, without any goto-s.

But I do need to free(data) and unlock the spinlock - so having
a common code to pass through makes sense.

Unless you mean have an condition on if ( !rc ), and do the normal path?
Like so:

    rc = verify_payload(upload, n);
    if ( rc )
        return rc;

    data = xzalloc(struct payload);

    spin_lock(&payload_lock);

    found = find_payload(n);
    if ( IS_ERR(found) )
        rc = PTR_ERR(found);
    else if ( found )
        rc = -EEXIST;

    if ( !rc && !data )
        rc = -ENOMEM;

    if ( !rc )
    {
        memcpy(data->name, n, strlen(n));
        data->state = XSPLICE_STATE_CHECKED;
        INIT_LIST_HEAD(&data->list);

        list_add_tail(&data->list, &payload_list);
        payload_cnt++;
        payload_version++;
    }

    spin_unlock(&payload_lock);

    if ( rc )
        xfree(data);

    return rc;


That looks fine here, but in the subsequent patch I have to also
check for

if ( __copy_from_guest(raw_data, upload->payload, upload->size) )       

and
rc = load_payload_data(data, raw_data, upload->size);

and goto statement help a lot there.

I would rather have it the way it is now if you are OK with that?




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel