Re: [PATCH v3 1/2] acpi: Make TPM version configurable.

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, Apr 27, 2023 at 8:49 AM Jennifer Herbert
<jennifer.herbert@xxxxxxxxxx> wrote:
>
>
> On 26/04/2023 21:27, Jason Andryuk wrote:
> > Hi, Jennifer,
> >
> > On Tue, Apr 25, 2023 at 1:48 PM Jennifer Herbert
> > <jennifer.herbert@xxxxxxxxxx> wrote:
> >> This patch makes the TPM version, for which the ACPI libary probes, 
> >> configurable.
> >> If acpi_config.tpm_verison is set to 1, it indicates that 1.2 (TCPA) 
> >> should be probed.
> >> I have also added to hvmloader an option to allow setting this new config, 
> >> which can
> >> be triggered by setting the platform/tpm_version xenstore key.
> >>
> >> Signed-off-by: Jennifer Herbert <jennifer.herbert@xxxxxxxxxx>
> > ...
> >> --- a/tools/libacpi/build.c
> >> +++ b/tools/libacpi/build.c
> >> @@ -409,38 +409,47 @@ static int construct_secondary_tables(struct 
> >> acpi_ctxt *ctxt,
> > ...
> >> +        switch ( config->tpm_version )
> >>           {
> >> -            tcpa->lasa = ctxt->mem_ops.v2p(ctxt, lasa);
> >> -            tcpa->laml = ACPI_2_0_TCPA_LAML_SIZE;
> >> -            memset(lasa, 0, tcpa->laml);
> >> -            set_checksum(tcpa,
> >> -                         offsetof(struct acpi_header, checksum),
> >> -                         tcpa->header.length);
> >> +        case 0: /* Assume legacy code wanted tpm 1.2 */
> > This shouldn't be reached, since tpm_version == 0 won't have
> > ACPI_HAS_TPM set.  Still, do you want to make it a break or drop the
> > case to avoid falling through to the TPM1.2 code?
>
> So there was concerns in v2 about backward compatilibity before of this
> area of code.  The exact nature of the concern was vauge, so I made this
> very conservative.   This was intended to mitigate agaist this code
> being run, but with the structure being set up with something other
> then  the code in tools/firmware/hvmloader/util.c.  Any such alaternate
> code would set the tpm flag, but not know about the version field, and
> so leave it at zero.  In this case, dropping into 1.2 probing would be
> the correct solution.
>
> As you say, in the use cases I'm farmiliar with, this would not get
> reached, so shoudn't affect anything.
> Haveing a break or dropping the case would result in it silently
> ignoring the 'invalid' tpm configuration, so if not compatibilty mode,
> I'd personally prefer some sort of assert, although i'm not sure how
> best to do that in this code.

Ok, sounds good to leave it as you wrote it here.  The split of
hvmloader and libacpi requires the backwards compatible approach
inside libacpi.

Thanks,
Jason