Re: [pve-devel] [PATCH qemu-server v2 2/3] Add check for TDX support

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

Am 04.10.25 um 3:23 PM schrieb Anton Iacobaeus:
> From: Philipp Giersfeld <philipp.giersfeld@canarybit.eu>
> 
> Check whether TDX is enabled on this machine. Instead of using CPUID
> like AMD SEV, Intel TDX enablement can be verified by reading an MSR
> (https://cc-enabling.trustedservices.intel.com/intel-tdx-enabling-guide/05/host_os_setup/).
> 
> Signed-off-by: Philipp Giersfeld <philipp.giersfeld@canarybit.eu>
> Signed-off-by: Anton Iacobaeus <anton.iacobaeus@canarybit.eu>
> ---
>  .../query-machine-capabilities.c              | 97 ++++++++++++++++---
>  src/usr/modules-load.conf                     |  1 +
>  2 files changed, 85 insertions(+), 13 deletions(-)
> 
> diff --git a/src/query-machine-capabilities/query-machine-capabilities.c b/src/query-machine-capabilities/query-machine-capabilities.c
> index 0c522afc..5bc39450 100644
> --- a/src/query-machine-capabilities/query-machine-capabilities.c
> +++ b/src/query-machine-capabilities/query-machine-capabilities.c
> @@ -4,6 +4,8 @@
>  #include <sys/stat.h>
>  #include <errno.h>
>  #include <string.h>
> +#include <unistd.h>
> +#include <fcntl.h>
>  
>  #define eprintf(...) fprintf(stderr, __VA_ARGS__)
>  
> @@ -18,17 +20,56 @@ typedef struct {
>  
>      uint8_t cbitpos;
>      uint8_t reduced_phys_bits;
> -} cpu_caps_t;
> +} cpu_caps_amd_sev_t;
>  
> -void query_cpu_capabilities(cpu_caps_t *res) {
> +typedef struct {
> +    bool tdx_support;

Should we also check for Intel TME and SGX as described in the guide you
referenced?

> +} cpu_caps_intel_tdx_t;
> +
> +int read_msr(uint32_t msr_index, unsigned int *value) {
> +    int64_t data;

It would be nicer if 'data' and 'value' are the same type (apart from
the pointer). Both could be uint64_t, or? For value, it should also be
adapted on the call site if going for this.

> +    char* msr_file_name = "/dev/cpu/0/msr";
> +    int fd;
> +
> +    fd = open(msr_file_name, O_RDONLY);
> +    if (fd < 0) {
> +        if (errno == ENXIO) {
> +            eprintf("rdmsr: No CPU 0\n");
> +            return -1;
> +        } else if (errno == EIO) {
> +            eprintf("rdmsr: CPU doesn't support MSRs\n");
> +            return -1;
> +        } else {
> +            perror("rdmsr: failed to open MSR");
> +            return -1;
> +        }
> +    }
> +
> +    if (pread(fd, &data, sizeof data, msr_index) != sizeof data) {

Style nit: I'd prefer sizeof() with parentheses, because it eases
reading a bit and is more common in my experience

> +        if (errno == EIO) {
> +            eprintf("rdmsr: CPU cannot read MSR 0x%08x\n", msr_index);
> +            return -1;
> +        } else {
> +            perror("rdmsr: pread");
> +            return -1;
> +        }
> +    }
> +
> +    *value = data;
> +
> +    close(fd);
> +    return 0;
> +}
> +
> +void query_cpu_capabilities_sev(cpu_caps_amd_sev_t *res) {
>      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)
> +        : "=a"(eax), "=b"(ebx), "=c"(ecx), "=d"(edx)
> +        : "0"(query_function)
>      );
>  
>      res->sev_support = (eax & (1<<1)) != 0;
> @@ -39,6 +80,18 @@ void query_cpu_capabilities(cpu_caps_t *res) {
>      res->reduced_phys_bits = (ebx >> 6) & 0x3f;
>  }
>  
> +int query_cpu_capabilities_tdx(cpu_caps_intel_tdx_t *res) {
> +    unsigned int value;
> +
> +    if (read_msr(0x1401, &value) == 0) {
> +        res->tdx_support = (value & (1<<11));
> +    } else {
> +        eprintf("Error reading MSR, Intel TDX support undetermined\n");

Nit: Since the register is expected to not be there for many CPUs,
should we rather say "Cannot read" instead of "Error reading"? Error
makes it sound like something is wrong which is not necessarily the
case. Just to avoid admins getting nervous ;) Or maybe even just reduce
the message to "Intel TDX support undetermined", the actual info what
happened is already printed inside read_msr().

> +        return -1;
> +    }
> +    return 0;
> +}
> +
>  int prepare_output_directory() {
>      // Check that the directory exists and create it if it does not.
>      struct stat statbuf;
> @@ -65,8 +118,8 @@ int main() {
>          return 1;
>      }
>  
> -    cpu_caps_t caps;
> -    query_cpu_capabilities(&caps);
> +    cpu_caps_amd_sev_t caps_sev;
> +    query_cpu_capabilities_sev(&caps_sev);

Pre-existing, but should we try to detect whether the CPU is AMD or
Intel right away and then only do the relevant call? Otherwise, we risk
the JSON claiming support for the other vendor's feature, because we
query data in a way that doesn't make sense for the current vendor. Also
to not show the error messages for AMD CPUs on every boot:

Oct 08 11:50:21 pve9a1 query-machine-capabilities[1139]: rdmsr: CPU
cannot read MSR 0x00001401
Oct 08 11:50:21 pve9a1 query-machine-capabilities[1139]: Error reading
MSR, Intel TDX support undetermined

>  
>      FILE *file = fopen(OUTPUT_PATH, "w");
>      if (file == NULL) {
> @@ -82,18 +135,36 @@ int main() {
>          " \"sev-support\": %s,"
>          " \"sev-support-es\": %s,"
>          " \"sev-support-snp\": %s"
> -        " }"
> -        " }\n",
> -        caps.cbitpos,
> -        caps.reduced_phys_bits,
> -        caps.sev_support ? "true" : "false",
> -        caps.sev_es_support ? "true" : "false",
> -        caps.sev_snp_support ? "true" : "false"
> +        " }",
> +        caps_sev.cbitpos,
> +        caps_sev.reduced_phys_bits,
> +        caps_sev.sev_support ? "true" : "false",
> +        caps_sev.sev_es_support ? "true" : "false",
> +        caps_sev.sev_snp_support ? "true" : "false"
>      );
>      if (ret < 0) {
>          eprintf("Error writing to file '" OUTPUT_PATH "': %s\n", strerror(errno));
>      }
>  
> +    cpu_caps_intel_tdx_t caps_tdx;
> +    if (query_cpu_capabilities_tdx(&caps_tdx) == 0) {
> +        ret = fprintf(file,
> +            ","
> +            " \"intel-tdx\": {"
> +            " \"tdx-support\": %s"
> +            " }",
> +            caps_tdx.tdx_support ? "true" : "false"
> +        );
> +        if (ret < 0) {
> +            eprintf("Error writing to file '" OUTPUT_PATH "': %s\n", strerror(errno));
> +        }
> +    }
> +
> +    ret = fprintf(file, " }\n");
> +    if (ret < 0) {
> +        eprintf("Error writing to file '" OUTPUT_PATH "': %s\n", strerror(errno));
> +    }
> +
>      ret = fclose(file);
>      if (ret != 0) {
>          eprintf("Error closing file '" OUTPUT_PATH "': %s\n", strerror(errno));
> diff --git a/src/usr/modules-load.conf b/src/usr/modules-load.conf
> index aee7d42a..f45d256b 100644
> --- a/src/usr/modules-load.conf
> +++ b/src/usr/modules-load.conf
> @@ -1 +1,2 @@
>  vhost_net
> +msr



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