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 04:12 PM, Jan Beulich wrote:
> > > > On 25.11.16 at 14:59, <andr2000@xxxxxxxxx> wrote:
> > On 11/25/2016 02:54 PM, Jan Beulich wrote:
> > > > > > On 25.11.16 at 12:57, <andr2000@xxxxxxxxx> wrote:
> > > > +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...
> I don't understand: You have one such entry per page, so how
> can it be larger than (or even equal to) PAGE_SIZE / sizeof(grant_ref_t)?
we employ the idea of a linked list here, e.g. gref_dir_next_page points
to the next page of the same structure.
if there are no more pages with grefs then num_grefs will not be equal to
(PAGE_SIZE - sizeof(gref_dir_next_page) - sizeof(num_grefs)) / 
sizeof(grant_ref_t),
but will contain the remaining number of grefs in this page
this way we can flexibly allocate and reference buffers of any requested 
size
> > > > +union xensnd_req {
> > > > +    struct {
> > > > +        uint16_t id;
> > > > +        uint8_t operation;
> > > > +        uint8_t stream_idx;
> > > > +        uint32_t padding;
> > > > +        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];
> > > 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
> No, that's why I've asked for it to be a member of the inner union.
got you
> Jan

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