[Skiboot] [PATCH v2 01/11] nest data structure definitions

[Daniel Axtens]

> +struct ima_chip_cb
> +{
> +        uint64_t ima_chip_run_status;
> +        uint64_t ima_chip_command;
> +        uint64_t ima_chip_collection_speed;
> +};
Should these be u64? __u64?
> +
> +/*
> + * PORE_SLW_IMA modes
> + */
> +#define SLW_IMA_MODE_PRODUCTION	0x1
> +
> +/*
> + * PORE_SLW_IMA reserved memory (in HOMER region)
> + */
> +#define SLW_IMA_OFFSET		0x00320000
> +#define SLW_IMA_TOTAL_SIZE	0x80000
> +
> +/*
> + * Counter Storage size (exposed as part of DT)
> + */
> +#define SLW_IMA_SIZE		0x10000
> +
> +/*
> + * PTS Scoms and values
> + */
> +#define IMA_PTS_SCOM		0x00068009
> +#define IMA_PTS_ENABLE		0x00F0000000000000
> +#define IMA_PTS_DISABLE		0x00E0000000000000
> +#define IMA_PTS_START		0x1
> +#define IMA_PTS_STOP		0
> +#define IMA_PTS_ERROR		-1
> +
> +/*
> + * Catalogue structures.
> + * Catalogue is a meta data file provided as part of FW lid.
> + * This file contains information about the various events the
> + * HW supports under the "24x7" umbrella. Events are classified under
> + * 3 different Domains.
> + *	Domain 1  -- Chip Events (PORE_SLW_IMA)
> + *	Domain 2  -- Core Events (24x7 Core IMA)
> + *	Domain 3  -- per-Thread PMU Events
> + */
> +
> +struct nest_catalog_page_0 {
> +#define CATALOG_MAGIC 0x32347837 /* "24x7" in ASCII */
> +	__be32 magic;
> +	__be32 length; /* In 4096 byte pages */
> +	__be64 version; /* XXX: arbitrary? what's the meaning/usage/purpose? */
> +	__u8 build_time_stamp[16]; /* "YYYYMMDDHHMMSS\0\0" */
> +	__u8 reserved2[32];
> +	__be16 schema_data_offs; /* in 4096 byte pages */
> +	__be16 schema_data_len; /* in 4096 byte pages */
> +	__be16 schema_entry_count;
> +	__u8 reserved3[2];
> +	__be16 event_data_offs;
> +	__be16 event_data_len;
> +	__be16 event_entry_count;
> +	__u8 reserved4[2];
> +	__be16 group_data_offs; /* in 4096 byte pages */
> +	__be16 group_data_len; /* in 4096 byte pages */
> +	__be16 group_entry_count;
> +	__u8 reserved5[2];
> +	__be16 formula_data_offs; /* in 4096 byte pages */
> +	__be16 formula_data_len; /* in 4096 byte pages */
> +	__be16 formula_entry_count;
> +	__u8 reserved6[2];
> +	__be32 core_event_offset;
> +	__be32 thread_event_offset;
> +	__be32 chip_event_offset;
> +	__be32 core_group_offset;
> +	__be32 thread_group_offset;
> +	__be32 chip_group_offset;
> +} __packed;
I'm not clear on significance of double underscores in Skiboot. Is there
documentation on this somewhere? Do we need them for things like u8?
> +
> +struct nest_catalogue_group_data {
> +	__be16 length; /* in bytes, must be a multiple of 16 */
> +	__u8 reserved1[2];
> +	/* verified_state, unverified_state, caveat_state, broken_state, ... */
> +	__be32 flags;
> +	__u8 domain; /* Chip = 1, Core = 2 */
> +	__u8 reserved2[1];
> +	__be16 event_group_record_start_offs; /* in bytes, must be 8 byte aligned */
> +	__be16 event_group_record_len; /* in bytes */
> +	/* in bytes, offset from event_group_record */
> +	__u8 group_schema_index;
> +	__u8 event_count;
> +	__be16 event_index[16]; /* in bytes */
Is a 16 element __be16 array a 16 byte array, or a 16 element
2-byte/16-bit array? 
> +	__be16 group_name_len;
> +	__u8 remainder[];
Does this array need a size?
> +	/* __u8 event_name[event_name_len - 2]; */
> +	/* __be16 event_description_len; */
> +	/* __u8 event_desc[event_description_len - 2]; */
> +	/* __be16 detailed_desc_len; */
> +	/* __u8 detailed_desc[detailed_desc_len - 2]; */
Why are these commented out?
> +} __packed;
> +
> +struct nest_catalogue_event_data {
> +	__be16 length; /* in bytes, must be a multiple of 16 */
> +	__be16 formula_index;
> +	__u8 domain; /* Chip = 1, Core = 2 */
> +	__u8 reserved2[1];
> +	__be16 event_group_record_offs; /* in bytes, must be 8 byte aligned */
> +	__be16 event_group_record_len; /* in bytes */
> +
> +	/* in bytes, offset from event_group_record */
> +	__be16 event_counter_offs;
> +
> +	/* verified_state, unverified_state, caveat_state, broken_state, ... */
> +	__be32 flags;
> +
> +	__be16 primary_group_ix;
> +	__be16 group_count;
> +	__be16 event_name_len;
> +	__u8 remainder[];
> +	/* __u8 event_name[event_name_len - 2]; */
> +	/* __be16 event_description_len; */
> +	/* __u8 event_desc[event_description_len - 2]; */
> +	/* __be16 detailed_desc_len; */
> +	/* __u8 detailed_desc[detailed_desc_len - 2]; */
Likewise with these - why are they commented out?
> +} __packed;
> +
> +
> +#define CHIP_EVENTS_SUPPORTED	1
> +#define CHIP_EVENTS_NOT_SUPPORTED	0
> +
> +/*
> + * Just for optimisation, save only relevant addrs
> + */
> +struct nest_catalogue_ptr {
> +	char *catalogue;
> +	char *page0;
> +	char *group_entry;
> +	char *event_entry;
> +	char *thread_event_entry;
> +	char *core_event_entry;
> +	char *chip_event_entry;
> +	char *thread_group_entry;
> +	char *core_group_entry;
> +	char *chip_group_entry;
> +};
> +
> +#define CATALOGUE(x)		x->catalogue
> +#define PAGE0(x)		x->catalogue
Should this be x->page0?

