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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Mon, Jun 29, 2015 at 5:09 PM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> On Mon, 2015-06-22 at 17:31 +0530, vijay.kilari@xxxxxxxxx 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
>
> 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.
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel