[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v7 09/15] argo: implement the sendv op; evtchn: expose send_guest_global_virq
On Wed, Jan 30, 2019 at 08:28:14PM -0800, Christopher Clark wrote: > sendv operation is invoked to perform a synchronous send of buffers > contained in iovs to a remote domain's registered ring. > > It takes: > * A destination address (domid, port) for the ring to send to. > It performs a most-specific match lookup, to allow for wildcard. > * A source address, used to inform the destination of where to reply. > * The address of an array of iovs containing the data to send > * .. and the length of that array of iovs > * and a 32-bit message type, available to communicate message context > data (eg. kernel-to-kernel, separate from the application data). > > If insufficient space exists in the destination ring, it will return > -EAGAIN and Xen will notify the caller when sufficient space becomes > available. > > Accesses to the ring indices are appropriately atomic. The rings are > mapped into Xen's private address space to write as needed and the > mappings are retained for later use. > > Notifications are sent to guests via VIRQ and send_guest_global_virq is > exposed in the change to enable argo to call it. VIRQ_ARGO is claimed > from the VIRQ previously reserved for this purpose (#11). > > The VIRQ notification method is used rather than sending events using > evtchn functions directly because: > > * no current event channel type is an exact fit for the intended > behaviour. ECS_IPI is closest, but it disallows migration to > other VCPUs which is not necessarily a requirement for Argo. > > * at the point of argo_init, allocation of an event channel is > complicated by none of the guest VCPUs being initialized yet > and the event channel logic expects that a valid event channel > has a present VCPU. > > * at the point of signalling a notification, the VIRQ logic is already > defensive: if d->vcpu[0] is NULL, the notification is just silently > dropped, whereas the evtchn_send logic is not so defensive: vcpu[0] > must not be NULL, otherwise a null pointer dereference occurs. > > Using a VIRQ removes the need for the guest to query to determine which > event channel notifications will be delivered on. This is also likely to > simplify establishing future L0/L1 nested hypervisor argo communication. > > Signed-off-by: Christopher Clark <christopher.clark6@xxxxxxxxxxxxxx> > Tested-by: Chris Patterson <pattersonc@xxxxxxxxxxxx> There's one style nit that I think can be fixed while committing: Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx> Despite the usage of the open-coded mask below. As with previous patches this is argos code, so I'm not going to oppose, but again I think using such open coded masks is bad, and can lead to bugs in the code. It can be fixed by a follow up patch. > +static int > +ringbuf_insert(const struct domain *d, struct argo_ring_info *ring_info, > + const struct argo_ring_id *src_id, xen_argo_iov_t *iovs, > + unsigned int niov, uint32_t message_type, > + unsigned long *out_len) > +{ > + xen_argo_ring_t ring; > + struct xen_argo_ring_message_header mh = { }; > + int sp, ret; > + unsigned int len = 0; > + xen_argo_iov_t *piov; > + XEN_GUEST_HANDLE(uint8) NULL_hnd = { }; > + > + ASSERT(LOCKING_L3(d, ring_info)); > + > + /* > + * Obtain the total size of data to transmit -- sets the 'len' variable > + * -- and sanity check that the iovs conform to size and number limits. > + * Enforced below: no more than 'len' bytes of guest data > + * (plus the message header) will be sent in this operation. > + */ > + ret = iov_count(iovs, niov, &len); > + if ( ret ) > + return ret; > + > + /* > + * Upper bound check the message len against the ring size. > + * The message must not fill the ring; there must be at least one slot > + * remaining so we can distinguish a full ring from an empty one. > + * iov_count has already verified: len <= MAX_ARGO_MESSAGE_SIZE. > + */ > + if ( (ROUNDUP_MESSAGE(len) + sizeof(struct xen_argo_ring_message_header)) missing space ^ > + >= ring_info->len ) Align of >= also looks weird, should be aligned to the parenthesis before ROUNDUP_. > @@ -1175,6 +1766,42 @@ do_argo_op(unsigned int cmd, > XEN_GUEST_HANDLE_PARAM(void) arg1, > break; > } > > + case XEN_ARGO_OP_sendv: > + { > + xen_argo_send_addr_t send_addr; > + xen_argo_iov_t iovs[XEN_ARGO_MAXIOV]; > + unsigned int niov; > + > + XEN_GUEST_HANDLE_PARAM(xen_argo_send_addr_t) send_addr_hnd = > + guest_handle_cast(arg1, xen_argo_send_addr_t); > + XEN_GUEST_HANDLE_PARAM(xen_argo_iov_t) iovs_hnd = > + guest_handle_cast(arg2, xen_argo_iov_t); > + /* arg3 is niov */ > + /* arg4 is message_type. Must be a 32-bit value. */ > + > + rc = copy_from_guest(&send_addr, send_addr_hnd, 1) ? -EFAULT : 0; > + if ( rc ) > + break; > + > + /* > + * Reject niov above maximum limit or message_types that are outside > + * 32 bit range. > + */ > + if ( unlikely((arg3 > XEN_ARGO_MAXIOV) || (arg4 & ~0xffffffffUL)) ) I still think that using either UINT32_MAX, GB(4) or >> 32 would be better than an open-coded mask. Roger. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |