Re: [PATCH 8/9] tools/arch/x86: intel_sdsi: Add support for new GUID

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 11/1/22 20:10, David E. Box wrote:
> The structure and content of the On Demand registers is based on the GUID
> which is read from hardware through sysfs. Add support for decoding the
> registers of a new GUID 0xF210D9EF.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
>  tools/arch/x86/intel_sdsi/intel_sdsi.c | 32 ++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/arch/x86/intel_sdsi/intel_sdsi.c b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> index 01b5f9994e11..0680eda78b1a 100644
> --- a/tools/arch/x86/intel_sdsi/intel_sdsi.c
> +++ b/tools/arch/x86/intel_sdsi/intel_sdsi.c
> @@ -35,7 +35,8 @@
>  #define SDSI_DEV		"intel_vsec.sdsi"
>  #define AUX_DEV_PATH		"/sys/bus/auxiliary/devices/"
>  #define SDSI_PATH		(AUX_DEV_DIR SDSI_DEV)
> -#define GUID			0x6dd191
> +#define GUID_V1			0x6dd191
> +#define GUID_V2			0xF210D9EF
>  #define REGISTERS_MIN_SIZE	72
>  #define STATE_CERT_MAX_SIZE	4096
>  #define STATE_MAX_NUM_LICENSES	16
> @@ -100,9 +101,17 @@ struct sdsi_regs {
>  	struct availability prov_avail;
>  	struct nvram_update_limit limits;
>  	uint64_t pcu_cr3_capid_cfg;
> -	uint64_t socket_id;
> +	union {
> +		struct {
> +			uint64_t socket_id;
> +		} v1;
> +		struct {
> +			uint64_t reserved;
> +			uint64_t socket_id;
> +			uint64_t reserved2;
> +		} v2;
> +	} extra;
>  };
> -
>  #define CONTENT_TYPE_LK_ENC		0xD
>  #define CONTENT_TYPE_LK_BLOB_ENC	0xE
>  
> @@ -146,7 +155,7 @@ struct sdsi_dev {
>  	struct state_certificate sc;
>  	char *dev_name;
>  	char *dev_path;
> -	int guid;
> +	uint32_t guid;
>  };
>  
>  enum command {
> @@ -199,7 +208,7 @@ static int sdsi_update_registers(struct sdsi_dev *s)
>  		return -1;
>  	}
>  
> -	if (s->guid != GUID) {
> +	if (s->guid != GUID_V1 && s->guid != GUID_V2) {
>  		fprintf(stderr, "Unrecognized guid, 0x%x\n", s->guid);
>  		fclose(regs_ptr);
>  		return -1;
> @@ -207,7 +216,7 @@ static int sdsi_update_registers(struct sdsi_dev *s)
>  
>  	/* Update register info for this guid */
>  	ret = fread(&s->regs, sizeof(uint8_t), sizeof(s->regs), regs_ptr);
> -	if (ret != sizeof(s->regs)) {
> +	if (ret > (int)sizeof(s->regs)) { /* FIXME: Check size by guid */

This is wrong, fread will never return more then requested, that
would lead to buffer overflows. But it may return 0 on errors, or
a short read on an error.

So you need to fix the FIXME comment you added here as now you
have just disabled all error checking.

Regards,

Hans



>  		fprintf(stderr, "Could not read 'registers' file\n");
>  		fclose(regs_ptr);
>  		return -1;
> @@ -252,10 +261,13 @@ static int sdsi_read_reg(struct sdsi_dev *s)
>  	printf("    Updates Available:          %d\n", s->regs.prov_avail.available);
>  	printf("    Updates Threshold:          %d\n", s->regs.prov_avail.threshold);
>  	printf("NVRAM Udate Limit\n");
> -	printf("    50%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_50_pct ? "Yes" : "No");
> -	printf("    75%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_75_pct ? "Yes" : "No");
> -	printf("    90%% Limit Reached:         %s\n", !!s->regs.limits.sdsi_90_pct ? "Yes" : "No");
> -	printf("Socket ID:                      %ld\n", s->regs.socket_id & 0xF);
> +	printf("    50%% Limit Reached:          %s\n", !!s->regs.limits.sdsi_50_pct ? "Yes" : "No");
> +	printf("    75%% Limit Reached:          %s\n", !!s->regs.limits.sdsi_75_pct ? "Yes" : "No");
> +	printf("    90%% Limit Reached:          %s\n", !!s->regs.limits.sdsi_90_pct ? "Yes" : "No");
> +	if (s->guid == GUID_V1)
> +		printf("Socket ID:                      %ld\n", s->regs.extra.v1.socket_id & 0xF);
> +	else
> +		printf("Socket ID:                      %ld\n", s->regs.extra.v2.socket_id & 0xF);
>  
>  	return 0;
>  }