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

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

On Thu, Jan 31, 2019 at 8:38 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> 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>

Thanks.

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

Have responded with a proposed fix to address this on the other thread.

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

ack

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

ack via proposal on other thread

On Thu, Jan 31, 2019 at 8:58 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 31.01.19 at 17:35, <roger.pau@xxxxxxxxxx> wrote:
> > On Wed, Jan 30, 2019 at 08:28:14PM -0800, Christopher Clark wrote:
> >> +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_.
>
> Well, to be precise the >= belongs at the end of the previous line,
> so perhaps the line wrapping wants to be changed altogether.

ack, have rewritten this.

Christopher

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