Re: [PATCH 04/29] tools/xenlogd: add transport layer

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Nov 1, 2023 at 5:34 AM Juergen Gross <jgross@xxxxxxxx> wrote:
>
> Add the transport layer of 9pfs. This is basically the infrastructure
> to receive requests from the frontend and to send the related answers
> via the rings.
>
> In order to avoid unaligned accesses e.g. on Arm, add the definition of
> __packed to the common-macros.h header.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---

> diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
> index ef0954d69d..590d06e906 100644
> --- a/tools/xenlogd/io.c
> +++ b/tools/xenlogd/io.c

> +static unsigned int get_request_bytes(device *device, unsigned int off,
> +                                      unsigned int len)
> +{
> +    unsigned int size;
> +    unsigned int out_data = ring_out_data(device);
> +    RING_IDX prod, cons;
> +
> +    size = min(len - off, out_data);
> +    prod = xen_9pfs_mask(device->intf->out_prod, device->ring_size);
> +    cons = xen_9pfs_mask(device->cons_pvt_out, device->ring_size);
> +    xen_9pfs_read_packet(device->buffer + off, device->data.out, size,
> +                         prod, &cons, device->ring_size);
> +
> +    xen_rmb();           /* Read data out before setting visible consumer. */
> +    device->cons_pvt_out += size;
> +    device->intf->out_cons = device->cons_pvt_out;
> +
> +    /* Signal that more space is available now. */
> +    xenevtchn_notify(xe, device->evtchn);
> +
> +    return size;
> +}
> +
> +static unsigned int put_request_bytes(device *device, unsigned int off,
> +                                      unsigned int len)

Should this be named put_response_bytes?

> +{
> +    unsigned int size;
> +    unsigned int in_data = ring_in_free(device);
> +    RING_IDX prod, cons;
> +
> +    size = min(len - off, in_data);

IIUC, len is the total length of the outgoing data.  Maybe total_len
would be a better name?  I at least read len as just a length for a
particular call.  Same comment applies to get_request_bytes() if you
want to follow it.

> +    prod = xen_9pfs_mask(device->prod_pvt_in, device->ring_size);
> +    cons = xen_9pfs_mask(device->intf->in_cons, device->ring_size);
> +    xen_9pfs_write_packet(device->data.in, device->buffer + off, size,
> +                          &prod, cons, device->ring_size);
> +
> +    xen_wmb();           /* Write data out before setting visible producer. 
> */
> +    device->prod_pvt_in += size;
> +    device->intf->in_prod = device->prod_pvt_in;
> +
> +    return size;
> +}
> +
>  static bool io_work_pending(device *device)
>  {
>      if ( device->stop_thread )
>          return true;
> -    return false;
> +    if ( device->error )
> +        return false;
> +    return device->handle_response ? ring_in_free(device)
> +                                   : ring_out_data(device);
>  }
>
>  void *io_thread(void *arg)
>  {
>      device *device = arg;
> +    unsigned int count = 0;
> +    struct p9_header hdr;
> +    bool in_hdr = true;
> +
> +    device->max_size = device->ring_size;
> +    device->buffer = malloc(device->max_size);
> +    if ( !device->buffer )
> +    {
> +        syslog(LOG_CRIT, "memory allocation failure!");
> +        return NULL;
> +    }
>
>      while ( !device->stop_thread )
>      {
> @@ -36,9 +127,56 @@ void *io_thread(void *arg)
>          }
>          pthread_mutex_unlock(&device->mutex);
>
> -        /* TODO: I/O handling. */
> +        if ( device->stop_thread || device->error )
> +            continue;
> +
> +        if ( !device->handle_response )
> +        {
> +            if ( in_hdr )
> +            {
> +                count += get_request_bytes(device, count, sizeof(hdr));
> +                if ( count != sizeof(hdr) )
> +                    continue;
> +                hdr = *(struct p9_header *)device->buffer;
> +                if ( hdr.size > device->max_size || hdr.size < sizeof(hdr) )
> +                {
> +                    syslog(LOG_ERR, "%u.%u specified illegal request length 
> %u",
> +                           device->domid, device->devid, hdr.size);
> +                    device->error = true;

When device->error is set, io_thread stops processing requests, but do
we want to also tear down this backend?  The event channel at least is
left in place and unmasked.

> +                    continue;
> +                }
> +                in_hdr = false;
> +            }
> +
> +            count += get_request_bytes(device, count, hdr.size);
> +            if ( count < hdr.size )
> +                continue;
> +
> +            /* TODO: handle request. */
> +
> +            device->handle_response = true;
> +            hdr.size = ((struct p9_header *)device->buffer)->size;

hdr.size is set during the struct copy above, so this isn't needed?

Thanks,
Jason