If this didn't cause an error, does this indicate that these macros
aren't used anywhere?
> +#define GROUP_ENTRY(x)		x->group_entry
> +#define EVENT_ENTRY(x)		x->event_entry
> +#define THREAD_EVENT_ENTRY(x)	x->thread_event_entry
> +#define CORE_EVENT_ENTRY(x)	x->core_event_entry
> +#define CHIP_EVENT_ENTRY(x)	x->chip_event_entry
> +#define THREAD_GROUP_ENTRY(x)	x->thread_group_entry
> +#define CORE_GROUP_ENTRY(x)	x->core_group_entry
> +#define CHIP_GROUP_ENTRY(x)	x->chip_group_entry
> +
Should x be in brackets here? e.g. (x)->chip_group_entry
It seems to be common practice with macros, but I don't know if it's
necessary here.

> +/*Size of Nest Catalogue LID (256KBytes) */
> +#define NEST_CATALOGUE_SIZE             0x40000
> +
> +/* Event Domains, Chip=1, Core=2 */
> +#define DOMAIN_CHIP	1
> +#define DOMAIN_CORE	2
> +
> +/* dimm information for utilisation metrics */
> +#define MURANO_CENTAUR_DIMM	24000
> +#define VENICE_CENTAUR_DIMM	27000
> +
> +/*
> + * Function prototypes
> + */
> +int preload_catalogue_lid(void);
> +int load_catalogue_lid(int);
> +int dt_create_nest_chip_types(struct dt_node *);
> +int dt_create_nest_chip_type (struct dt_node *,
> +				struct nest_catalogue_group_data *);
> +int dt_create_nest_chip_mcs_type(struct dt_node *,
> +				struct nest_catalogue_group_data *,char *);
> +int dt_create_nest_chip_mcs_event(struct dt_node *,int,u32);
> +int dt_create_nest_chip_powerbus_type(struct dt_node *,
> +				struct nest_catalogue_group_data *,char *);
> +int dt_create_nest_chip_powerbus_event(struct dt_node *,u32, const char *);
> +int dt_create_nest_chip_alink_event(struct dt_node *,int,u32);
> +int dt_create_nest_chip_alink_xlink_type(struct dt_node *,
> +				struct nest_catalogue_group_data *,char *);
> +int dt_create_nest_chip_xlink_event(struct dt_node *,int,u32);
> +void nest_pmu_init(int);
Would it be possible to use named parameters for these functions?
> +
> +#endif	/* __NEST_H__ */

-- 
Regards,
Daniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 860 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20150728/d79bf146/attachment-0001.sig>