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.

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

On 11/28/2016 03:27 PM, Jan Beulich wrote:
> > > > 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?
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

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