On 11/28/2016 03:27 PM, Jan Beulich wrote:
On 28.11.16 at 13:30, <andr2...@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
