Re: [PATCH] mini-os: xenbus: support large messages

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Hello,

Thanks for having worked on this!

Juergen Gross, le mer. 18 août 2021 17:26:10 +0200, a ecrit:
> +static void xenbus_read_data(char *buf, unsigned int len)
> +{
> +    unsigned int off = 0;
> +    unsigned int prod;
> +    unsigned int size;
> +    int notify;
> +
> +    while (off != len)
> +    {
> +        if (xenstore_buf->rsp_prod == xenstore_buf->rsp_cons)
> +            wait_event(xb_waitq,
> +                       xenstore_buf->rsp_prod != xenstore_buf->rsp_cons);

The if is redundant since wait_event already tests it.

> +        prod = xenstore_buf->rsp_prod;
> +        DEBUG("Rsp_cons %d, rsp_prod %d.\n", xenstore_buf->rsp_cons, prod);
> +        size = min(len - off, prod - xenstore_buf->rsp_cons);
> +        memcpy_from_ring(xenstore_buf->rsp, buf + off,
> +                         MASK_XENSTORE_IDX(xenstore_buf->rsp_cons), size);
> +        off += size;
> +        notify = (xenstore_buf->rsp_cons + XENSTORE_RING_SIZE ==
> +                  xenstore_buf->rsp_prod);

This looks odd to me?  We want to notify as soon as the ring is empty,
which can happen at any place in the ring right?

Linux' code uses (intf->rsp_prod - cons >= XENSTORE_RING_SIZE), *after*
the rsp_cons increase.

> +        rmb();

rmb() must be placed before memcpy_from_ring, to make sure that the data
we read from the buffer is up-to-date according to the read we made from
rsp_prod.

The rest seems ok to me.

Samuel