Re: [Xen-devel] [PATCH v7 09/15] argo: implement the sendv op; evtchn: expose send_guest_global_virq

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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