Re: [pve-devel] [PATCH qemu-server v9 1/3] add C program to get hardware capabilities from CPUID

[Fiona Ebner <f.ebner@proxmox.com>]

Am 26.04.24 um 11:58 schrieb Markus Frank:
> Implement a systemd service that runs a C program that extracts AMD
> SEV hardware information 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/qemu-server/hw-params.json.
> 

In the code, it's /run/qemu-server/host-hw-capabilities.json

> This programm can also be used to read and save other hardware
> information at boot time.
> 

Nit: s/programm/program/

Question is if it should live in a more central place/package then? E.g.
what if the next time we need to query a capability, it's needed by
pve-container? But I guess we can still move it later when the need arises.

> diff --git a/query-machine-capabilities/Makefile b/query-machine-capabilities/Makefile
> new file mode 100644
> index 0000000..c5f6348
> --- /dev/null
> +++ b/query-machine-capabilities/Makefile
> @@ -0,0 +1,21 @@
> +DESTDIR=
> +PREFIX=/usr
> +SBINDIR=${PREFIX}/libexec/qemu-server

Why call it SBINDIR?

[...]

> +
> +    const char *path = "/run/qemu-server/";
> +    // Check that the directory exists and create it if it does not.
> +    struct stat statbuf;
> +    int stats = stat(path, &statbuf);

A bit confused about the name "stats", because this is just the success
indicator. Maybe "ret" or something that doesn't sound like it's the
actual stats. You could also branch on the function call itself and on
success once more with S_ISDR, to not miss the "success, but not a
directory" case, see below.

> +    if (stats == 0 && S_ISDIR(statbuf.st_mode)) {
> +	printf("Directory %s already exists.\n", path);
> +    } else if (errno == ENOENT) {
> +	printf("%s does not exist. Creating directory.\n", path);

Not sure if the above two messages are worth logging, but either way is
fine.

> +	if (mkdir(path, 0755) != 0) {
> +	    printf("Error creating directory %s: %s\n", path, strerror(errno));
> +	    return 1;
> +	}
> +    } else {
> +	printf("Error checking path %s: %s\n", path, strerror(errno));

Unlikely, but if errno is 0 (can be when the file exists but is not a
directory for example) then strerror(errno) will be "Success" and result
in a confusing error message.

> +	return 1;
> +    }
> +
> +    FILE *file;
> +    const char *filename = "/run/qemu-server/host-hw-capabilities.json";
> +    file = fopen(filename, "w");
> +    if (file == NULL) {
> +	perror("Error opening file");
> +	return 1;
> +    }
> +
> +    fprintf(file,
> +	"{"
> +	" \"amd-sev\": {"
> +	" \"cbitpos\": %u,"
> +	" \"reduced-phys-bits\": %u,"
> +	" \"sev-support\": %s,"
> +	" \"sev-support-es\": %s,"
> +	" \"sev-support-snp\": %s"
> +	" }"
> +	" }\n",
> +	cbitpos,
> +	reduced_phys_bits,
> +	sev_support ? "true" : "false",
> +	sev_es_support ? "true" : "false",
> +	sev_snp_support ? "true" : "false"
> +    );
> +
> +    fclose(file);

Maybe check for errors for fprintf and fclose?

> +    return 0;
> +}
> diff --git a/query-machine-capabilities/query-machine-capabilities.service b/query-machine-capabilities/query-machine-capabilities.service
> new file mode 100644
> index 0000000..f926074
> --- /dev/null
> +++ b/query-machine-capabilities/query-machine-capabilities.service
> @@ -0,0 +1,12 @@
> +[Unit]
> +Description=read AMD SEV parameters

Should also be updated to reflect the more general (future) use case.

> +RequiresMountsFor=/run
> +Before=pve-ha-lrm.service
> +Before=pve-guests.service
> +
> +[Service]
> +ExecStart=/usr/libexec/qemu-server/query-machine-capabilities
> +Type=oneshot
> +
> +[Install]
> +WantedBy=multi-user.target


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