Message ID | 1476008057-2395-3-git-send-email-jarkko.sakkinen@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 10/09/2016 03:44 PM, Jarkko Sakkinen wrote: > Refactored tpm2_get_tpm_pt to tpm2_getcap_cmd, which means that it also > takes capability ID as input. This is required to access > TPM_CAP_HANDLES, which contains metadata needed for swapping transient > data. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > drivers/char/tpm/tpm.h | 6 +++- > drivers/char/tpm/tpm2-cmd.c | 64 ++++++++++++++++++++--------------------- > drivers/char/tpm/tpm_tis_core.c | 3 +- > 3 files changed, 38 insertions(+), 35 deletions(-) > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 0fab6d5..8176f42 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -85,6 +85,10 @@ enum tpm2_capabilities { > TPM2_CAP_TPM_PROPERTIES = 6, > }; > > +enum tpm2_properties { > + TPM2_PT_FAMILY_INDICATOR = 0x100, > +}; > + > enum tpm2_startup_types { > TPM2_SU_CLEAR = 0x0000, > TPM2_SU_STATE = 0x0001, > @@ -485,7 +489,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > int tpm2_unseal_trusted(struct tpm_chip *chip, > struct trusted_key_payload *payload, > struct trusted_key_options *options); > -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, > +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, > u32 *value, const char *desc); > > int tpm2_auto_startup(struct tpm_chip *chip); > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index 2900e18..fcf3d86 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -111,13 +111,13 @@ struct tpm2_pcr_extend_in { > u8 digest[TPM_DIGEST_SIZE]; > } __packed; > > -struct tpm2_get_tpm_pt_in { > +struct tpm2_getcap_in { > __be32 cap_id; > __be32 property_id; > __be32 property_cnt; > } __packed; > > -struct tpm2_get_tpm_pt_out { > +struct tpm2_getcap_out { > u8 more_data; > __be32 subcap_id; > __be32 property_cnt; > @@ -140,8 +140,8 @@ union tpm2_cmd_params { > struct tpm2_pcr_read_in pcrread_in; > struct tpm2_pcr_read_out pcrread_out; > struct tpm2_pcr_extend_in pcrextend_in; > - struct tpm2_get_tpm_pt_in get_tpm_pt_in; > - struct tpm2_get_tpm_pt_out get_tpm_pt_out; > + struct tpm2_getcap_in getcap_in; > + struct tpm2_getcap_out getcap_out; > struct tpm2_get_random_in getrandom_in; > struct tpm2_get_random_out getrandom_out; > }; > @@ -435,16 +435,6 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) > return total ? total : -EIO; > } > > -#define TPM2_GET_TPM_PT_IN_SIZE \ > - (sizeof(struct tpm_input_header) + \ > - sizeof(struct tpm2_get_tpm_pt_in)) > - > -static const struct tpm_input_header tpm2_get_tpm_pt_header = { > - .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), > - .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE), > - .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) > -}; > - > /** > * Append TPMS_AUTH_COMMAND to the buffer. The buffer must be allocated with > * tpm_buf_alloc(). > @@ -750,35 +740,43 @@ out: > return rc; > } > > +#define TPM2_GETCAP_IN_SIZE \ > + (sizeof(struct tpm_input_header) + sizeof(struct tpm2_getcap_in)) > + > +static const struct tpm_input_header tpm2_getcap_header = { > + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), > + .length = cpu_to_be32(TPM2_GETCAP_IN_SIZE), > + .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) > +}; > + > /** > - * tpm2_get_tpm_pt() - get value of a TPM_CAP_TPM_PROPERTIES type property > - * @chip: TPM chip to use. > - * @property_id: property ID. > - * @value: output variable. > + * tpm2_getcap_cmd() - execute a TPM2_GetCapability command > + * @chip: TPM chip to use > + * @cap_id: capability ID > + * @property_id: property ID > + * @value: value of the property > * @desc: passed to tpm_transmit_cmd() > * > - * 0 is returned when the operation is successful. If a negative number is > - * returned it remarks a POSIX error code. If a positive number is returned > - * it remarks a TPM error. > + * Return: same as with tpm_transmit_cmd > */ > -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, > - const char *desc) > +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, > + u32 *value, const char *desc) This function currently returns single value "u32 *value" as output data. Some calling function expect list of values from capabilities output. For eg., get_active_banks() use TPM_CAP_PCRS capability to retrieve list of active banks. And this capability returns array of pcr selections(which is a struct representing each active PCR bank) I was thinking, can we define output parameter as struct of cap_id and union of expected cap_data ? struct cap_out { u32 cap_id; union cap_data { struct tpml_pcr_selection assignedPCR; struct tpml_tagged_tpm_property tpmProperties; struct tpml_handle handles; } } And then the calling function, map union to the cap_data expected as per id, and parse it. Thanks & Regards, - Nayna > { > struct tpm2_cmd cmd; > int rc; > > - cmd.header.in = tpm2_get_tpm_pt_header; > - cmd.params.get_tpm_pt_in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES); > - cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(property_id); > - cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1); > + cmd.header.in = tpm2_getcap_header; > + cmd.params.getcap_in.cap_id = cpu_to_be32(cap_id); > + cmd.params.getcap_in.property_id = cpu_to_be32(property_id); > + cmd.params.getcap_in.property_cnt = cpu_to_be32(1); > > rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, desc); > if (!rc) > - *value = be32_to_cpu(cmd.params.get_tpm_pt_out.value); > + *value = be32_to_cpu(cmd.params.getcap_out.value); > > return rc; > } > -EXPORT_SYMBOL_GPL(tpm2_get_tpm_pt); > +EXPORT_SYMBOL_GPL(tpm2_getcap_cmd); > > #define TPM2_STARTUP_IN_SIZE \ > (sizeof(struct tpm_input_header) + \ > @@ -978,10 +976,10 @@ int tpm2_probe(struct tpm_chip *chip) > struct tpm2_cmd cmd; > int rc; > > - cmd.header.in = tpm2_get_tpm_pt_header; > - cmd.params.get_tpm_pt_in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES); > - cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(0x100); > - cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1); > + cmd.header.in = tpm2_getcap_header; > + cmd.params.getcap_in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES); > + cmd.params.getcap_in.property_id = cpu_to_be32(TPM2_PT_FAMILY_INDICATOR); > + cmd.params.getcap_in.property_cnt = cpu_to_be32(1); > > rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, NULL); > if (rc < 0) > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index e3bf31b..792ccd1 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -528,7 +528,8 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) > cap_t cap; > > if (chip->flags & TPM_CHIP_FLAG_TPM2) > - return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc); > + return tpm2_getcap_cmd(chip, TPM2_CAP_TPM_PROPERTIES, > + TPM2_PT_FAMILY_INDICATOR, &cap2, desc); > else > return tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc); > } > ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On Fri, Nov 11, 2016 at 09:51:45AM +0530, Nayna wrote: > > > On 10/09/2016 03:44 PM, Jarkko Sakkinen wrote: > > Refactored tpm2_get_tpm_pt to tpm2_getcap_cmd, which means that it also > > takes capability ID as input. This is required to access > > TPM_CAP_HANDLES, which contains metadata needed for swapping transient > > data. > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > --- > > drivers/char/tpm/tpm.h | 6 +++- > > drivers/char/tpm/tpm2-cmd.c | 64 ++++++++++++++++++++--------------------- > > drivers/char/tpm/tpm_tis_core.c | 3 +- > > 3 files changed, 38 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > > index 0fab6d5..8176f42 100644 > > --- a/drivers/char/tpm/tpm.h > > +++ b/drivers/char/tpm/tpm.h > > @@ -85,6 +85,10 @@ enum tpm2_capabilities { > > TPM2_CAP_TPM_PROPERTIES = 6, > > }; > > > > +enum tpm2_properties { > > + TPM2_PT_FAMILY_INDICATOR = 0x100, > > +}; > > + > > enum tpm2_startup_types { > > TPM2_SU_CLEAR = 0x0000, > > TPM2_SU_STATE = 0x0001, > > @@ -485,7 +489,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > > int tpm2_unseal_trusted(struct tpm_chip *chip, > > struct trusted_key_payload *payload, > > struct trusted_key_options *options); > > -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, > > +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, > > u32 *value, const char *desc); > > > > int tpm2_auto_startup(struct tpm_chip *chip); > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > > index 2900e18..fcf3d86 100644 > > --- a/drivers/char/tpm/tpm2-cmd.c > > +++ b/drivers/char/tpm/tpm2-cmd.c > > @@ -111,13 +111,13 @@ struct tpm2_pcr_extend_in { > > u8 digest[TPM_DIGEST_SIZE]; > > } __packed; > > > > -struct tpm2_get_tpm_pt_in { > > +struct tpm2_getcap_in { > > __be32 cap_id; > > __be32 property_id; > > __be32 property_cnt; > > } __packed; > > > > -struct tpm2_get_tpm_pt_out { > > +struct tpm2_getcap_out { > > u8 more_data; > > __be32 subcap_id; > > __be32 property_cnt; > > @@ -140,8 +140,8 @@ union tpm2_cmd_params { > > struct tpm2_pcr_read_in pcrread_in; > > struct tpm2_pcr_read_out pcrread_out; > > struct tpm2_pcr_extend_in pcrextend_in; > > - struct tpm2_get_tpm_pt_in get_tpm_pt_in; > > - struct tpm2_get_tpm_pt_out get_tpm_pt_out; > > + struct tpm2_getcap_in getcap_in; > > + struct tpm2_getcap_out getcap_out; > > struct tpm2_get_random_in getrandom_in; > > struct tpm2_get_random_out getrandom_out; > > }; > > @@ -435,16 +435,6 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) > > return total ? total : -EIO; > > } > > > > -#define TPM2_GET_TPM_PT_IN_SIZE \ > > - (sizeof(struct tpm_input_header) + \ > > - sizeof(struct tpm2_get_tpm_pt_in)) > > - > > -static const struct tpm_input_header tpm2_get_tpm_pt_header = { > > - .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), > > - .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE), > > - .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) > > -}; > > - > > /** > > * Append TPMS_AUTH_COMMAND to the buffer. The buffer must be allocated with > > * tpm_buf_alloc(). > > @@ -750,35 +740,43 @@ out: > > return rc; > > } > > > > +#define TPM2_GETCAP_IN_SIZE \ > > + (sizeof(struct tpm_input_header) + sizeof(struct tpm2_getcap_in)) > > + > > +static const struct tpm_input_header tpm2_getcap_header = { > > + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), > > + .length = cpu_to_be32(TPM2_GETCAP_IN_SIZE), > > + .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) > > +}; > > + > > /** > > - * tpm2_get_tpm_pt() - get value of a TPM_CAP_TPM_PROPERTIES type property > > - * @chip: TPM chip to use. > > - * @property_id: property ID. > > - * @value: output variable. > > + * tpm2_getcap_cmd() - execute a TPM2_GetCapability command > > + * @chip: TPM chip to use > > + * @cap_id: capability ID > > + * @property_id: property ID > > + * @value: value of the property > > * @desc: passed to tpm_transmit_cmd() > > * > > - * 0 is returned when the operation is successful. If a negative number is > > - * returned it remarks a POSIX error code. If a positive number is returned > > - * it remarks a TPM error. > > + * Return: same as with tpm_transmit_cmd > > */ > > -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, > > - const char *desc) > > +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, > > + u32 *value, const char *desc) > > This function currently returns single value "u32 *value" as output data. > > Some calling function expect list of values from capabilities output. > For eg., get_active_banks() use TPM_CAP_PCRS capability to retrieve list of > active banks. And this capability returns array of pcr selections(which is a > struct representing each active PCR bank) > > I was thinking, can we define output parameter as struct of cap_id and union > of expected cap_data ? Unless you have major concerns about performance, which I think are not relevant here, calling this in a loop is perfectly adequate and a lot of simpler than having a generic version (with moreData handling and everything). I would rather see uses of struct cap_t to shrink than to expand. The same goes for other horrific unions that exist today in the driver. /Jarkko ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On Fri, Nov 11, 2016 at 04:02:43PM -0800, Jarkko Sakkinen wrote: > On Fri, Nov 11, 2016 at 09:51:45AM +0530, Nayna wrote: > > > > > > On 10/09/2016 03:44 PM, Jarkko Sakkinen wrote: > > > Refactored tpm2_get_tpm_pt to tpm2_getcap_cmd, which means that it also > > > takes capability ID as input. This is required to access > > > TPM_CAP_HANDLES, which contains metadata needed for swapping transient > > > data. > > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > --- > > > drivers/char/tpm/tpm.h | 6 +++- > > > drivers/char/tpm/tpm2-cmd.c | 64 ++++++++++++++++++++--------------------- > > > drivers/char/tpm/tpm_tis_core.c | 3 +- > > > 3 files changed, 38 insertions(+), 35 deletions(-) > > > > > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > > > index 0fab6d5..8176f42 100644 > > > --- a/drivers/char/tpm/tpm.h > > > +++ b/drivers/char/tpm/tpm.h > > > @@ -85,6 +85,10 @@ enum tpm2_capabilities { > > > TPM2_CAP_TPM_PROPERTIES = 6, > > > }; > > > > > > +enum tpm2_properties { > > > + TPM2_PT_FAMILY_INDICATOR = 0x100, > > > +}; > > > + > > > enum tpm2_startup_types { > > > TPM2_SU_CLEAR = 0x0000, > > > TPM2_SU_STATE = 0x0001, > > > @@ -485,7 +489,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > > > int tpm2_unseal_trusted(struct tpm_chip *chip, > > > struct trusted_key_payload *payload, > > > struct trusted_key_options *options); > > > -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, > > > +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, > > > u32 *value, const char *desc); > > > > > > int tpm2_auto_startup(struct tpm_chip *chip); > > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > > > index 2900e18..fcf3d86 100644 > > > --- a/drivers/char/tpm/tpm2-cmd.c > > > +++ b/drivers/char/tpm/tpm2-cmd.c > > > @@ -111,13 +111,13 @@ struct tpm2_pcr_extend_in { > > > u8 digest[TPM_DIGEST_SIZE]; > > > } __packed; > > > > > > -struct tpm2_get_tpm_pt_in { > > > +struct tpm2_getcap_in { > > > __be32 cap_id; > > > __be32 property_id; > > > __be32 property_cnt; > > > } __packed; > > > > > > -struct tpm2_get_tpm_pt_out { > > > +struct tpm2_getcap_out { > > > u8 more_data; > > > __be32 subcap_id; > > > __be32 property_cnt; > > > @@ -140,8 +140,8 @@ union tpm2_cmd_params { > > > struct tpm2_pcr_read_in pcrread_in; > > > struct tpm2_pcr_read_out pcrread_out; > > > struct tpm2_pcr_extend_in pcrextend_in; > > > - struct tpm2_get_tpm_pt_in get_tpm_pt_in; > > > - struct tpm2_get_tpm_pt_out get_tpm_pt_out; > > > + struct tpm2_getcap_in getcap_in; > > > + struct tpm2_getcap_out getcap_out; > > > struct tpm2_get_random_in getrandom_in; > > > struct tpm2_get_random_out getrandom_out; > > > }; > > > @@ -435,16 +435,6 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) > > > return total ? total : -EIO; > > > } > > > > > > -#define TPM2_GET_TPM_PT_IN_SIZE \ > > > - (sizeof(struct tpm_input_header) + \ > > > - sizeof(struct tpm2_get_tpm_pt_in)) > > > - > > > -static const struct tpm_input_header tpm2_get_tpm_pt_header = { > > > - .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), > > > - .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE), > > > - .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) > > > -}; > > > - > > > /** > > > * Append TPMS_AUTH_COMMAND to the buffer. The buffer must be allocated with > > > * tpm_buf_alloc(). > > > @@ -750,35 +740,43 @@ out: > > > return rc; > > > } > > > > > > +#define TPM2_GETCAP_IN_SIZE \ > > > + (sizeof(struct tpm_input_header) + sizeof(struct tpm2_getcap_in)) > > > + > > > +static const struct tpm_input_header tpm2_getcap_header = { > > > + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), > > > + .length = cpu_to_be32(TPM2_GETCAP_IN_SIZE), > > > + .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) > > > +}; > > > + > > > /** > > > - * tpm2_get_tpm_pt() - get value of a TPM_CAP_TPM_PROPERTIES type property > > > - * @chip: TPM chip to use. > > > - * @property_id: property ID. > > > - * @value: output variable. > > > + * tpm2_getcap_cmd() - execute a TPM2_GetCapability command > > > + * @chip: TPM chip to use > > > + * @cap_id: capability ID > > > + * @property_id: property ID > > > + * @value: value of the property > > > * @desc: passed to tpm_transmit_cmd() > > > * > > > - * 0 is returned when the operation is successful. If a negative number is > > > - * returned it remarks a POSIX error code. If a positive number is returned > > > - * it remarks a TPM error. > > > + * Return: same as with tpm_transmit_cmd > > > */ > > > -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, > > > - const char *desc) > > > +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, > > > + u32 *value, const char *desc) > > > > This function currently returns single value "u32 *value" as output data. > > > > Some calling function expect list of values from capabilities output. > > For eg., get_active_banks() use TPM_CAP_PCRS capability to retrieve list of > > active banks. And this capability returns array of pcr selections(which is a > > struct representing each active PCR bank) > > > > I was thinking, can we define output parameter as struct of cap_id and union > > of expected cap_data ? > > Unless you have major concerns about performance, which I think are not > relevant here, calling this in a loop is perfectly adequate and a lot of > simpler than having a generic version (with moreData handling and > everything). > > I would rather see uses of struct cap_t to shrink than to expand. The > same goes for other horrific unions that exist today in the driver. If you are fine with this patch (give Reviewed-by) I can apply this and add to my 4.10 pull request so that we have common baseline to develop both event log and the resource manager. This is a very low risk commit to take to 4.10 because it is only used for interrupt generation in the TIS driver at the moment. /Jarkko ------------------------------------------------------------------------------
On 11/12/2016 05:32 AM, Jarkko Sakkinen wrote: > On Fri, Nov 11, 2016 at 09:51:45AM +0530, Nayna wrote: >> >> >> On 10/09/2016 03:44 PM, Jarkko Sakkinen wrote: >>> Refactored tpm2_get_tpm_pt to tpm2_getcap_cmd, which means that it also >>> takes capability ID as input. This is required to access >>> TPM_CAP_HANDLES, which contains metadata needed for swapping transient >>> data. >>> >>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> >>> --- >>> drivers/char/tpm/tpm.h | 6 +++- >>> drivers/char/tpm/tpm2-cmd.c | 64 ++++++++++++++++++++--------------------- >>> drivers/char/tpm/tpm_tis_core.c | 3 +- >>> 3 files changed, 38 insertions(+), 35 deletions(-) >>> >>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h >>> index 0fab6d5..8176f42 100644 >>> --- a/drivers/char/tpm/tpm.h >>> +++ b/drivers/char/tpm/tpm.h >>> @@ -85,6 +85,10 @@ enum tpm2_capabilities { >>> TPM2_CAP_TPM_PROPERTIES = 6, >>> }; >>> >>> +enum tpm2_properties { >>> + TPM2_PT_FAMILY_INDICATOR = 0x100, >>> +}; >>> + >>> enum tpm2_startup_types { >>> TPM2_SU_CLEAR = 0x0000, >>> TPM2_SU_STATE = 0x0001, >>> @@ -485,7 +489,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip, >>> int tpm2_unseal_trusted(struct tpm_chip *chip, >>> struct trusted_key_payload *payload, >>> struct trusted_key_options *options); >>> -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, >>> +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, >>> u32 *value, const char *desc); >>> >>> int tpm2_auto_startup(struct tpm_chip *chip); >>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c >>> index 2900e18..fcf3d86 100644 >>> --- a/drivers/char/tpm/tpm2-cmd.c >>> +++ b/drivers/char/tpm/tpm2-cmd.c >>> @@ -111,13 +111,13 @@ struct tpm2_pcr_extend_in { >>> u8 digest[TPM_DIGEST_SIZE]; >>> } __packed; >>> >>> -struct tpm2_get_tpm_pt_in { >>> +struct tpm2_getcap_in { >>> __be32 cap_id; >>> __be32 property_id; >>> __be32 property_cnt; >>> } __packed; >>> >>> -struct tpm2_get_tpm_pt_out { >>> +struct tpm2_getcap_out { >>> u8 more_data; >>> __be32 subcap_id; >>> __be32 property_cnt; >>> @@ -140,8 +140,8 @@ union tpm2_cmd_params { >>> struct tpm2_pcr_read_in pcrread_in; >>> struct tpm2_pcr_read_out pcrread_out; >>> struct tpm2_pcr_extend_in pcrextend_in; >>> - struct tpm2_get_tpm_pt_in get_tpm_pt_in; >>> - struct tpm2_get_tpm_pt_out get_tpm_pt_out; >>> + struct tpm2_getcap_in getcap_in; >>> + struct tpm2_getcap_out getcap_out; >>> struct tpm2_get_random_in getrandom_in; >>> struct tpm2_get_random_out getrandom_out; >>> }; >>> @@ -435,16 +435,6 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) >>> return total ? total : -EIO; >>> } >>> >>> -#define TPM2_GET_TPM_PT_IN_SIZE \ >>> - (sizeof(struct tpm_input_header) + \ >>> - sizeof(struct tpm2_get_tpm_pt_in)) >>> - >>> -static const struct tpm_input_header tpm2_get_tpm_pt_header = { >>> - .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), >>> - .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE), >>> - .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) >>> -}; >>> - >>> /** >>> * Append TPMS_AUTH_COMMAND to the buffer. The buffer must be allocated with >>> * tpm_buf_alloc(). >>> @@ -750,35 +740,43 @@ out: >>> return rc; >>> } >>> >>> +#define TPM2_GETCAP_IN_SIZE \ >>> + (sizeof(struct tpm_input_header) + sizeof(struct tpm2_getcap_in)) >>> + >>> +static const struct tpm_input_header tpm2_getcap_header = { >>> + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), >>> + .length = cpu_to_be32(TPM2_GETCAP_IN_SIZE), >>> + .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) >>> +}; >>> + >>> /** >>> - * tpm2_get_tpm_pt() - get value of a TPM_CAP_TPM_PROPERTIES type property >>> - * @chip: TPM chip to use. >>> - * @property_id: property ID. >>> - * @value: output variable. >>> + * tpm2_getcap_cmd() - execute a TPM2_GetCapability command >>> + * @chip: TPM chip to use >>> + * @cap_id: capability ID >>> + * @property_id: property ID >>> + * @value: value of the property >>> * @desc: passed to tpm_transmit_cmd() >>> * >>> - * 0 is returned when the operation is successful. If a negative number is >>> - * returned it remarks a POSIX error code. If a positive number is returned >>> - * it remarks a TPM error. >>> + * Return: same as with tpm_transmit_cmd >>> */ >>> -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, >>> - const char *desc) >>> +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, >>> + u32 *value, const char *desc) >> >> This function currently returns single value "u32 *value" as output data. >> >> Some calling function expect list of values from capabilities output. >> For eg., get_active_banks() use TPM_CAP_PCRS capability to retrieve list of >> active banks. And this capability returns array of pcr selections(which is a >> struct representing each active PCR bank) >> >> I was thinking, can we define output parameter as struct of cap_id and union >> of expected cap_data ? > > Unless you have major concerns about performance, which I think are not > relevant here, calling this in a loop is perfectly adequate and a lot of > simpler than having a generic version (with moreData handling and > everything). > > I would rather see uses of struct cap_t to shrink than to expand. The > same goes for other horrific unions that exist today in the driver. I agree with the idea of not using union. I tested this for capability TPM2_CAP_PCRS. It seems TPM2_CAP_PCRS capability always returns full PCR allocation, and more_data as 0, So, I think the idea of looping over based on more_data may not work for this capability. I was thinking in place of having u32 *value returning actual value, can we have it as void *value, where getcap_cmd will memcpy its out parameter content after subcap_id to value, and then calling function typecast to expected type and use it. Please let me know if I am missing something in using this function. Thanks & Regards, - Nayna > > /Jarkko > ------------------------------------------------------------------------------
On Thu, Nov 17, 2016 at 05:20:36PM +0530, Nayna wrote: > I tested this for capability TPM2_CAP_PCRS. It seems TPM2_CAP_PCRS > capability always returns full PCR allocation, and more_data as 0, So, I > think the idea of looping over based on more_data may not work for this > capability. You can always request one value at a time until there's no more. If you request N values, depending on the hardware, the hardware returns to you anything from 1 to N values. If you implement a function that requests N values in the command, you *must* handle the case where moreData is 1 even if the hardware you are testing that never happens. That's the reason why I would start with a function that you request one property of one capability and optimize it in future if it doesn't scale for some workload. Do you have a workload where it doesn't scale? /Jarkko ------------------------------------------------------------------------------
On 11/17/2016 11:12 PM, Jarkko Sakkinen wrote: > On Thu, Nov 17, 2016 at 05:20:36PM +0530, Nayna wrote: > >> I tested this for capability TPM2_CAP_PCRS. It seems TPM2_CAP_PCRS >> capability always returns full PCR allocation, and more_data as 0, So, I >> think the idea of looping over based on more_data may not work for this >> capability. > > You can always request one value at a time until there's no more. > > If you request N values, depending on the hardware, the hardware returns > to you anything from 1 to N values. If you implement a function that > requests N values in the command, you *must* handle the case where > moreData is 1 even if the hardware you are testing that never happens. > > That's the reason why I would start with a function that you request one > property of one capability and optimize it in future if it doesn't scale > for some workload. > > Do you have a workload where it doesn't scale? Thanks Jarkko for explaining in detail. If I understood correctly, the idea is to request for one property at a time, and if we need multiple properties, then to request for each of them in a loop. In case of TPM2_CAP_PCRS, property is always zero. This is how I am calling getcap_cmd for TPM2_CAP_PCRS. tpm2_getcap_cmd(chip, TPM2_CAP_PCRS, 0, &cap_data, "get active pcr banks"); Output : [ 17.081665] tpm: cap id to receive value is 2 [ 17.081666] tpm: TPM2_CAP_COMMANDS: more data 1 [ 17.081667] tpm: 2 [ 17.081668] tpm: tpm2_get_active_banks -------> cap is TPM2_CAP_PCRS [ 17.171665] tpm: cap id to receive value is 5 [ 17.171666] tpm: TPM2_CAP_PCRS: more data 0 ---> more data is zero. [ 17.171666] tpm: TPM2_CAP_PCRS: more data 0 [ 17.171667] tpm: count pcr banks is 2 ------> count of active pcr banks information returned more_data is always zero here, so am not sure how to handle more_data in this case ? Since property_id is always zero, I am not able to request for one property at a time. and response_buffer returns the details for both active banks. This is the expected behavior defined in TCG 2.0 Part 3 Commands Specification (Section 30.2.1): "TPM_CAP_PCRS – Returns the current allocation of PCR in a TPML_PCR_SELECTION. The property parameter shall be zero. The TPM will always respond to this command with the full PCR allocation and moreData will be NO." Please let me know, if I am missing something. Thanks & Regards, - Nayna > > /Jarkko > ------------------------------------------------------------------------------
On Fri, Nov 18, 2016 at 05:42:01PM +0530, Nayna wrote: > > > On 11/17/2016 11:12 PM, Jarkko Sakkinen wrote: > > On Thu, Nov 17, 2016 at 05:20:36PM +0530, Nayna wrote: > > > > > I tested this for capability TPM2_CAP_PCRS. It seems TPM2_CAP_PCRS > > > capability always returns full PCR allocation, and more_data as 0, So, I > > > think the idea of looping over based on more_data may not work for this > > > capability. > > > > You can always request one value at a time until there's no more. > > > > If you request N values, depending on the hardware, the hardware returns > > to you anything from 1 to N values. If you implement a function that > > requests N values in the command, you *must* handle the case where > > moreData is 1 even if the hardware you are testing that never happens. > > > > That's the reason why I would start with a function that you request one > > property of one capability and optimize it in future if it doesn't scale > > for some workload. > > > > Do you have a workload where it doesn't scale? > > Thanks Jarkko for explaining in detail. > > If I understood correctly, the idea is to request for one property at a > time, and if we need multiple properties, then to request for each of them > in a loop. In case of TPM2_CAP_PCRS, property is always zero. This is how I > am calling getcap_cmd for TPM2_CAP_PCRS. > > tpm2_getcap_cmd(chip, TPM2_CAP_PCRS, 0, &cap_data, "get active pcr banks"); > > Output : > > [ 17.081665] tpm: cap id to receive value is 2 > [ 17.081666] tpm: TPM2_CAP_COMMANDS: more data 1 > [ 17.081667] tpm: 2 > [ 17.081668] tpm: tpm2_get_active_banks -------> cap is TPM2_CAP_PCRS > [ 17.171665] tpm: cap id to receive value is 5 > [ 17.171666] tpm: TPM2_CAP_PCRS: more data 0 ---> more data is zero. > [ 17.171666] tpm: TPM2_CAP_PCRS: more data 0 > [ 17.171667] tpm: count pcr banks is 2 ------> count of active pcr banks > information returned > > more_data is always zero here, so am not sure how to handle more_data in > this case ? > Since property_id is always zero, I am not able to request for one property > at a time. > and response_buffer returns the details for both active banks. > > This is the expected behavior defined in TCG 2.0 Part 3 Commands > Specification (Section 30.2.1): > > "TPM_CAP_PCRS – Returns the current allocation of PCR in a > TPML_PCR_SELECTION. The property parameter shall be zero. The TPM will > always respond to this command with the full PCR allocation and moreData > will be NO." > > Please let me know, if I am missing something. Thanks for pointing that. I think you got it right and I had some wrong assumptions about 'moreData'. Here's what I propose. Do a non-generic function just for getting CAP_PCRS. You could call it tpm2_get_pcr_allocation() as you don't want or rather need to handle all the bells and whistles in that TPM command. It makes a lot more sense now than having one-size-for-all function. /Jarkko ------------------------------------------------------------------------------
On 11/18/2016 09:43 PM, Jarkko Sakkinen wrote: > On Fri, Nov 18, 2016 at 05:42:01PM +0530, Nayna wrote: >> >> >> On 11/17/2016 11:12 PM, Jarkko Sakkinen wrote: >>> On Thu, Nov 17, 2016 at 05:20:36PM +0530, Nayna wrote: >>> >>>> I tested this for capability TPM2_CAP_PCRS. It seems TPM2_CAP_PCRS >>>> capability always returns full PCR allocation, and more_data as 0, So, I >>>> think the idea of looping over based on more_data may not work for this >>>> capability. >>> >>> You can always request one value at a time until there's no more. >>> >>> If you request N values, depending on the hardware, the hardware returns >>> to you anything from 1 to N values. If you implement a function that >>> requests N values in the command, you *must* handle the case where >>> moreData is 1 even if the hardware you are testing that never happens. >>> >>> That's the reason why I would start with a function that you request one >>> property of one capability and optimize it in future if it doesn't scale >>> for some workload. >>> >>> Do you have a workload where it doesn't scale? >> >> Thanks Jarkko for explaining in detail. >> >> If I understood correctly, the idea is to request for one property at a >> time, and if we need multiple properties, then to request for each of them >> in a loop. In case of TPM2_CAP_PCRS, property is always zero. This is how I >> am calling getcap_cmd for TPM2_CAP_PCRS. >> >> tpm2_getcap_cmd(chip, TPM2_CAP_PCRS, 0, &cap_data, "get active pcr banks"); >> >> Output : >> >> [ 17.081665] tpm: cap id to receive value is 2 >> [ 17.081666] tpm: TPM2_CAP_COMMANDS: more data 1 >> [ 17.081667] tpm: 2 >> [ 17.081668] tpm: tpm2_get_active_banks -------> cap is TPM2_CAP_PCRS >> [ 17.171665] tpm: cap id to receive value is 5 >> [ 17.171666] tpm: TPM2_CAP_PCRS: more data 0 ---> more data is zero. >> [ 17.171666] tpm: TPM2_CAP_PCRS: more data 0 >> [ 17.171667] tpm: count pcr banks is 2 ------> count of active pcr banks >> information returned >> >> more_data is always zero here, so am not sure how to handle more_data in >> this case ? >> Since property_id is always zero, I am not able to request for one property >> at a time. >> and response_buffer returns the details for both active banks. >> >> This is the expected behavior defined in TCG 2.0 Part 3 Commands >> Specification (Section 30.2.1): >> >> "TPM_CAP_PCRS – Returns the current allocation of PCR in a >> TPML_PCR_SELECTION. The property parameter shall be zero. The TPM will >> always respond to this command with the full PCR allocation and moreData >> will be NO." >> >> Please let me know, if I am missing something. > > Thanks for pointing that. I think you got it right and I had some wrong > assumptions about 'moreData'. > > Here's what I propose. Do a non-generic function just for getting CAP_PCRS. > You could call it tpm2_get_pcr_allocation() as you don't want or rather > need to handle all the bells and whistles in that TPM command. > > It makes a lot more sense now than having one-size-for-all function. Thanks Jarkko, Yeah, Sure, I will write it as different non-generic call. Otherwise, the function works good. Also, I am thinking now I can write "multi-bank support for extend" on top of master branch itself. Any issues with that ? Thanks & Regards, - Nayna > > /Jarkko > ------------------------------------------------------------------------------
On Tue, Nov 22, 2016 at 02:38:21PM +0530, Nayna wrote: > > > On 11/18/2016 09:43 PM, Jarkko Sakkinen wrote: > > On Fri, Nov 18, 2016 at 05:42:01PM +0530, Nayna wrote: > > > > > > > > > On 11/17/2016 11:12 PM, Jarkko Sakkinen wrote: > > > > On Thu, Nov 17, 2016 at 05:20:36PM +0530, Nayna wrote: > > > > > > > > > I tested this for capability TPM2_CAP_PCRS. It seems TPM2_CAP_PCRS > > > > > capability always returns full PCR allocation, and more_data as 0, So, I > > > > > think the idea of looping over based on more_data may not work for this > > > > > capability. > > > > > > > > You can always request one value at a time until there's no more. > > > > > > > > If you request N values, depending on the hardware, the hardware returns > > > > to you anything from 1 to N values. If you implement a function that > > > > requests N values in the command, you *must* handle the case where > > > > moreData is 1 even if the hardware you are testing that never happens. > > > > > > > > That's the reason why I would start with a function that you request one > > > > property of one capability and optimize it in future if it doesn't scale > > > > for some workload. > > > > > > > > Do you have a workload where it doesn't scale? > > > > > > Thanks Jarkko for explaining in detail. > > > > > > If I understood correctly, the idea is to request for one property at a > > > time, and if we need multiple properties, then to request for each of them > > > in a loop. In case of TPM2_CAP_PCRS, property is always zero. This is how I > > > am calling getcap_cmd for TPM2_CAP_PCRS. > > > > > > tpm2_getcap_cmd(chip, TPM2_CAP_PCRS, 0, &cap_data, "get active pcr banks"); > > > > > > Output : > > > > > > [ 17.081665] tpm: cap id to receive value is 2 > > > [ 17.081666] tpm: TPM2_CAP_COMMANDS: more data 1 > > > [ 17.081667] tpm: 2 > > > [ 17.081668] tpm: tpm2_get_active_banks -------> cap is TPM2_CAP_PCRS > > > [ 17.171665] tpm: cap id to receive value is 5 > > > [ 17.171666] tpm: TPM2_CAP_PCRS: more data 0 ---> more data is zero. > > > [ 17.171666] tpm: TPM2_CAP_PCRS: more data 0 > > > [ 17.171667] tpm: count pcr banks is 2 ------> count of active pcr banks > > > information returned > > > > > > more_data is always zero here, so am not sure how to handle more_data in > > > this case ? > > > Since property_id is always zero, I am not able to request for one property > > > at a time. > > > and response_buffer returns the details for both active banks. > > > > > > This is the expected behavior defined in TCG 2.0 Part 3 Commands > > > Specification (Section 30.2.1): > > > > > > "TPM_CAP_PCRS – Returns the current allocation of PCR in a > > > TPML_PCR_SELECTION. The property parameter shall be zero. The TPM will > > > always respond to this command with the full PCR allocation and moreData > > > will be NO." > > > > > > Please let me know, if I am missing something. > > > > Thanks for pointing that. I think you got it right and I had some wrong > > assumptions about 'moreData'. > > > > Here's what I propose. Do a non-generic function just for getting CAP_PCRS. > > You could call it tpm2_get_pcr_allocation() as you don't want or rather > > need to handle all the bells and whistles in that TPM command. > > > > It makes a lot more sense now than having one-size-for-all function. > > Thanks Jarkko, Yeah, Sure, I will write it as different non-generic call. > Otherwise, the function works good. > Also, I am thinking now I can write "multi-bank support for extend" on top > of master branch itself. Any issues with that ? I'm not sure what exactly I should or should not have an issue with. I can check the patch but won't apply or test it before it is used for something if that is what you are asking. /Jarkko ------------------------------------------------------------------------------
On 11/15/2016 05:18 AM, Jarkko Sakkinen wrote: > On Fri, Nov 11, 2016 at 04:02:43PM -0800, Jarkko Sakkinen wrote: >> On Fri, Nov 11, 2016 at 09:51:45AM +0530, Nayna wrote: >>> >>> >>> On 10/09/2016 03:44 PM, Jarkko Sakkinen wrote: >>>> Refactored tpm2_get_tpm_pt to tpm2_getcap_cmd, which means that it also >>>> takes capability ID as input. This is required to access >>>> TPM_CAP_HANDLES, which contains metadata needed for swapping transient >>>> data. >>>> >>>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> >>>> --- >>>> drivers/char/tpm/tpm.h | 6 +++- >>>> drivers/char/tpm/tpm2-cmd.c | 64 ++++++++++++++++++++--------------------- >>>> drivers/char/tpm/tpm_tis_core.c | 3 +- >>>> 3 files changed, 38 insertions(+), 35 deletions(-) >>>> >>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h >>>> index 0fab6d5..8176f42 100644 >>>> --- a/drivers/char/tpm/tpm.h >>>> +++ b/drivers/char/tpm/tpm.h >>>> @@ -85,6 +85,10 @@ enum tpm2_capabilities { >>>> TPM2_CAP_TPM_PROPERTIES = 6, >>>> }; >>>> >>>> +enum tpm2_properties { >>>> + TPM2_PT_FAMILY_INDICATOR = 0x100, >>>> +}; >>>> + >>>> enum tpm2_startup_types { >>>> TPM2_SU_CLEAR = 0x0000, >>>> TPM2_SU_STATE = 0x0001, >>>> @@ -485,7 +489,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip, >>>> int tpm2_unseal_trusted(struct tpm_chip *chip, >>>> struct trusted_key_payload *payload, >>>> struct trusted_key_options *options); >>>> -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, >>>> +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, >>>> u32 *value, const char *desc); >>>> >>>> int tpm2_auto_startup(struct tpm_chip *chip); >>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c >>>> index 2900e18..fcf3d86 100644 >>>> --- a/drivers/char/tpm/tpm2-cmd.c >>>> +++ b/drivers/char/tpm/tpm2-cmd.c >>>> @@ -111,13 +111,13 @@ struct tpm2_pcr_extend_in { >>>> u8 digest[TPM_DIGEST_SIZE]; >>>> } __packed; >>>> >>>> -struct tpm2_get_tpm_pt_in { >>>> +struct tpm2_getcap_in { >>>> __be32 cap_id; >>>> __be32 property_id; >>>> __be32 property_cnt; >>>> } __packed; >>>> >>>> -struct tpm2_get_tpm_pt_out { >>>> +struct tpm2_getcap_out { >>>> u8 more_data; >>>> __be32 subcap_id; >>>> __be32 property_cnt; >>>> @@ -140,8 +140,8 @@ union tpm2_cmd_params { >>>> struct tpm2_pcr_read_in pcrread_in; >>>> struct tpm2_pcr_read_out pcrread_out; >>>> struct tpm2_pcr_extend_in pcrextend_in; >>>> - struct tpm2_get_tpm_pt_in get_tpm_pt_in; >>>> - struct tpm2_get_tpm_pt_out get_tpm_pt_out; >>>> + struct tpm2_getcap_in getcap_in; >>>> + struct tpm2_getcap_out getcap_out; >>>> struct tpm2_get_random_in getrandom_in; >>>> struct tpm2_get_random_out getrandom_out; >>>> }; >>>> @@ -435,16 +435,6 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) >>>> return total ? total : -EIO; >>>> } >>>> >>>> -#define TPM2_GET_TPM_PT_IN_SIZE \ >>>> - (sizeof(struct tpm_input_header) + \ >>>> - sizeof(struct tpm2_get_tpm_pt_in)) >>>> - >>>> -static const struct tpm_input_header tpm2_get_tpm_pt_header = { >>>> - .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), >>>> - .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE), >>>> - .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) >>>> -}; >>>> - >>>> /** >>>> * Append TPMS_AUTH_COMMAND to the buffer. The buffer must be allocated with >>>> * tpm_buf_alloc(). >>>> @@ -750,35 +740,43 @@ out: >>>> return rc; >>>> } >>>> >>>> +#define TPM2_GETCAP_IN_SIZE \ >>>> + (sizeof(struct tpm_input_header) + sizeof(struct tpm2_getcap_in)) >>>> + >>>> +static const struct tpm_input_header tpm2_getcap_header = { >>>> + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), >>>> + .length = cpu_to_be32(TPM2_GETCAP_IN_SIZE), >>>> + .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) >>>> +}; >>>> + >>>> /** >>>> - * tpm2_get_tpm_pt() - get value of a TPM_CAP_TPM_PROPERTIES type property >>>> - * @chip: TPM chip to use. >>>> - * @property_id: property ID. >>>> - * @value: output variable. >>>> + * tpm2_getcap_cmd() - execute a TPM2_GetCapability command >>>> + * @chip: TPM chip to use >>>> + * @cap_id: capability ID >>>> + * @property_id: property ID >>>> + * @value: value of the property >>>> * @desc: passed to tpm_transmit_cmd() >>>> * >>>> - * 0 is returned when the operation is successful. If a negative number is >>>> - * returned it remarks a POSIX error code. If a positive number is returned >>>> - * it remarks a TPM error. >>>> + * Return: same as with tpm_transmit_cmd >>>> */ >>>> -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, >>>> - const char *desc) >>>> +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, >>>> + u32 *value, const char *desc) >>> >>> This function currently returns single value "u32 *value" as output data. >>> >>> Some calling function expect list of values from capabilities output. >>> For eg., get_active_banks() use TPM_CAP_PCRS capability to retrieve list of >>> active banks. And this capability returns array of pcr selections(which is a >>> struct representing each active PCR bank) >>> >>> I was thinking, can we define output parameter as struct of cap_id and union >>> of expected cap_data ? >> >> Unless you have major concerns about performance, which I think are not >> relevant here, calling this in a loop is perfectly adequate and a lot of >> simpler than having a generic version (with moreData handling and >> everything). >> >> I would rather see uses of struct cap_t to shrink than to expand. The >> same goes for other horrific unions that exist today in the driver. > > If you are fine with this patch (give Reviewed-by) I can apply this > and add to my 4.10 pull request so that we have common baseline to > develop both event log and the resource manager. > > This is a very low risk commit to take to 4.10 because it is only used > for interrupt generation in the TIS driver at the moment. Works for capabilities with more_data(yes) and single value properties. Reviewed-by: Nayna Jain <nayna@linux.vnet.ibm.com> Thanks & Regards, - Nayna > > /Jarkko > ------------------------------------------------------------------------------
On Thu, Nov 24, 2016 at 07:12:57PM +0530, Nayna wrote: > > > On 11/15/2016 05:18 AM, Jarkko Sakkinen wrote: > > On Fri, Nov 11, 2016 at 04:02:43PM -0800, Jarkko Sakkinen wrote: > > > On Fri, Nov 11, 2016 at 09:51:45AM +0530, Nayna wrote: > > > > > > > > > > > > On 10/09/2016 03:44 PM, Jarkko Sakkinen wrote: > > > > > Refactored tpm2_get_tpm_pt to tpm2_getcap_cmd, which means that it also > > > > > takes capability ID as input. This is required to access > > > > > TPM_CAP_HANDLES, which contains metadata needed for swapping transient > > > > > data. > > > > > > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > > --- > > > > > drivers/char/tpm/tpm.h | 6 +++- > > > > > drivers/char/tpm/tpm2-cmd.c | 64 ++++++++++++++++++++--------------------- > > > > > drivers/char/tpm/tpm_tis_core.c | 3 +- > > > > > 3 files changed, 38 insertions(+), 35 deletions(-) > > > > > > > > > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > > > > > index 0fab6d5..8176f42 100644 > > > > > --- a/drivers/char/tpm/tpm.h > > > > > +++ b/drivers/char/tpm/tpm.h > > > > > @@ -85,6 +85,10 @@ enum tpm2_capabilities { > > > > > TPM2_CAP_TPM_PROPERTIES = 6, > > > > > }; > > > > > > > > > > +enum tpm2_properties { > > > > > + TPM2_PT_FAMILY_INDICATOR = 0x100, > > > > > +}; > > > > > + > > > > > enum tpm2_startup_types { > > > > > TPM2_SU_CLEAR = 0x0000, > > > > > TPM2_SU_STATE = 0x0001, > > > > > @@ -485,7 +489,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > > > > > int tpm2_unseal_trusted(struct tpm_chip *chip, > > > > > struct trusted_key_payload *payload, > > > > > struct trusted_key_options *options); > > > > > -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, > > > > > +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, > > > > > u32 *value, const char *desc); > > > > > > > > > > int tpm2_auto_startup(struct tpm_chip *chip); > > > > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > > > > > index 2900e18..fcf3d86 100644 > > > > > --- a/drivers/char/tpm/tpm2-cmd.c > > > > > +++ b/drivers/char/tpm/tpm2-cmd.c > > > > > @@ -111,13 +111,13 @@ struct tpm2_pcr_extend_in { > > > > > u8 digest[TPM_DIGEST_SIZE]; > > > > > } __packed; > > > > > > > > > > -struct tpm2_get_tpm_pt_in { > > > > > +struct tpm2_getcap_in { > > > > > __be32 cap_id; > > > > > __be32 property_id; > > > > > __be32 property_cnt; > > > > > } __packed; > > > > > > > > > > -struct tpm2_get_tpm_pt_out { > > > > > +struct tpm2_getcap_out { > > > > > u8 more_data; > > > > > __be32 subcap_id; > > > > > __be32 property_cnt; > > > > > @@ -140,8 +140,8 @@ union tpm2_cmd_params { > > > > > struct tpm2_pcr_read_in pcrread_in; > > > > > struct tpm2_pcr_read_out pcrread_out; > > > > > struct tpm2_pcr_extend_in pcrextend_in; > > > > > - struct tpm2_get_tpm_pt_in get_tpm_pt_in; > > > > > - struct tpm2_get_tpm_pt_out get_tpm_pt_out; > > > > > + struct tpm2_getcap_in getcap_in; > > > > > + struct tpm2_getcap_out getcap_out; > > > > > struct tpm2_get_random_in getrandom_in; > > > > > struct tpm2_get_random_out getrandom_out; > > > > > }; > > > > > @@ -435,16 +435,6 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) > > > > > return total ? total : -EIO; > > > > > } > > > > > > > > > > -#define TPM2_GET_TPM_PT_IN_SIZE \ > > > > > - (sizeof(struct tpm_input_header) + \ > > > > > - sizeof(struct tpm2_get_tpm_pt_in)) > > > > > - > > > > > -static const struct tpm_input_header tpm2_get_tpm_pt_header = { > > > > > - .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), > > > > > - .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE), > > > > > - .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) > > > > > -}; > > > > > - > > > > > /** > > > > > * Append TPMS_AUTH_COMMAND to the buffer. The buffer must be allocated with > > > > > * tpm_buf_alloc(). > > > > > @@ -750,35 +740,43 @@ out: > > > > > return rc; > > > > > } > > > > > > > > > > +#define TPM2_GETCAP_IN_SIZE \ > > > > > + (sizeof(struct tpm_input_header) + sizeof(struct tpm2_getcap_in)) > > > > > + > > > > > +static const struct tpm_input_header tpm2_getcap_header = { > > > > > + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), > > > > > + .length = cpu_to_be32(TPM2_GETCAP_IN_SIZE), > > > > > + .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) > > > > > +}; > > > > > + > > > > > /** > > > > > - * tpm2_get_tpm_pt() - get value of a TPM_CAP_TPM_PROPERTIES type property > > > > > - * @chip: TPM chip to use. > > > > > - * @property_id: property ID. > > > > > - * @value: output variable. > > > > > + * tpm2_getcap_cmd() - execute a TPM2_GetCapability command > > > > > + * @chip: TPM chip to use > > > > > + * @cap_id: capability ID > > > > > + * @property_id: property ID > > > > > + * @value: value of the property > > > > > * @desc: passed to tpm_transmit_cmd() > > > > > * > > > > > - * 0 is returned when the operation is successful. If a negative number is > > > > > - * returned it remarks a POSIX error code. If a positive number is returned > > > > > - * it remarks a TPM error. > > > > > + * Return: same as with tpm_transmit_cmd > > > > > */ > > > > > -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, > > > > > - const char *desc) > > > > > +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, > > > > > + u32 *value, const char *desc) > > > > > > > > This function currently returns single value "u32 *value" as output data. > > > > > > > > Some calling function expect list of values from capabilities output. > > > > For eg., get_active_banks() use TPM_CAP_PCRS capability to retrieve list of > > > > active banks. And this capability returns array of pcr selections(which is a > > > > struct representing each active PCR bank) > > > > > > > > I was thinking, can we define output parameter as struct of cap_id and union > > > > of expected cap_data ? > > > > > > Unless you have major concerns about performance, which I think are not > > > relevant here, calling this in a loop is perfectly adequate and a lot of > > > simpler than having a generic version (with moreData handling and > > > everything). > > > > > > I would rather see uses of struct cap_t to shrink than to expand. The > > > same goes for other horrific unions that exist today in the driver. > > > > If you are fine with this patch (give Reviewed-by) I can apply this > > and add to my 4.10 pull request so that we have common baseline to > > develop both event log and the resource manager. > > > > This is a very low risk commit to take to 4.10 because it is only used > > for interrupt generation in the TIS driver at the moment. > > Works for capabilities with more_data(yes) and single value properties. > > Reviewed-by: Nayna Jain <nayna@linux.vnet.ibm.com> > > Thanks & Regards, > - Nayna Thanks. Not taking this into release though because you don't need it as was the result of earlier discussion. /Jarkko ------------------------------------------------------------------------------
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 0fab6d5..8176f42 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -85,6 +85,10 @@ enum tpm2_capabilities { TPM2_CAP_TPM_PROPERTIES = 6, }; +enum tpm2_properties { + TPM2_PT_FAMILY_INDICATOR = 0x100, +}; + enum tpm2_startup_types { TPM2_SU_CLEAR = 0x0000, TPM2_SU_STATE = 0x0001, @@ -485,7 +489,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip, int tpm2_unseal_trusted(struct tpm_chip *chip, struct trusted_key_payload *payload, struct trusted_key_options *options); -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, u32 *value, const char *desc); int tpm2_auto_startup(struct tpm_chip *chip); diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index 2900e18..fcf3d86 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -111,13 +111,13 @@ struct tpm2_pcr_extend_in { u8 digest[TPM_DIGEST_SIZE]; } __packed; -struct tpm2_get_tpm_pt_in { +struct tpm2_getcap_in { __be32 cap_id; __be32 property_id; __be32 property_cnt; } __packed; -struct tpm2_get_tpm_pt_out { +struct tpm2_getcap_out { u8 more_data; __be32 subcap_id; __be32 property_cnt; @@ -140,8 +140,8 @@ union tpm2_cmd_params { struct tpm2_pcr_read_in pcrread_in; struct tpm2_pcr_read_out pcrread_out; struct tpm2_pcr_extend_in pcrextend_in; - struct tpm2_get_tpm_pt_in get_tpm_pt_in; - struct tpm2_get_tpm_pt_out get_tpm_pt_out; + struct tpm2_getcap_in getcap_in; + struct tpm2_getcap_out getcap_out; struct tpm2_get_random_in getrandom_in; struct tpm2_get_random_out getrandom_out; }; @@ -435,16 +435,6 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max) return total ? total : -EIO; } -#define TPM2_GET_TPM_PT_IN_SIZE \ - (sizeof(struct tpm_input_header) + \ - sizeof(struct tpm2_get_tpm_pt_in)) - -static const struct tpm_input_header tpm2_get_tpm_pt_header = { - .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), - .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE), - .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) -}; - /** * Append TPMS_AUTH_COMMAND to the buffer. The buffer must be allocated with * tpm_buf_alloc(). @@ -750,35 +740,43 @@ out: return rc; } +#define TPM2_GETCAP_IN_SIZE \ + (sizeof(struct tpm_input_header) + sizeof(struct tpm2_getcap_in)) + +static const struct tpm_input_header tpm2_getcap_header = { + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS), + .length = cpu_to_be32(TPM2_GETCAP_IN_SIZE), + .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY) +}; + /** - * tpm2_get_tpm_pt() - get value of a TPM_CAP_TPM_PROPERTIES type property - * @chip: TPM chip to use. - * @property_id: property ID. - * @value: output variable. + * tpm2_getcap_cmd() - execute a TPM2_GetCapability command + * @chip: TPM chip to use + * @cap_id: capability ID + * @property_id: property ID + * @value: value of the property * @desc: passed to tpm_transmit_cmd() * - * 0 is returned when the operation is successful. If a negative number is - * returned it remarks a POSIX error code. If a positive number is returned - * it remarks a TPM error. + * Return: same as with tpm_transmit_cmd */ -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, - const char *desc) +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id, + u32 *value, const char *desc) { struct tpm2_cmd cmd; int rc; - cmd.header.in = tpm2_get_tpm_pt_header; - cmd.params.get_tpm_pt_in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES); - cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(property_id); - cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1); + cmd.header.in = tpm2_getcap_header; + cmd.params.getcap_in.cap_id = cpu_to_be32(cap_id); + cmd.params.getcap_in.property_id = cpu_to_be32(property_id); + cmd.params.getcap_in.property_cnt = cpu_to_be32(1); rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, desc); if (!rc) - *value = be32_to_cpu(cmd.params.get_tpm_pt_out.value); + *value = be32_to_cpu(cmd.params.getcap_out.value); return rc; } -EXPORT_SYMBOL_GPL(tpm2_get_tpm_pt); +EXPORT_SYMBOL_GPL(tpm2_getcap_cmd); #define TPM2_STARTUP_IN_SIZE \ (sizeof(struct tpm_input_header) + \ @@ -978,10 +976,10 @@ int tpm2_probe(struct tpm_chip *chip) struct tpm2_cmd cmd; int rc; - cmd.header.in = tpm2_get_tpm_pt_header; - cmd.params.get_tpm_pt_in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES); - cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(0x100); - cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1); + cmd.header.in = tpm2_getcap_header; + cmd.params.getcap_in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES); + cmd.params.getcap_in.property_id = cpu_to_be32(TPM2_PT_FAMILY_INDICATOR); + cmd.params.getcap_in.property_cnt = cpu_to_be32(1); rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), 0, NULL); if (rc < 0) diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index e3bf31b..792ccd1 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -528,7 +528,8 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) cap_t cap; if (chip->flags & TPM_CHIP_FLAG_TPM2) - return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc); + return tpm2_getcap_cmd(chip, TPM2_CAP_TPM_PROPERTIES, + TPM2_PT_FAMILY_INDICATOR, &cap2, desc); else return tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc); }
Refactored tpm2_get_tpm_pt to tpm2_getcap_cmd, which means that it also takes capability ID as input. This is required to access TPM_CAP_HANDLES, which contains metadata needed for swapping transient data. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/char/tpm/tpm.h | 6 +++- drivers/char/tpm/tpm2-cmd.c | 64 ++++++++++++++++++++--------------------- drivers/char/tpm/tpm_tis_core.c | 3 +- 3 files changed, 38 insertions(+), 35 deletions(-)