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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

> +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.

>  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.

> +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.

> --- 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?

Jan



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