Re: [Embedded-pv-devel] [PATCH v13] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.11.16 at 13:30, <andr2000@xxxxxxxxx> wrote:
> + * Request open - open a PCM stream for playback or capture:
> + *          0                 1                  2                3        
> octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                | XENSND_OP_OPEN  |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                padding                                |

This still say "padding" instead of "reserved". Also isn't this still part
of the common header?

> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                pcm_rate                               |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |  pcm_format     |  pcm_channels   |             reserved              |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               buffer_sz                               |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                         gref_directory_start                          |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|

Don't you elsewhere use this as an "and so one" marker? That's not
what you want here afaict, because of ...

> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+

... this.

> +/*
> + * Shared page for XENSND_OP_OPEN buffer descriptor (gref_directory in the
> + *   request) employs a list of pages, describing all pages of the shared 
> data
> + *   buffer:
> + *          0                 1                  2                3        
> octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                          gref_dir_next_page                           |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                gref[0]                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                gref[i]                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                gref[N -1]                             |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * gref_dir_next_page - grant_ref_t, reference to the next page describing
> + *   page directory. Must be 0 if no more pages in the list.
> + * gref[i] - grant_ref_t, reference to a shared page of the buffer
> + *   allocated at XENSND_OP_OPEN
> + *
> + * Number of grant_ref_t entries in the whole page directory is not
> + * passed, but instead can be calculated as:
> + *   num_grefs_total = DIV_ROUND_UP(XENSND_OP_OPEN.buffer_sz, PAGE_SIZE);

The header should be self contained, and there's no DIV_ROUND_UP()
anywhere under public/io/ for a reader to refer to. Please express this
using mathematical terms plus, if needed, standard C library ones.

> + * operation - XENSND_OP_MUTE for mute or XENSND_OP_UNMUTE for unmute
> + * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
> + * values:
> + *
> + *          0                 1                  2                3        
> octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |   channel[0]    |   channel[1]    |   channel[2]    |   channel[3]    |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |   channel[i]    |   channel[i+1]  |   channel[i+2]  |   channel[i+3]  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+

Iirc you had confirmed to change this, but you didn't: I can only repeat
that other than e.g. for the volume operations, it is unclear what the
upper bound here is, as you don't use N.

> +struct xensnd_req {
> +    uint16_t id;
> +    uint8_t operation;
> +    uint8_t stream_idx;
> +    uint32_t reserved;
> +    union {
> +        struct xensnd_open_req open;
> +        struct xensnd_close_req close;
> +        struct xensnd_rw_req write;
> +        struct xensnd_rw_req read;

I guess this has been this way before, but - why two of them? Just like
thy type can be shared, the field needs to be here just once (perhaps
named "rw").

> +        struct xensnd_get_vol_req get_vol;
> +        struct xensnd_set_vol_req set_vol;

Same here - if these dummy structures really need to survive, then
I don't think you need multiple for the volume operations or ...

> +        struct xensnd_mute_req mute;
> +        struct xensnd_unmute_req unmute;

... or the muting ones. In fact, if these dummy structures are needed
in the first place, perhaps just one total would suffice?

> +struct xensnd_resp {
> +    uint16_t id;
> +    uint8_t operation;
> +    uint8_t stream_idx;
> +    int8_t status;
> +    uint8_t padding[26];
> +};

2 + 1 + 1 + 1 + 26 = 31, which I don't think is what you want.

Jan

_______________________________________________
Embedded-pv-devel mailing list
Embedded-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/embedded-pv-devel