Re: [RFC PATCH v3 25/26] target/arm/kvm-rme: Add measurement log

[Jean-Philippe Brucker]

On Mon, Dec 09, 2024 at 05:08:37PM -0500, Stefan Berger wrote:
> >   typedef struct {
> >       hwaddr base;
> >       hwaddr size;
> > +    uint8_t *data;
> > +    RmeLogFiletype *filetype;
> >   } RmeRamRegion;
> > +typedef struct {
> > +    char        signature[16];
> > +    char        name[32];
> > +    char        version[40];
> > +    uint64_t    ram_size;
> > +    uint32_t    num_cpus;
> > +    uint64_t    flags;
> > +} EventLogVmmVersion;
> > +
> > +typedef struct {
> > +    uint32_t    id;
> > +    uint32_t    data_size;
> > +    uint8_t     data[];
> > +} EventLogTagged;
> > +
> 
> 
> > +#define EVENT_LOG_TAG_REALM_CREATE  1
> > +#define EVENT_LOG_TAG_INIT_RIPAS    2
> > +#define EVENT_LOG_TAG_REC_CREATE    3
> > +
> If these are ARM-related structures and constants from a document you may
> want to mention the document you got them from.

Agreed. At the moment they're just numbers and structures I made up [1].
I'm not certain in which standard they should go. TCG would seem
appropriate, or IETF is also used for protocols related to confidential
computing attestation. Or maybe it could live in the reference verifier
documentation. QEMU docs wouldn't be the best place because VMMs might
been reluctant to adopt it because they don't consider it a standard (like
cloud-hv and fw_cfg)

When researching this I found TCG event types and payloads that only seem
to be documented in their respective project:
* efistub [2] with
  * EV_EVENT_TAG, id=0x8F3B22EC, data="Linux initrd",
  * EV_EVENT_TAG, id=0x8F3B22ED, data="LOADED_IMAGE::LoadOptions"
* grub [3] with a few EV_IPL
* systemd [4] with various EV_EVENT_TAG and EV_IPL

I'm wondering if we could create a common registry somewhere for these,
like IANA or somewhere informal.


[1] 
https://github.com/veraison/cca-realm-measurements/blob/main/docs/measurement-log.md#rim-log
[2] 
https://lore.kernel.org/all/20211119114745.1560453-1-ilias.apalodimas@linaro.org/
[3] https://www.gnu.org/software/grub/manual/grub/html_node/Measured-Boot.html
[4] https://systemd.io/TPM2_PCR_MEASUREMENTS/


> > +/* Log VM type and Realm Descriptor create */
> > +static int rme_log_realm_create(Error **errp)
> > +{
> > +    int ret;
> > +    ARMCPU *cpu;
> > +    EventLogVmmVersion vmm_version = {
> > +        .signature = "VM VERSION",
> > +        .name = "QEMU",
> > +        .version = "9.1", /* TODO: dynamic */
> 
> $ grep -r QEMU_VERSION_M build/
> build/config-host.h:#define QEMU_VERSION_MAJOR 9
> build/config-host.h:#define QEMU_VERSION_MICRO 93
> build/config-host.h:#define QEMU_VERSION_MINOR 1
> 
> $ cat VERSION
> 9.1.93

Ah yes that would work, thank you

> > +static int rme_log_rec(uint64_t flags, uint64_t pc, uint64_t gprs[8], 
> > Error **errp)
> > +{
> 
> $ ./scripts/checkpatch.pl ./tmp/*.patch
> [...]
> Checking ./tmp/0002-target-arm-kvm-rme-Add-measurement-log.patch...
> WARNING: line over 80 characters
> #353: FILE: target/arm/kvm-rme.c:303:
> +static int rme_log_rec(uint64_t flags, uint64_t pc, uint64_t gprs[8], Error
> **errp)
> 
> May want to run this on all patches.
> 
> Rest LGTM.

Thank you!

Jean