Re: [Xen-devel] [PATCH v3 04/15] argo: init, destroy and soft-reset, with enable command line opt

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Fri, Jan 11, 2019 at 3:54 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 07.01.19 at 08:42, <christopher.w.clark@xxxxxxxxx> wrote:
> > --- a/xen/common/argo.c
> > +++ b/xen/common/argo.c
> > @@ -17,7 +17,177 @@
> >   */
> >
> >  #include <xen/errno.h>
> > +#include <xen/sched.h>
> > +#include <xen/domain.h>
> > +#include <xen/argo.h>
> > +#include <xen/event.h>
> > +#include <xen/domain_page.h>
> >  #include <xen/guest_access.h>
> > +#include <xen/time.h>
> > +#include <public/argo.h>
> > +
> > +DEFINE_XEN_GUEST_HANDLE(xen_argo_addr_t);
> > +DEFINE_XEN_GUEST_HANDLE(xen_argo_ring_t);
> > +
> > +/* Xen command line option to enable argo */
> > +static bool __read_mostly opt_argo_enabled;
> > +boolean_param("argo", opt_argo_enabled);
> > +
> > +typedef struct argo_ring_id
> > +{
> > +    uint32_t port;
>
> evtchn_port_t?

No; so to avoid the potential for confusion, I've renamed that (and other
places where 'port' was used) to 'aport', for "argo port", and added a
definition for the type: xen_argo_port_t as uint32_t, so the distinction is
clearer.

>
> > +static void
> > +ring_remove_mfns(const struct domain *d, struct argo_ring_info *ring_info)
> > +{
> > +    unsigned int i;
> > +
> > +    ASSERT(rw_is_write_locked(&d->argo->lock) ||
> > +           rw_is_write_locked(&argo_lock));
> > +
> > +    if ( !ring_info->mfns )
> > +        return;
> > +
> > +    if ( !ring_info->mfn_mapping )
> > +    {
> > +        ASSERT_UNREACHABLE();
> > +        return;
> > +    }
> > +
> > +    ring_unmap(ring_info);
> > +
> > +    for ( i = 0; i < ring_info->nmfns; i++ )
> > +        if ( !mfn_eq(ring_info->mfns[i], INVALID_MFN) )
> > +            put_page_and_type(mfn_to_page(ring_info->mfns[i]));
> > +
> > +    xfree(ring_info->mfns);
> > +    ring_info->mfns = NULL;
> > +    ring_info->npage = 0;
> > +    xfree(ring_info->mfn_mapping);
> > +    ring_info->mfn_mapping = NULL;
> > +    ring_info->nmfns = 0;
>
> While it shouldn't matter with locking in use, I generally would
> consider it better if counts got set to zero before freeing the
> arrays.

ack, done.

>
> >  long
> >  do_argo_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) arg1,
> >             XEN_GUEST_HANDLE_PARAM(void) arg2, unsigned long arg3,
> >             unsigned long arg4)
> >  {
> > -    return -ENOSYS;
> > +    struct domain *currd = current->domain;
> > +    long rc = -EFAULT;
> > +
> > +    argo_dprintk("->do_argo_op(%u,%p,%p,%d,%d)\n", cmd,
> > +                 (void *)arg1.p, (void *)arg2.p, (int) arg3, (int) arg4);
> > +
> > +    if ( unlikely(!opt_argo_enabled) )
> > +    {
> > +        rc = -EOPNOTSUPP;
> > +        return rc;
> > +    }
> > +
> > +    domain_lock(currd);
>
> What is the rationale for using the domain lock here? We're trying to
> limit its use as much as possible, due to the otherwise heavy
> contention which can result, as it may be held for comparably long
> periods of time.

My inference is that was intended for avoiding interaction between the
hypercall ops and domain destroy, but it is not necessary. I've removed it.
Thanks.

>
> > +int
> > +argo_init(struct domain *d)
> > +{
> > +    struct argo_domain *argo;
> > +
> > +    if ( !opt_argo_enabled )
> > +    {
> > +        argo_dprintk("argo disabled, domid: %d\n", d->domain_id);
> > +        return 0;
> > +    }
> > +
> > +    argo_dprintk("init: domid: %d\n", d->domain_id);
> > +
> > +    argo = xmalloc(struct argo_domain);
> > +    if ( !argo )
> > +        return -ENOMEM;
> > +
> > +    write_lock(&argo_lock);
> > +
> > +    argo_domain_init(argo);
>
> I doubt the lock needs to be held for this function call.

ack; have reordered it to not do that.

>
> > --- a/xen/include/xlat.lst
> > +++ b/xen/include/xlat.lst
> > @@ -148,3 +148,5 @@
> >  ?    flask_setenforce                xsm/flask_op.h
> >  !    flask_sid_context               xsm/flask_op.h
> >  ?    flask_transition                xsm/flask_op.h
> > +?    argo_addr                       argo.h
> > +?    argo_ring                       argo.h
>
> Did I overlook the use of what these cause to be generated?

The last commit in the v3 series added a file to make use of it
but I've now melded that into the commit series now, so coverage
will be introduced as hypercall argument types are added in the
next series.

thanks,

Christopher

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel