Re: [RFC PATCH v3 05/18] xen/arm: ITS: Port ITS driver to xen

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-06-29 at 21:17 +0530, Vijay Kilari wrote:
> On Mon, Jun 29, 2015 at 9:13 PM, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> > On Mon, Jun 29, 2015 at 5:09 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> >> On Mon, 2015-06-22 at 17:31 +0530, vijay.kilari@gmail.com wrote:
> >> [...]
> >>> +/*
> >>> + * ITS command descriptors - parameters to be encoded in a command
> >>> + * block.
> >>> + */
> >>> +struct its_cmd_desc {
> >>> +    union {
> >>> +        struct {
> >>> +            struct its_collection *col;
> >>> +            u32 event_id;
> >>> +            u32 dev_id;
> >>> +        } its_inv_cmd;
> >> [...]
> >>> +static struct its_collection *its_build_inv_cmd(its_cmd_block *cmd,
> >>> +                                                struct its_cmd_desc *desc)
> >>> +{
> >>> +    memset(cmd, 0x0, sizeof(its_cmd_block));
> >>> +    cmd->inv.cmd = GITS_CMD_INV;
> >>> +    cmd->inv.devid = desc->its_inv_cmd.dev_id;
> >>> +    cmd->inv.event = desc->its_inv_cmd.event_id;
> >>> +
> >>> +#ifdef DEBUG_GIC_ITS
> >>> +    dump_cmd(cmd);
> >>> +#endif
> >>> +
> >>> +    return desc->its_inv_cmd.col;
> >>> +}
> >> [...]
> >>> +void its_send_inv(struct its_device *dev, struct its_collection *col,
> >>> +                  u32 event_id)
> >>> +{
> >>> +    struct its_cmd_desc desc;
> >>> +
> >>> +    desc.its_inv_cmd.dev_id = dev->device_id;
> >>> +    desc.its_inv_cmd.event_id = event_id;
> >>> +    desc.its_inv_cmd.col = col;
> >>> +
> >>> +    its_send_single_command(dev->its, its_build_inv_cmd, &desc);
> >>> +}
> >> [...]
> >>> +typedef struct __packed {
> >>> +    u64 cmd:8;
> >>> +    u64 res1:24;
> >>> +    u64 devid:32;
> >>> +    u64 event:32;
> >>> +    u64 res2:32;
> >>> +    u64 res3:64;
> >>> +    u64 res4:64;
> >>> +}inv_cmd_t;
> >>
> >> (I've trimmed this to just the INV command, but it's the same for all of
> >> them)
> >>
> >> I suppose this is a mix of the way the Linux code was structured and my
> >> request to use a struct/union to encode these things, but I'm afraid
> >> this is not how I intended to suggest things be done.
> >>
> >> What I expected was something analogous to the hsr or lpae_t types, e.g.
> >> a single:
> >> union its_cmd {
> >>     uint64_t bits[N];
> >>
> >>     struct {
> >>         uint8_t cmd;
> >>         uint8_t pad[...];
> >>     } hdr;
> >>
> >>     struct {
> >>         uint8_t cmd;
> >>         uint8_t res1[3];
> >>         uint32_t devid;
> >>         uint32_t event;
> >>         uint64_t res2[2];
> >>     } inv;
> >> };
> >
> 
>  Commands like MAPD has 40 bit ITT, 5 bit Size and 1 bit valid bit. So we have
> to still manage with bit fields to manage with ease. So we have to use
> mix of bit fields
> and normal types (uint8_t & uint32_t etc.,)

That's fine. Just use normal types for fields are sized that way and
bitfields for the rest. e.g. for mapd:
    struct {
        uint8_t cmd;
        uint8_t res1[3];
        uint32_t devid;
        uint64_t size:5;
        uint64_t res2:59;
...
    };

Looking at ITT_addr you might consider including the res0 bits in bits
0..7 of that double word in the itt_addr, allowing you to avoid a shift
on use of that field (this assumes that the reason for those res0 bits
is the alignment constraints on itt_addr).

Ian.