Re: [kvm-unit-tests PATCH v5 09/13] arm/arm64: ITS: Commands

[Zenghui Yu]

Hi Eric,

On 2020/3/10 22:54, Eric Auger wrote:
> Implement main ITS commands. The code is largely inherited from
> the ITS driver.
>
> Signed-off-by: Eric Auger <address@hidden>
>
> ---
>
> v3 -> v4:
> - device's itt now is a VGA
> - pass verbose to choose whether we shall print the cmd
> - use printf instead of report_info
>
> v2 -> v3:
> - do not use report() anymore
> - assert if cmd_write exceeds the queue capacity
>
> v1 -> v2:
> - removed its_print_cmd_state
> ---
>   arm/Makefile.arm64         |   2 +-
>   lib/arm64/asm/gic-v3-its.h |  57 +++++
>   lib/arm64/gic-v3-its-cmd.c | 463 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 521 insertions(+), 1 deletion(-)
>   create mode 100644 lib/arm64/gic-v3-its-cmd.c
>
> diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
> index 60182ae..dfd0c56 100644
> --- a/arm/Makefile.arm64
> +++ b/arm/Makefile.arm64
> @@ -19,7 +19,7 @@ endef
>   cstart.o = $(TEST_DIR)/cstart64.o
>   cflatobjs += lib/arm64/processor.o
>   cflatobjs += lib/arm64/spinlock.o
> -cflatobjs += lib/arm64/gic-v3-its.o
> +cflatobjs += lib/arm64/gic-v3-its.o lib/arm64/gic-v3-its-cmd.o
>   
>   OBJDIRS += lib/arm64
>   
> diff --git a/lib/arm64/asm/gic-v3-its.h b/lib/arm64/asm/gic-v3-its.h
> index 3da548b..889d6ce 100644
> --- a/lib/arm64/asm/gic-v3-its.h
> +++ b/lib/arm64/asm/gic-v3-its.h
> @@ -102,6 +102,28 @@ extern struct its_data its_data;
>   #define GITS_BASER_TYPE_DEVICE                1
>   #define GITS_BASER_TYPE_COLLECTION    4
>   
> +/*
> + * ITS commands
> + */
> +#define GITS_CMD_MAPD                  0x08
> +#define GITS_CMD_MAPC                  0x09
> +#define GITS_CMD_MAPTI                 0x0a
> +/* older GIC documentation used MAPVI for this command */
> +#define GITS_CMD_MAPVI                 GITS_CMD_MAPTI

You can drop it.

