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

[Madhavan Srinivasan]

On Tuesday 28 July 2015 02:04 PM, Daniel Axtens wrote:
>> +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?

Yes. I am rewriting the variable types to u64 and u32 as suggested
in the kernel side patch review.

>> +
>> +/*
>> + * 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?

__beXX is a typedef of uXX  (include/types.h), but on the other hand
beXX has a
typedef with endianness


>> +
>> +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?

It is 16 elements 2byte array.

>  
>> +	__be16 group_name_len;
>> +	__u8 remainder[];
> Does this array need a size?

yeah, I guess I should have it as 1 or just u8 instead of u8 array.
Will change it.

>> +	/* __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?

Reason is that, these elements of the structure are of variable size,
meaning, name and description of an event vary, hence size of this
structure. But we use the "remainder" element before these
commented  code as an marker to get the information further


>> +} __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?

Nice catch, I removed the PAGE0 macro in code, but missed to delete
it here. Rest of the macros are used.

>> +#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.

Yes true. Will follow the common practice. My bad.

>> +/*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?

Basically wanted to limit it to 80 char limit.

Thanks for review
Maddy

>> +
>> +#endif	/* __NEST_H__ */