|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 13/25] argo: implement the register op
>>> On 01.12.18 at 02:32, <christopher.w.clark@xxxxxxxxx> wrote:
> +static inline uint16_t
> +argo_hash_fn(const struct argo_ring_id *id)
We generally prefer to avoid "inline" outside of header files. Also
is there any strict need for the function to return a fixed width
type? Plus what's the point of the _fn suffix?
> +{
> + uint16_t ret;
> +
> + ret = (uint16_t)(id->addr.port >> 16);
> + ret ^= (uint16_t)id->addr.port;
Pointless casts (with ret itself being uint16_t).
> +static int
> +argo_ring_map_page(struct argo_ring_info *ring_info, uint32_t i,
> + uint8_t **page)
> +{
> + if ( i >= ring_info->nmfns )
> + {
> + printk(XENLOG_ERR "argo: ring (vm%u:%x vm%d) %p attempted to map
> page"
> + " %u of %u\n", ring_info->id.addr.domain_id,
> + ring_info->id.addr.port, ring_info->id.partner, ring_info,
> + i, ring_info->nmfns);
> + return -EFAULT;
> + }
> + ASSERT(ring_info->mfns);
> + ASSERT(ring_info->mfn_mapping);
> +
> + if ( !ring_info->mfn_mapping[i] )
> + {
> + /*
> + * TODO:
> + * The first page of the ring contains the ring indices, so both
> read and
> + * write access to the page is required by the hypervisor, but
> read-access
> + * is not needed for this mapping for the remainder of the ring.
> + * Since this mapping will remain resident in Xen's address space for
> + * the lifetime of the ring, and following the principle of least
> privilege,
> + * it could be preferable to:
> + * # add a XSM check to determine what policy is wanted here
> + * # depending on the XSM query, optionally create this mapping as
> + * _write-only_ on platforms that can support it.
> + * (eg. Intel EPT/AMD NPT).
> + */
> + ring_info->mfn_mapping[i] =
> map_domain_page_global(ring_info->mfns[i]);
> +
> + if ( !ring_info->mfn_mapping[i] )
> + {
> + printk(XENLOG_ERR "argo: ring (vm%u:%x vm%d) %p attempted to map
> page"
> + " %u of %u\n", ring_info->id.addr.domain_id,
> + ring_info->id.addr.port, ring_info->id.partner, ring_info,
> + i, ring_info->nmfns);
> + return -EFAULT;
Unsuitable error code.
> + }
> + argo_dprintk("mapping page %"PRI_mfn" to %p\n",
> + mfn_x(ring_info->mfns[i]), ring_info->mfn_mapping[i]);
> + }
> +
> + if ( page )
> + *page = ring_info->mfn_mapping[i];
This suggests that the parameter is misnamed. "page" variables
should be of types other than struct page_info * only under
exceptional circumstances.
> +static int
> +argo_update_tx_ptr(struct argo_ring_info *ring_info, uint32_t tx_ptr)
> +{
> + uint8_t *dst;
Why uint8_t when you don't mean to access bytes? For the
arithmetic below void * should do just fine.
> + uint32_t *p;
> + int ret;
> +
> + ret = argo_ring_map_page(ring_info, 0, &dst);
> + if ( ret )
> + return ret;
> +
> + p = (uint32_t *)(dst + offsetof(argo_ring_t, tx_ptr));
And then you also don't need any cast here.
> + write_atomic(p, tx_ptr);
> + mb();
While guests need to use non-SMP barriers, I don't see why an
SMP one wouldn't be sufficient here. I also don't see why this
isn't smp_wmb().
> @@ -231,6 +319,388 @@ argo_ring_remove_info(struct domain *d, struct
> argo_ring_info *ring_info)
> xfree(ring_info);
> }
>
> +/*
> + * ring
> + */
I can see the point of using multi-line comments in a few cases
where our style would not permit this, but a single word is imo
too little to justify a style violation.
> +static int
> +argo_find_ring_mfns(struct domain *d, struct argo_ring_info *ring_info,
> + uint32_t npage, XEN_GUEST_HANDLE_PARAM(argo_pfn_t)
> pfn_hnd,
> + uint32_t len)
> +{
> + int i;
> + int ret = 0;
> +
> + if ( (npage << PAGE_SHIFT) < len )
> + return -EINVAL;
> +
> + if ( ring_info->mfns )
> + {
> + /*
> + * Ring already existed. Check if it's the same ring,
> + * i.e. same number of pages and all translated gpfns still
> + * translating to the same mfns
> + */
This comment makes me wonder whether the translations are
permitted to change at other times. If so I'm not sure what
value verification here has. If not, this probably would want to
be debugging-only code.
> + if ( ring_info->npage != npage )
> + i = ring_info->nmfns + 1; /* forces re-register below */
> + else
> + {
> + for ( i = 0; i < ring_info->nmfns; i++ )
> + {
> + argo_pfn_t pfn;
> + mfn_t mfn;
> +
> + ret = copy_from_guest_offset_errno(&pfn, pfn_hnd, i, 1);
> + if ( ret )
> + break;
> +
> + ret = argo_find_ring_mfn(d, pfn, &mfn);
> + if ( ret )
> + break;
> +
> + if ( mfn_x(mfn) != mfn_x(ring_info->mfns[i]) )
> + break;
> + }
> + }
> + if ( i != ring_info->nmfns )
> + {
> + printk(XENLOG_INFO "argo: vm%u re-registering existing argo ring"
> + " (vm%u:%x vm%d), clearing MFN list\n",
> + current->domain->domain_id, ring_info->id.addr.domain_id,
> + ring_info->id.addr.port, ring_info->id.partner);
> +
> + argo_ring_remove_mfns(d, ring_info);
> + ASSERT(!ring_info->mfns);
> + }
> + }
> +
> + if ( !ring_info->mfns )
> + {
> + mfn_t *mfns;
> + uint8_t **mfn_mapping;
> +
> + mfns = xmalloc_array(mfn_t, npage);
> + if ( !mfns )
> + return -ENOMEM;
> +
> + for ( i = 0; i < npage; i++ )
> + mfns[i] = INVALID_MFN;
> +
> + mfn_mapping = xmalloc_array(uint8_t *, npage);
Perhaps better to xzalloc_array() here than to ...
> + if ( !mfn_mapping )
> + {
> + xfree(mfns);
> + return -ENOMEM;
> + }
> +
> + ring_info->npage = npage;
> + ring_info->mfns = mfns;
> + ring_info->mfn_mapping = mfn_mapping;
> + }
> + ASSERT(ring_info->npage == npage);
> +
> + if ( ring_info->nmfns == ring_info->npage )
> + return 0;
> +
> + for ( i = ring_info->nmfns; i < ring_info->npage; i++ )
> + {
> + argo_pfn_t pfn;
> + mfn_t mfn;
> +
> + ret = copy_from_guest_offset_errno(&pfn, pfn_hnd, i, 1);
> + if ( ret )
> + break;
> +
> + ret = argo_find_ring_mfn(d, pfn, &mfn);
> + if ( ret )
> + {
> + printk(XENLOG_ERR "argo: vm%u passed invalid gpfn %"PRI_xen_pfn
> + " ring (vm%u:%x vm%d) %p seq %d of %d\n",
> + d->domain_id, pfn, ring_info->id.addr.domain_id,
> + ring_info->id.addr.port, ring_info->id.partner,
> + ring_info, i, ring_info->npage);
> + break;
> + }
> +
> + ring_info->mfns[i] = mfn;
> + ring_info->nmfns = i + 1;
> +
> + argo_dprintk("%d: %"PRI_xen_pfn" -> %"PRI_mfn"\n",
> + i, pfn, mfn_x(ring_info->mfns[i]));
> +
> + ring_info->mfn_mapping[i] = NULL;
... zap individual slots late here?
> +static struct argo_ring_info *
> +argo_ring_find_info(const struct domain *d, const struct argo_ring_id *id)
> +{
> + uint16_t hash;
> + struct hlist_node *node;
const?
> +static long
> +argo_register_ring(struct domain *d,
> + XEN_GUEST_HANDLE_PARAM(argo_ring_t) ring_hnd,
> + XEN_GUEST_HANDLE_PARAM(argo_pfn_t) pfn_hnd, uint32_t
> npage,
> + bool fail_exist)
> +{
> + struct argo_ring ring;
> + struct argo_ring_info *ring_info;
> + int ret = 0;
> + bool update_tx_ptr = 0;
bool type means true/false in initializers and assignments.
> + uint64_t dst_domain_cookie = 0;
> +
> + if ( !(guest_handle_is_aligned(ring_hnd, ~PAGE_MASK)) )
> + return -EINVAL;
Why? You don't store the handle for later use (and you shouldn't).
If there really is a need for a full page's worth of memory, it
would better be passed in as GFN.
> + read_lock (&argo_lock);
> +
> + do {
> + if ( !d->argo )
> + {
> + ret = -ENODEV;
> + break;
> + }
> +
> + if ( copy_from_guest(&ring, ring_hnd, 1) )
> + {
> + ret = -EFAULT;
> + break;
> + }
> +
> + if ( ring.magic != ARGO_RING_MAGIC )
> + {
> + ret = -EINVAL;
> + break;
> + }
> +
> + if ( (ring.len < (sizeof(struct argo_ring_message_header)
> + + ARGO_ROUNDUP(1) + ARGO_ROUNDUP(1))) ||
An expression like this wants at least a brief explaining comment
attached.
> + (ARGO_ROUNDUP(ring.len) != ring.len) )
> + {
> + ret = -EINVAL;
> + break;
> + }
> +
> + if ( ring.len > ARGO_MAX_RING_SIZE )
> + {
> + ret = -EINVAL;
> + break;
> + }
> +
> + if ( ring.id.partner == ARGO_DOMID_ANY )
> + {
> + ret = xsm_argo_register_any_source(d,
> argo_mac_bootparam_enforcing);
> + if ( ret )
> + break;
> + }
> + else
> + {
> + struct domain *dst_d = get_domain_by_id(ring.id.partner);
> + if ( !dst_d )
Blank line between declaration(s) and statement(s) please.
> + {
> + argo_dprintk("!dst_d, ECONNREFUSED\n");
> + ret = -ECONNREFUSED;
> + break;
> + }
> +
> + ret = xsm_argo_register_single_source(d, dst_d);
> + if ( ret )
> + {
> + put_domain(dst_d);
> + break;
> + }
> +
> + if ( !dst_d->argo )
> + {
> + argo_dprintk("!dst_d->argo, ECONNREFUSED\n");
> + ret = -ECONNREFUSED;
> + put_domain(dst_d);
> + break;
> + }
> +
> + dst_domain_cookie = dst_d->argo->domain_cookie;
> +
> + put_domain(dst_d);
> + }
> +
> + ring.id.addr.domain_id = d->domain_id;
> + if ( copy_field_to_guest(ring_hnd, &ring, id) )
Whenever you copy back out fields (or entire structures) that
you've copied in before, __copy_* variants of the functions
suffice).
> + {
> + ret = -EFAULT;
> + break;
> + }
> +
> + /*
> + * no need for a lock yet, because only we know about this
> + * set the tx pointer if it looks bogus (we don't reset it
> + * because this might be a re-register after S4)
> + */
> +
> + if ( ring.tx_ptr >= ring.len ||
> + ARGO_ROUNDUP(ring.tx_ptr) != ring.tx_ptr )
> + {
> + /*
> + * Since the ring is a mess, attempt to flush the contents of it
> + * here by setting the tx_ptr to the next aligned message slot
> past
> + * the latest rx_ptr we have observed. Handle ring wrap
> correctly.
> + */
> + ring.tx_ptr = ARGO_ROUNDUP(ring.rx_ptr);
> +
> + if ( ring.tx_ptr >= ring.len )
> + ring.tx_ptr = 0;
> +
> + /* ring.tx_ptr will be written back to the guest ring below. */
> + update_tx_ptr = 1;
> + }
> +
> + /* W(L2) protects all the elements of the domain's ring_info */
> + write_lock(&d->argo->lock);
> +
> + do {
> + ring_info = argo_ring_find_info(d, &ring.id);
> +
> + if ( !ring_info )
> + {
> + uint16_t hash;
> +
> + ring_info = xmalloc(struct argo_ring_info);
> + if ( !ring_info )
> + {
> + ret = -ENOMEM;
> + break;
> + }
> +
> + spin_lock_init(&ring_info->lock);
> +
> + ring_info->mfns = NULL;
> + ring_info->npage = 0;
> + ring_info->mfn_mapping = NULL;
> + ring_info->len = 0;
> + ring_info->nmfns = 0;
> + ring_info->tx_ptr = 0;
xzalloc() used above would eliminate the need for all of these.
> + ring_info->partner_cookie = dst_domain_cookie;
> +
> + ring_info->id = ring.id;
> + INIT_HLIST_HEAD(&ring_info->pending);
> +
> + hash = argo_hash_fn(&ring_info->id);
> + hlist_add_head(&ring_info->node, &d->argo->ring_hash[hash]);
> +
> + printk(XENLOG_INFO "argo: vm%u registering ring (vm%u:%x
> vm%d)\n",
> + current->domain->domain_id, ring.id.addr.domain_id,
> + ring.id.addr.port, ring.id.partner);
Please consider using XENLOG_G_INFO for such guest related log
messages.
> @@ -253,6 +723,34 @@ do_argo_message_op(int cmd, XEN_GUEST_HANDLE_PARAM(void)
> arg1,
>
> switch (cmd)
> {
> + case ARGO_MESSAGE_OP_register_ring:
> + {
> + XEN_GUEST_HANDLE_PARAM(argo_ring_t) ring_hnd =
> + guest_handle_cast(arg1, argo_ring_t);
> + XEN_GUEST_HANDLE_PARAM(argo_pfn_t) pfn_hnd =
> + guest_handle_cast(arg2, argo_pfn_t);
> + uint32_t npage = arg3;
> + bool fail_exist = arg4 & ARGO_REGISTER_FLAG_FAIL_EXIST;
> +
> + if ( unlikely(!guest_handle_okay(ring_hnd, 1)) )
> + break;
I don't understand the need for this and ...
> + if ( unlikely(npage > (ARGO_MAX_RING_SIZE >> PAGE_SHIFT)) )
> + {
> + rc = -EINVAL;
> + break;
> + }
> + if ( unlikely(!guest_handle_okay(pfn_hnd, npage)) )
> + break;
... perhaps also this, when you use copy_from_guest() upon access.
> + /* arg4: reserve currently-undefined bits, require zero. */
> + if ( unlikely(arg4 & ~ARGO_REGISTER_FLAG_MASK) )
> + {
> + rc = -EINVAL;
> + break;
> + }
> +
> + rc = argo_register_ring(d, ring_hnd, pfn_hnd, npage, fail_exist);
> + break;
> + }
> default:
Blank line above here please.
> --- a/xen/include/public/argo.h
> +++ b/xen/include/public/argo.h
> @@ -21,6 +21,20 @@
>
> #include "xen.h"
>
> +#define ARGO_RING_MAGIC 0xbd67e163e7777f2fULL
> +
> +#define ARGO_DOMID_ANY DOMID_INVALID
> +
> +/*
> + * The maximum size of an Argo ring is defined to be: 16GB
> + * -- which is 0x1000000 or 16777216 bytes.
> + * A byte index into the ring is at most 24 bits.
> + */
> +#define ARGO_MAX_RING_SIZE (16777216ULL)
> +
> +/* pfn type: 64-bit on all architectures to aid avoiding a compat ABI */
> +typedef uint64_t argo_pfn_t;
> +
> typedef struct argo_addr
> {
> uint32_t port;
It must have started in an earlier patch where I didn't pay
attention: Please can you make sure to prefix all public
header additions to global name space with XEN_ / xen_?
Unless of course it is thought that ARGO_ / argo_ are
entirely impossible to be used in any other environment.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |