Re: [pve-devel] [PATCH qemu-server v6 1/3] add C program to get AMD SEV hardware parameters from CPUID

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 19/04/2024 um 12:59 schrieb Markus Frank:
> Implement a systemd service that runs a C program that extracts AMD SEV
> hardware parameters such as reduced-phys-bios and cbitpos from CPUID at boot
> time, looks if SEV, SEV-ES & SEV-SNP are enabled, and outputs these details
> as JSON to /run/amd-sev-params.

general comments:

I'd not call this SEV support, we will probably re-use this to get other
features from both Intel and AMD in the future, so I'd name it something
slightly more general like "query-machine-capabilities"

The output file should not pollute top-level /run, we already have
/run/qemu-server in use, so that could be used here too.

> 

misses a co-authored or originally-by me trailer ;-)

> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  Makefile                                |  1 +
>  amd-sev-support/Makefile                | 21 +++++++++++
>  amd-sev-support/amd-sev-support.c       | 48 +++++++++++++++++++++++++
>  amd-sev-support/amd-sev-support.service | 12 +++++++
>  4 files changed, 82 insertions(+)
>  create mode 100644 amd-sev-support/Makefile
>  create mode 100644 amd-sev-support/amd-sev-support.c
>  create mode 100644 amd-sev-support/amd-sev-support.service
> 
> diff --git a/Makefile b/Makefile
> index 133468d..ccd12a1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -65,6 +65,7 @@ install: $(PKGSOURCES)
>  	install -m 0644 -D bootsplash.jpg $(DESTDIR)/usr/share/$(PACKAGE)
>  	$(MAKE) -C PVE install
>  	$(MAKE) -C qmeventd install
> +	$(MAKE) -C amd-sev-support install
>  	$(MAKE) -C qemu-configs install
>  	$(MAKE) -C vm-network-scripts install
>  	install -m 0755 qm $(DESTDIR)$(SBINDIR)
> diff --git a/amd-sev-support/Makefile b/amd-sev-support/Makefile
> new file mode 100644
> index 0000000..022ed94
> --- /dev/null
> +++ b/amd-sev-support/Makefile
> @@ -0,0 +1,21 @@
> +DESTDIR=
> +PREFIX=/usr
> +SBINDIR=${PREFIX}/libexec/qemu-server
> +SERVICEDIR=/lib/systemd/system
> +
> +CC ?= gcc
> +CFLAGS += -O2 -fanalyzer -Werror -Wall -Wextra -Wpedantic -Wtype-limits -Wl,-z,relro -std=gnu11
> +
> +amd-sev-support: amd-sev-support.c
> +	$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS)
> +
> +.PHONY: install
> +install: amd-sev-support
> +	install -d ${DESTDIR}/${SBINDIR}
> +	install -d ${DESTDIR}${SERVICEDIR}
> +	install -m 0644 amd-sev-support.service ${DESTDIR}${SERVICEDIR}
> +	install -m 0755 amd-sev-support ${DESTDIR}${SBINDIR}
> +
> +.PHONY: clean
> +clean:
> +	rm -f amd-sev-support
> diff --git a/amd-sev-support/amd-sev-support.c b/amd-sev-support/amd-sev-support.c
> new file mode 100644
> index 0000000..73a7bd8
> --- /dev/null
> +++ b/amd-sev-support/amd-sev-support.c
> @@ -0,0 +1,48 @@
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <stdbool.h>
> +
> +int main() {
> +    uint32_t eax, ebx, ecx, edx;
> +
> +    // query Encrypted Memory Capabilities, see:
> +    // https://en.wikipedia.org/wiki/CPUID#EAX=8000001Fh:_Encrypted_Memory_Capabilities
> +    uint32_t query_function = 0x8000001F;
> +    asm volatile("cpuid"
> +	 : "=a"(eax), "=b"(ebx), "=c"(ecx), "=d"(edx)
> +	 : "0"(query_function)
> +    );
> +
> +    bool sev_support = (eax & (1<<1)) != 0;
> +    bool sev_es_support = (eax & (1<<3)) != 0;
> +    bool sev_snp_support = (eax & (1<<4)) != 0;
> +
> +    uint8_t cbitpos = ebx & 0x3f;
> +    uint8_t reduced_phys_bits = (ebx >> 6) & 0x3f;
> +
> +    FILE *file;
> +    char *filename = "/run/amd-sev-params";

besides saving this in the /run/qemu-server directory it'd be additionally
nice to add a ".json" file type suffix to clarify what format this
is in.

> +
> +    file = fopen(filename, "w");
> +    if (file == NULL) {
> +	perror("Error opening file");
> +	return 1;
> +    }
> +
> +    fprintf(file, "{"
> +	" \"cbitpos\": %u,"
> +	" \"reduced-phys-bits\": %u,"
> +	" \"sev\": %s,"
> +	" \"sev-es\": %s,"
> +	" \"sev-snp\": %s"
> +	" }\n",
> +	cbitpos,
> +	reduced_phys_bits,
> +	sev_support ? "true" : "false",
> +	sev_es_support ? "true" : "false",
> +	sev_snp_support ? "true" : "false"
> +    );
> +
> +    fclose(file);
> +    return 0;
> +}
> diff --git a/amd-sev-support/amd-sev-support.service b/amd-sev-support/amd-sev-support.service
> new file mode 100644
> index 0000000..466dd0a
> --- /dev/null
> +++ b/amd-sev-support/amd-sev-support.service
> @@ -0,0 +1,12 @@
> +[Unit]
> +Description=Read AMD SEV Parameters
> +RequiresMountsFor=/run
> +Before=pve-ha-lrm.service
> +Before=pve-guests.service
> +
> +[Service]
> +ExecStart=/usr/libexec/qemu-server/amd-sev-support
> +Type=forking

why is this "forking", you do not even fork once?! (nor should you)
I'd use the "oneshot" type that is designed for such use cases.
 

Korrigieren

Schließen

Grammatik

This sentence does not start with an uppercase letter.

MissesIgnorieren



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel