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

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

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;
};

So its_send_single_command can take a "union its_cmd *" and its_send_inv
should look like:

void its_send_inv(struct its_device *dev, struct its_collection *col,
                  u32 event_id)
{
    union its_cmd cmd;
    /* memset perhaps, or sets .bits = {0,} */

    cmd.inv.cmd = GITS_CMD_INV;
    cmd.inv.dev_id = dev->device_id;
    cmd.inv.event_id = event_id;
    cmd.inv.col = col;

    return its_send_single_command(dev->its, &cmd);
}

I've omitted the its_ prefix and _cmd/_desc suffix where they aren't
needed in the context they are used. (so not cmd.cmd_inv etc).

I've also used proper types where possible instead of bitfields of u64
(although unsigned long bitfields should still be used for sub 8-bit
fields).

The "hdr" member of the union should contain any field which is global
to all commands and which generic code (i.e. which isn't aware which
command is in the cmd in its hand) can use. Off the top of my head that
is just the cmd code itself.

You should also consider doing the collection sync in the caller as
appropriate instead of pushing it down into its_send_single_command.
IMHO its_send_single_command should do just that, not optionally do some
other command too.

Ian.