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

[Oleksandr Andrushchenko <andr2000@gmail.com>]

On 11/28/2016 03:27 PM, Jan Beulich wrote:
>>>> On 28.11.16 at 13:30, <andr2000@gmail.com> 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?
done
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                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.
done
>> +/*
>> + * 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.
done, will put:
num_grefs_total = (XENSND_OP_OPEN.buffer_sz + PAGE_SIZE - 1) / PAGE_SIZE
>> + * 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.
done
>> +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").
done
>> +        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?
I will remove all empty structures
>> +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.
changed 26 to 27
>
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel