From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id EA2931FF38C for ; Fri, 17 May 2024 14:46:18 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EF3DD12910; Fri, 17 May 2024 14:46:30 +0200 (CEST) Message-ID: <2253550b-ebbf-421c-8ab9-7730e6157249@proxmox.com> Date: Fri, 17 May 2024 14:45:37 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Markus Frank References: <20240426095829.746663-1-m.frank@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20240426095829.746663-1-m.frank@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.115 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment PROLO_LEO1 0.1 Meta Catches all Leo drug variations so far SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [multi-user.target] Subject: Re: [pve-devel] [PATCH qemu-server v9 1/3] add C program to get hardware capabilities from CPUID X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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