Re: [Qemu-devel] [PATCH v8 5/5] PoC: tpm: add ACPI memory clear interface

[Marc-André Lureau]

Hi

On Mon, Jul 23, 2018 at 1:08 PM, Igor Mammedov <address@hidden> wrote:
> On Tue, 17 Jul 2018 17:39:12 +0200
> Marc-André Lureau <address@hidden> wrote:
>
>> Hi
>>
>> On Tue, Jul 17, 2018 at 9:57 AM, Igor Mammedov <address@hidden> wrote:
>> > On Mon, 16 Jul 2018 14:59:48 +0200
>> > Marc-André Lureau <address@hidden> wrote:
>> >
>> >> This allows to pass the last failing test from the Windows HLK TPM 2.0
>> >> TCG PPI 1.3 tests.
>> >>
>> >> The interface is described in the "TCG Platform Reset Attack
>> >> Mitigation Specification", chapter 6 "ACPI _DSM Function". According
>> >> to Laszlo, it's not so easy to implement in OVMF, he suggested to do
>> >> it in qemu instead. It is relatively simple to implement in QEMU, but
>> >> make this a proof-of-concept PATCH for discussion, since this is not
>> >> a conventional approach.
>> >>
>> >> Signed-off-by: Marc-André Lureau <address@hidden>
>> >> ---
>> >>  hw/i386/acpi-build.c | 46 ++++++++++++++++++++++++++++++++++++++++++++
>> >>  hw/tpm/tpm_ppi.c     | 25 ++++++++++++++++++++++++
>> >>  docs/specs/tpm.txt   |  2 ++
>> >>  hw/tpm/trace-events  |  3 +++
>> >>  4 files changed, 76 insertions(+)
>> >>
>> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> >> index 882fc27846..7e3c217076 100644
>> >> --- a/hw/i386/acpi-build.c
>> >> +++ b/hw/i386/acpi-build.c
>> >> @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
>> >>      pprq = aml_name("PPRQ");
>> >>      pprm = aml_name("PPRM");
>> >>
>> >> +    aml_append(dev,
>> >> +               aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
>> >> +                                    aml_int(TPM_PPI_ADDR_BASE + 0x200),
>> >> +                                    0x1));
>> >> +    field = aml_field("TPP3", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
>> >                                  ^^^ AML_BYTE_ACC
>>
>> ok
>>
>>
>> >
>> >> +    aml_append(field, aml_named_field("MOVV", 8));
>> >> +    aml_append(dev, field);
>> >>      /*
>> >>       * DerefOf in Windows is broken with SYSTEM_MEMORY.  Use a dynamic
>> >>       * operation region inside of a method for getting FUNC[op].
>> >> @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
>> >>              aml_append(ifctx, aml_return(aml_buffer(1, zerobyte)));
>> >>          }
>> >>          aml_append(method, ifctx);
>> >> +
>> >> +        ifctx = aml_if(
>> >> +            aml_equal(uuid,
>> >> +                      
>> >> aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
>> >> +        {
>> >> +            /* standard DSM query function */
>> >> +            ifctx2 = aml_if(aml_equal(function, zero));
>> >> +            {
>> >> +                uint8_t byte_list[1] = { 0x03 };
>> >> +                aml_append(ifctx2, aml_return(aml_buffer(1, byte_list)));
>> >> +            }
>> >> +            aml_append(ifctx, ifctx2);
>> >> +
>> >> +            /*
>> >> +             * TCG Platform Reset Attack Mitigation Specification 1.0 
>> >> Ch.6
>> >> +             *
>> >> +             * Arg 2 (Integer): Function Index = 1
>> >> +             * Arg 3 (Package): Arguments = Package: Type: Integer
>> >> +             *                  Operation Value of the Request
>> >> +             * Returns: Type: Integer
>> >> +             *          0: Success
>> >> +             *          1: General Failure
>> >> +             */
>> >> +            ifctx2 = aml_if(aml_equal(function, one));
>> >> +            {
>> >> +                aml_append(ifctx2,
>> >> +                           aml_store(aml_derefof(aml_index(arguments, 
>> >> zero)),
>> >> +                                     op));
>> >> +                {
>> >> +                    aml_append(ifctx2, aml_store(op, aml_name("MOVV")));
>> >> +
>> >> +                    /* 0: success */
>> >> +                    aml_append(ifctx2, aml_return(zero));
>> >> +                }
>> >> +            }
>> >> +            aml_append(ifctx, ifctx2);
>> >> +        }
>> >> +        aml_append(method, ifctx);
>> >>      }
>> >> +
>> >>      aml_append(dev, method);
>> >>  }
>> >>
>> >> diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
>> >> index 8b46b9dd4b..64e8d828e8 100644
>> >> --- a/hw/tpm/tpm_ppi.c
>> >> +++ b/hw/tpm/tpm_ppi.c
>> >> @@ -16,8 +16,31 @@
>> >>  #include "qapi/error.h"
>> >>  #include "cpu.h"
>> >>  #include "sysemu/memory_mapping.h"
>> >> +#include "sysemu/reset.h"
>> >>  #include "migration/vmstate.h"
>> >>  #include "tpm_ppi.h"
>> >> +#include "trace.h"
>> >> +
>> >> +static void tpm_ppi_reset(void *opaque)
>> >> +{
>> >> +    TPMPPI *tpmppi = opaque;
>> >> +    char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
>> >> +
>> >> +    if (ptr[0x200] & 0x1) {
>> >> +        GuestPhysBlockList guest_phys_blocks;
>> >> +        GuestPhysBlock *block;
>> >> +
>> >> +        guest_phys_blocks_init(&guest_phys_blocks);
>> >> +        guest_phys_blocks_append(&guest_phys_blocks);
>> >> +        QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
>> >> +            trace_tpm_ppi_memset(block->host_addr,
>> >> +                             block->target_end - block->target_start);
>> >> +            memset(block->host_addr, 0,
>> >> +                   block->target_end - block->target_start);
>> >> +        }
>> >> +        guest_phys_blocks_free(&guest_phys_blocks);
>> >> +    }
>> >> +}
>> >>
>> >>  bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
>> >>                    hwaddr addr, Object *obj, Error **errp)
>> >> @@ -27,5 +50,7 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion 
>> >> *m,
>> >>      vmstate_register_ram(&tpmppi->ram, DEVICE(obj));
>> >>
>> >>      memory_region_add_subregion(m, addr, &tpmppi->ram);
>> >> +    qemu_register_reset(tpm_ppi_reset, tpmppi);
>> > is it necessary,
>> > shouldn't it be reset by bus initiated reset?
>>
>> There is no bus for CRB device (we could eventually move it to SYSBUS,
>> but there was some complication with piix iirc).
>>
>> Other than that, the role for BusClass.reset() isn't so clear to me,
>> but it seems to be linked with the same qemu reset handler.
>
> I'd suggest to drop qemu_register_reset() here, export tpm_ppi_reset()
> and call it explicitly from tpm_crb_reset() and tpm_tis_reset() if
> ppi is enabled.
>

should be fine as well

thanks