Re: [Xen-devel] [PATCH v7 07/15] argo: implement the register op

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

n Thu, Jan 31, 2019 at 8:19 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Wed, Jan 30, 2019 at 08:28:12PM -0800, Christopher Clark wrote:
> > The register op is used by a domain to register a region of memory for
> > receiving messages from either a specified other domain, or, if specifying a
> > wildcard, any domain.
> >
> > This operation creates a mapping within Xen's private address space that
> > will remain resident for the lifetime of the ring. In subsequent commits,
> > the hypervisor will use this mapping to copy data from a sending domain into
> > this registered ring, making it accessible to the domain that registered the
> > ring to receive data.
> >
> > Wildcard any-sender rings are default disabled and registration will be
> > refused with EPERM unless they have been specifically enabled with the
> > new mac-permissive flag that is added to the argo boot option here. The
> > reason why the default for wildcard rings is 'deny' is that there is
> > currently no means to protect the ring from DoS by a noisy domain
> > spamming the ring, affecting other domains ability to send to it. This
> > will be addressed with XSM policy controls in subsequent work.
> >
> > Since denying access to any-sender rings is a significant functional
> > constraint, the new option "mac-permissive" for the argo bootparam
> > enables overriding this. eg: "argo=1,mac-permissive=1"
> >
> > The p2m type of the memory supplied by the guest for the ring must be
> > p2m_ram_rw and the memory will be pinned as PGT_writable_page while the ring
> > is registered.
> >
> > This hypercall op and its interface currently only supports 4K-sized pages.
> >
> > Signed-off-by: Christopher Clark <christopher.clark6@xxxxxxxxxxxxxx>
> > Tested-by: Chris Patterson <pattersonc@xxxxxxxxxxxx>
>
> Just one style issue below that should be fixed before commit, and two
> comments:
>
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks

> > +static int
> > +ring_map_page(const struct domain *d, struct argo_ring_info *ring_info,
> > +              unsigned int i, void **out_ptr)
> > +{
> > +    ASSERT(LOCKING_L3(d, ring_info));
> > +
> > +    /*
> > +     * FIXME: Investigate using vmap to create a single contiguous virtual
> > +     * address space mapping of the ring instead of using the array of 
> > single
> > +     * page mappings.
> > +     * Affects logic in memcpy_to_guest_ring, the mfn_mapping array data
> > +     * structure, and places where ring mappings are added or removed.
> > +     */
> > +
> > +    if ( i >= ring_info->nmfns )
> > +    {
> > +        gprintk(XENLOG_ERR,
> > +               "argo: ring (vm%u:%x vm%u) %p attempted to map page %u of 
> > %u\n",
> > +                ring_info->id.domain_id, ring_info->id.aport,
> > +                ring_info->id.partner_id, ring_info, i, ring_info->nmfns);
> > +        return -ENOMEM;
> > +    }
> > +    i = array_index_nospec(i, ring_info->nmfns);
> > +
> > +    if ( !ring_info->mfns || !ring_info->mfn_mapping)
>                                                        ^ missing space

ack

> [...]
> > +static int
> > +copy_gfn_from_handle(XEN_GUEST_HANDLE_PARAM(void) gfn_hnd, bool compat,
> > +                     unsigned int i, gfn_t *out_gfn)
> > +{
> > +    int ret;
> > +
> > +#ifdef CONFIG_COMPAT
> > +    if ( compat )
> > +    {
> > +        XEN_GUEST_HANDLE_PARAM(compat_pfn_t) c_gfn_hnd =
> > +            guest_handle_cast(gfn_hnd, compat_pfn_t);
> > +        compat_pfn_t c_gfn;
> > +
> > +        ret = __copy_from_guest_offset(&c_gfn, c_gfn_hnd, i, 1) ? -EFAULT 
> > : 0;
> > +        *out_gfn = _gfn(c_gfn);
> > +    }
> > +    else
>
> AFAICT you could place the #endif here and avoid the one below.

ack, thanks.

> > @@ -579,7 +1105,6 @@ compat_argo_op(unsigned int cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) arg1,
> >      argo_dprintk("<-compat_argo_op(%u)=%ld\n", cmd, rc);
> >
> >      return rc;
> > -
>
> This looks like a stray fix that should have gone into a different
> patch.

ack, fixed.

Christopher

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