Re: [Embedded-pv-devel] [PATCH v12] 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/25/2016 02:54 PM, Jan Beulich wrote:
> > > > On 25.11.16 at 12:57, <andr2000@xxxxxxxxx> wrote:
> > + * Request open - open a PCM stream for playback or capture:
> > + *          0                 1                  2                3        
> > octet
> > + * +-----------------+-----------------+-----------------+-----------------+
> > + * |                 id                |    operation    |     stream_idx  |
> > + * +-----------------+-----------------+-----------------+-----------------+
> I think individual requests would better spell out their values, instead
> of all saying "operation".
done, but for those operations like read/write I'll still have operation 
field
in the structure and have description like:
 * operation is equal to XENSND_OP_READ for read or XENSND_OP_WRITE for 
write
> > + * id - uint16_t, private guest value, echoed in response
> > + * operation - uint8_t, XENSND_OP_OPEN
> > + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
> > + *   of the stream)
> Please explain the common header fields just once, instead of for
> each of the commands.
done
> > + * gref_directory_start - grant_ref_t, a reference to the first shared page
> > + *   describing shared buffer references. At least one page exists. If shared
> > + *   buffer size exceeds what can be addressed by this single page, then
> > + *   reference to the next page must be supplied (gref_dir_next_page below
> > + *   is not NULL)
> NULL is a pointer value. You mean zero (or one of its synonyms).
"is not 0"
> > +struct xensnd_page_directory {
> > +    grant_ref_t gref_dir_next_page;
> > +    uint32_t num_grefs;
> You can't fit that many requests on one page anyway, so I think it
> would be better to limit this to 16 bits, and leave 16 bits reserved
> for future use (specified as must-be-zero).
these are not for requests, but for buffers. you can imagine the size of 
this
buffer if we play something like 8 channels 192kHz...
> (Which made me look
> back at other reserved fields: You them "padding" in the ASCII art,
> but "reserved" in the structure definitions, and I don't think I've
> found a place stating that they're required to be zero filled, again
> for future extensibility.)
done
> > +    /* zero-sized arrays are not allowed */
> > +    grant_ref_t gref[1] /* Variable length */
> The second comment is fine; I once again don't think the former
> (describing C language aspects again) is needed.
done
> > +struct xensnd_write_req {
> > +    uint32_t offset;
> > +    uint32_t len;
> > +};
> > +
> > +struct xensnd_read_req {
> > +    uint32_t offset;
> > +    uint32_t len;
> > +};
> There should be just one of them, possibly named xensnd_rw_req.
agree
> > + * 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]  |
> > + * +-----------------+-----------------+-----------------+-----------------+
> There's no i defined anywhere here. Did you mean the last one to
> be XENSND_OP_OPEN.pcm_channels - 1 (just like for the two volume
> operations)?
fixed
> > +union xensnd_req {
> > +    struct {
> > +        uint16_t id;
> > +        uint8_t operation;
> > +        uint8_t stream_idx;
> > +        uint32_t padding;
> reserved (I don't see why you'd need padding here)
done
> > +        union {
> > +            struct xensnd_open_req open;
> > +            struct xensnd_close_req close;
> > +            struct xensnd_write_req write;
> > +            struct xensnd_read_req read;
> > +            struct xensnd_get_vol_req get_vol;
> > +            struct xensnd_set_vol_req set_vol;
> > +            struct xensnd_mute_req mute;
> > +            struct xensnd_unmute_req unmute;
> > +        } op;
> > +    } data;
> > +    uint8_t raw[64];
BTW, I missed a change of 64 to 32 as discussed and agreed before
> This is still misplaced imo, and belongs into the inner union, with the
> array size suitably reduced. After all the initial part is mean to be
> common for all verbs aiui, i.e. including future ones.
if we put this in then you have to find the biggest structure and 
calculate its size so
you know how many bytes need to be in that padding array
if you change something you have to redo the same again and beware
of the fact that this is needed
> > +union xensnd_resp {
> > +    struct {
> > +        uint16_t id;
> > +        uint8_t operation;
> > +        uint8_t stream_idx;
> > +        int8_t status;
> > +        uint8_t padding[3];
> > +    } data;
> > +    uint8_t raw[64];
> And here you don't need the field at all - just make the padding field
> large enough.
I can do it, but my concern is just like above
So, I would have the raw arrays where they are and only change their 
sizes to 32
> Jan

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