[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Embedded-pv-devel] [PATCH v10] xen: add para-virtual sound interface header file



Hi Oleksandr,

On 24/11/16 17:33, Oleksandr Andrushchenko wrote:
+struct xensnd_close_req {
+};


I'm afraid structures without any members aren't permitted by C89.

There are 2 options I see:
1. Remove empty structures
2. Leave them in, but add a comment and add a dummy place holder
I would prefer option 1 as this way the interface seems to be clearer,
like all requests are defined etc.
Typo: I would prefer option 2 with placeholders and comments

+struct xensnd_request {
+     uint8_t raw[64];
+};
+
+struct xensnd_response {
+     uint8_t raw[64];
+};


What are these two needed for now? If you want to enforce a
minimum size, that would better go ...

That is because we want to be cache line aligned.


But this is only accurate to your platform. There is HW available with 128
bytes.

For instance on Xen ARM, all the structures are aligned to 128 bytes in
order to fit in most of cache line. Although technically it would be
possible have bigger one (see the definition of CSSIDR).

So, then I will align to 32

To confirm, you don't plan to reduce the size of the command packet to 32 octet?

What it means in practice
is not clear as we have so many HW around, so it is not possible to
fit all. So the options could be:
1. 32 or 64
2. 48 I think is not an option here


Why 48?
It is not an option, saw something like this in fbif.h, but it is 40
actually there:
#define XENFB_OUT_EVENT_SIZE 40

But your 32 value seems to be as random as 40. I guess they define a size that would fit all the command and would potentially allow the addition of commands.

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.