> +#define GITS_CMD_MAPI                  0x0b
> +#define GITS_CMD_MOVI                  0x01
> +#define GITS_CMD_DISCARD               0x0f
> +#define GITS_CMD_INV                   0x0c
> +#define GITS_CMD_MOVALL                        0x0e
> +#define GITS_CMD_INVALL                        0x0d
> +#define GITS_CMD_INT                   0x03
> +#define GITS_CMD_CLEAR                 0x04
> +#define GITS_CMD_SYNC                  0x05
> +
> +struct its_cmd_block {
> +       u64 raw_cmd[4];
> +};
> +
>   extern void its_parse_typer(void);
>   extern void its_init(void);
>   extern int its_baser_lookup(int i, struct its_baser *baser);
> @@ -109,4 +131,39 @@ extern void its_enable_defaults(void);
>   extern struct its_device *its_create_device(u32 dev_id, int nr_ites);
>   extern struct its_collection *its_create_collection(u32 col_id, u32 
> target_pe);
>   
> +extern void __its_send_mapd(struct its_device *dev, int valid, bool verbose);
> +extern void __its_send_mapc(struct its_collection *col, int valid, bool 
> verbose);
> +extern void __its_send_mapti(struct its_device *dev, u32 irq_id, u32 event_id,
> +                            struct its_collection *col, bool verbose);
> +extern void __its_send_int(struct its_device *dev, u32 event_id, bool verbose);
> +extern void __its_send_inv(struct its_device *dev, u32 event_id, bool verbose);
> +extern void __its_send_discard(struct its_device *dev, u32 event_id, bool 
> verbose);
> +extern void __its_send_clear(struct its_device *dev, u32 event_id, bool 
> verbose);
> +extern void __its_send_invall(struct its_collection *col, bool verbose);
> +extern void __its_send_movi(struct its_device *dev, struct its_collection *col,
> +                           u32 id, bool verbose);
> +extern void __its_send_sync(struct its_collection *col, bool verbose);
> +
> +#define its_send_mapd(dev, valid)                      __its_send_mapd(dev, 
> valid, true)
> +#define its_send_mapc(col, valid)                      __its_send_mapc(col, 
> valid, true)
> +#define its_send_mapti(dev, irqid, eventid, col)       __its_send_mapti(dev, 
> irqid, eventid, col, true)
> +#define its_send_int(dev, eventid)                     __its_send_int(dev, 
> eventid, true)
> +#define its_send_inv(dev, eventid)                     __its_send_inv(dev, 
> eventid, true)
> +#define its_send_discard(dev, eventid)                 __its_send_discard(dev, 
> eventid, true)
> +#define its_send_clear(dev, eventid)                   __its_send_clear(dev, 
> eventid, true)
> +#define its_send_invall(col)                           __its_send_invall(col, 
> true)
> +#define its_send_movi(dev, col, id)                    __its_send_movi(dev, 
> col, id, true)
> +#define its_send_sync(col)                             __its_send_sync(col, 
> true)
> +
> +#define its_send_mapd_nv(dev, valid)                   __its_send_mapd(dev, 
> valid, false)
> +#define its_send_mapc_nv(col, valid)                   __its_send_mapc(col, 
> valid, false)
> +#define its_send_mapti_nv(dev, irqid, eventid, col)    __its_send_mapti(dev, 
> irqid, eventid, col, false)
> +#define its_send_int_nv(dev, eventid)                  __its_send_int(dev, 
> eventid, false)
> +#define its_send_inv_nv(dev, eventid)                  __its_send_inv(dev, 
> eventid, false)
> +#define its_send_discard_nv(dev, eventid)              __its_send_discard(dev, 
> eventid, false)
> +#define its_send_clear_nv(dev, eventid)                        
> __its_send_clear(dev, eventidn false)
> +#define its_send_invall_nv(col)                                
> __its_send_invall(col, false)
> +#define its_send_movi_nv(dev, col, id)                 __its_send_movi(dev, 
> col, id, false)
> +#define its_send_sync_nv(col)                          __its_send_sync(col, 
> false)

(not verbose? Naming is always difficult ;-).)

[...]

> +
> +static void its_build_invall_cmd(struct its_cmd_block *cmd,
> +                             struct its_cmd_desc *desc)
> +{
> +       its_encode_cmd(cmd, GITS_CMD_INVALL);
> +       its_encode_collection(cmd, desc->its_invall_cmd.col->col_id);
> +       its_fixup_cmd(cmd);
> +       if (desc->verbose)
> +               printf("INVALL col_id=%d\n", desc->its_invall_cmd.col->col_id);
> +}
> +
> +static void its_build_clear_cmd(struct its_cmd_block *cmd,
> +                               struct its_cmd_desc *desc)
> +{
> +       its_encode_cmd(cmd, GITS_CMD_CLEAR);
> +       its_encode_devid(cmd, desc->its_clear_cmd.dev->device_id);
> +       its_encode_event_id(cmd, desc->its_clear_cmd.event_id);
> +       its_fixup_cmd(cmd);
> +       if (desc->verbose)
> +               printf("CLEAR col_id=%d\n", desc->its_invall_cmd.col->col_id);

its_invall_cmd.col->col_id?

Don't you want to print the arguments (DeviceID and EventID) as you've
done for other commands?

> +}
> +
> +static void its_build_discard_cmd(struct its_cmd_block *cmd,
> +                                 struct its_cmd_desc *desc)
> +{
> +       its_encode_cmd(cmd, GITS_CMD_DISCARD);
> +       its_encode_devid(cmd, desc->its_discard_cmd.dev->device_id);
> +       its_encode_event_id(cmd, desc->its_discard_cmd.event_id);
> +       its_fixup_cmd(cmd);
> +       if (desc->verbose)
> +               printf("DISCARD col_id=%d\n", desc->its_invall_cmd.col->col_id);

The same question here.

[...]

And afaict, there's some fixes for the Linux ITS driver since the RFC
version of this series. Please have a check if you can.


Thanks,
Zenghui