* Re: [PATCH 1/1] virt: tdx-guest: Optimize the get-quote polling interval time
From: Kuppuswamy Sathyanarayanan @ 2026-02-20 18:45 UTC (permalink / raw)
To: Jun Miao, kas, dave.hansen, rick.p.edgecombe
Cc: linux-coco, kvm, linux-kernel
In-Reply-To: <20260211085801.4036464-2-jun.miao@intel.com>
Hi Miao,
On 2/11/2026 12:58 AM, Jun Miao wrote:
> The TD guest sends TDREPORT to the TD Quoting Enclave via a vsock or
> a tdvmcall. In general, vsock is indeed much faster than tdvmcall,
> and Quote requests usually take a few millisecond to complete rather
> than seconds based on actual measurements.
>
> The following get quote time via tdvmcall were obtained on the GNR:
>
> | msleep_interruptible(time) | 1s | 5ms | 1ms |
> | ------------------------------ | -------- | -------- | ---------- |
> | Duration | 1.004 s | 1.005 s | 1.036 s |
> | Total(Get Quote) | 167 | 142 | 167 |
> | Success: | 167 | 142 | 167 |
> | Failure: | 0 | 0 | 0 |
> | Avg total / 1s | 0.97 | 141.31 | 166.35 |
> | Avg success / 1s | 0.97 | 141.31 | 166.35 |
> | Avg total / 1s / thread | 0.97 | 141.31 | 166.35 |
> | Avg success / 1s / thread | 0.97 | 141.31 | 166.35 |
> | Min elapsed_time | 1025.95ms| 6.85 ms | 2.99 ms |
> | Max elapsed_time | 1025.95ms| 10.93 ms | 10.76 ms |
>
Thanks for sharing the data!
> According to trace analysis, the typical execution tdvmcall get the
> quote time is 4 ms. Therefore, 5 ms is a reasonable balance between
> performance efficiency and CPU overhead.
Since the average is 4 ms, why choose 5ms?
>
> And compared to the previous throughput of one request per second,
> the current 5ms can get 142 requests per second delivers a
> 142× performance improvement, which is critical for high-frequency
> use cases without vsock.
Is this addressing a real customer issue or a theoretical improvement?
If this is solving a real problem, could you share more details about
the use case and Quoting Service implementation you're testing against?
I ask because the Quote completion time depends heavily on the Quoting
Service implementation, which varies by deployment. Since we're optimizing
for performance, I'm wondering if we should consider an interrupt-based
approach using the SetupEventNotifyInterrupt TDVMCALL instead of polling.
>
> So, change the 1s (MSEC_PER_SEC) -> 5ms (MSEC_PER_SEC / 200)
>
> Signed-off-by: Jun Miao <jun.miao@intel.com>
> ---
> drivers/virt/coco/tdx-guest/tdx-guest.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 4e239ec960c9..71d2d7304b1a 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -251,11 +251,11 @@ static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeou
> int i = 0;
>
> /*
> - * Quote requests usually take a few seconds to complete, so waking up
> - * once per second to recheck the status is fine for this use case.
> + * Quote requests usually take a few milliseconds to complete, so waking up
> + * once per 5 milliseconds to recheck the status is fine for this use case.
> */
> - while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout) {
> - if (msleep_interruptible(MSEC_PER_SEC))
> + while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < 200 * timeout) {
> + if (msleep_interruptible(MSEC_PER_SEC / 200))
> return -EINTR;
> }
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply
* Re: [PATCH 1/1] firmware: smccc: add support for Live Firmware Activation (LFA)
From: Andre Przywara @ 2026-02-20 17:39 UTC (permalink / raw)
To: Salman Nabi
Cc: vvidwans, sudeep.holla, mark.rutland, lpieralisi, ardb, chao.gao,
linux-arm-kernel, linux-coco, linux-kernel, sdonthineni, vsethi,
vwadekar
In-Reply-To: <20260119122729.287522-2-salman.nabi@arm.com>
On Mon, 19 Jan 2026 12:27:29 +0000
Salman Nabi <salman.nabi@arm.com> wrote:
Hey,
for the records: while working on improving the patch and during internal
review, we found some bugs and issues in there. I will mark them below for
the benefit of others. I will send a v2 after -rc1, with those things
fixed and some improvements, and will include Vedashree's patches, so
that we have everything in one series.
> The Arm Live Firmware Activation (LFA) is a specification [1] to describe
> activating firmware components without a reboot. Those components
> (like TF-A's BL31, EDK-II, TF-RMM, secure paylods) would be updated the
> usual way: via fwupd, FF-A or other secure storage methods, or via some
> IMPDEF Out-Of-Bound method. The user can then activate this new firmware,
> at system runtime, without requiring a reboot.
> The specification covers the SMCCC interface to list and query available
> components and eventually trigger the activation.
>
> Add a new directory under /sys/firmware to present firmware components
> capable of live activation. Each of them is a directory under lfa/,
> and is identified via its GUID. The activation will be triggered by echoing
> "1" into the "activate" file:
> ==========================================
> /sys/firmware/lfa # ls -l . 6c*
> .:
> total 0
> drwxr-xr-x 2 0 0 0 Jan 19 11:33 47d4086d-4cfe-9846-9b95-2950cbbd5a00
> drwxr-xr-x 2 0 0 0 Jan 19 11:33 6c0762a6-12f2-4b56-92cb-ba8f633606d9
> drwxr-xr-x 2 0 0 0 Jan 19 11:33 d6d0eea7-fcea-d54b-9782-9934f234b6e4
>
> 6c0762a6-12f2-4b56-92cb-ba8f633606d9:
> total 0
> --w------- 1 0 0 4096 Jan 19 11:33 activate
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 activation_capable
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 activation_pending
> --w------- 1 0 0 4096 Jan 19 11:33 cancel
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 cpu_rendezvous
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 current_version
> -rw-r--r-- 1 0 0 4096 Jan 19 11:33 force_cpu_rendezvous
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 may_reset_cpu
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 name
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 pending_version
> /sys/firmware/lfa/6c0762a6-12f2-4b56-92cb-ba8f633606d9 # grep . *
> grep: activate: Permission denied
> activation_capable:1
> activation_pending:1
> grep: cancel: Permission denied
> cpu_rendezvous:1
> current_version:0.0
> force_cpu_rendezvous:1
> may_reset_cpu:0
> name:TF-RMM
> pending_version:0.0
> /sys/firmware/lfa/6c0762a6-12f2-4b56-92cb-ba8f633606d9 # echo 1 > activate
> [ 2825.797871] Arm LFA: firmware activation succeeded.
> /sys/firmware/lfa/6c0762a6-12f2-4b56-92cb-ba8f633606d9 #
> ==========================================
>
> [1] https://developer.arm.com/documentation/den0147/latest/
>
> Signed-off-by: Salman Nabi <salman.nabi@arm.com>
> ---
> drivers/firmware/smccc/Kconfig | 8 +
> drivers/firmware/smccc/Makefile | 1 +
> drivers/firmware/smccc/lfa_fw.c | 668 ++++++++++++++++++++++++++++++++
> 3 files changed, 677 insertions(+)
> create mode 100644 drivers/firmware/smccc/lfa_fw.c
>
> diff --git a/drivers/firmware/smccc/Kconfig b/drivers/firmware/smccc/Kconfig
> index 15e7466179a6..ff7ca49486b0 100644
> --- a/drivers/firmware/smccc/Kconfig
> +++ b/drivers/firmware/smccc/Kconfig
> @@ -23,3 +23,11 @@ config ARM_SMCCC_SOC_ID
> help
> Include support for the SoC bus on the ARM SMCCC firmware based
> platforms providing some sysfs information about the SoC variant.
> +
> +config ARM_LFA
> + tristate "Arm Live Firmware activation support"
> + depends on HAVE_ARM_SMCCC_DISCOVERY
As the kernel test robot correctly pointed out, this only works on ARM64.
SMCCC v1.2 is only defined for AArch64, and the LFA spec documents
actually explicitly mentions AArch64-only at the beginning.
> + default y
> + help
> + Include support for triggering Live Firmware Activation, which
> + allows to upgrade certain firmware components without a reboot.
> diff --git a/drivers/firmware/smccc/Makefile b/drivers/firmware/smccc/Makefile
> index 40d19144a860..a6dd01558a94 100644
> --- a/drivers/firmware/smccc/Makefile
> +++ b/drivers/firmware/smccc/Makefile
> @@ -2,3 +2,4 @@
> #
> obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smccc.o kvm_guest.o
> obj-$(CONFIG_ARM_SMCCC_SOC_ID) += soc_id.o
> +obj-$(CONFIG_ARM_LFA) += lfa_fw.o
> diff --git a/drivers/firmware/smccc/lfa_fw.c b/drivers/firmware/smccc/lfa_fw.c
> new file mode 100644
> index 000000000000..ce54049b7190
> --- /dev/null
> +++ b/drivers/firmware/smccc/lfa_fw.c
> @@ -0,0 +1,668 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2025 Arm Limited
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/kobject.h>
> +#include <linux/module.h>
> +#include <linux/stop_machine.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/arm-smccc.h>
> +#include <linux/psci.h>
> +#include <uapi/linux/psci.h>
> +#include <linux/uuid.h>
> +#include <linux/array_size.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
This misses <linux/workqueue.h>. We get it via other includes, but better
list it here explicitly.
> +
> +#undef pr_fmt
> +#define pr_fmt(fmt) "Arm LFA: " fmt
> +
> +/* LFA v1.0b0 specification */
> +#define LFA_1_0_FN_BASE 0xc40002e0
> +#define LFA_1_0_FN(n) (LFA_1_0_FN_BASE + (n))
> +
> +#define LFA_1_0_FN_GET_VERSION LFA_1_0_FN(0)
> +#define LFA_1_0_FN_CHECK_FEATURE LFA_1_0_FN(1)
> +#define LFA_1_0_FN_GET_INFO LFA_1_0_FN(2)
> +#define LFA_1_0_FN_GET_INVENTORY LFA_1_0_FN(3)
> +#define LFA_1_0_FN_PRIME LFA_1_0_FN(4)
> +#define LFA_1_0_FN_ACTIVATE LFA_1_0_FN(5)
> +#define LFA_1_0_FN_CANCEL LFA_1_0_FN(6)
> +
> +/* CALL_AGAIN flags (returned by SMC) */
> +#define LFA_PRIME_CALL_AGAIN BIT(0)
> +#define LFA_ACTIVATE_CALL_AGAIN BIT(0)
> +
> +/* LFA return values */
> +#define LFA_SUCCESS 0
> +#define LFA_NOT_SUPPORTED 1
> +#define LFA_BUSY 2
> +#define LFA_AUTH_ERROR 3
> +#define LFA_NO_MEMORY 4
> +#define LFA_CRITICAL_ERROR 5
> +#define LFA_DEVICE_ERROR 6
> +#define LFA_WRONG_STATE 7
> +#define LFA_INVALID_PARAMETERS 8
> +#define LFA_COMPONENT_WRONG_STATE 9
> +#define LFA_INVALID_ADDRESS 10
> +#define LFA_ACTIVATION_FAILED 11
> +
> +#define LFA_ERROR_STRING(name) \
> + [name] = #name
> +
> +static const char * const lfa_error_strings[] = {
> + LFA_ERROR_STRING(LFA_SUCCESS),
> + LFA_ERROR_STRING(LFA_NOT_SUPPORTED),
> + LFA_ERROR_STRING(LFA_BUSY),
> + LFA_ERROR_STRING(LFA_AUTH_ERROR),
> + LFA_ERROR_STRING(LFA_NO_MEMORY),
> + LFA_ERROR_STRING(LFA_CRITICAL_ERROR),
> + LFA_ERROR_STRING(LFA_DEVICE_ERROR),
> + LFA_ERROR_STRING(LFA_WRONG_STATE),
> + LFA_ERROR_STRING(LFA_INVALID_PARAMETERS),
> + LFA_ERROR_STRING(LFA_COMPONENT_WRONG_STATE),
> + LFA_ERROR_STRING(LFA_INVALID_ADDRESS),
> + LFA_ERROR_STRING(LFA_ACTIVATION_FAILED)
> +};
> +
> +enum image_attr_names {
> + LFA_ATTR_NAME,
> + LFA_ATTR_CURRENT_VERSION,
> + LFA_ATTR_PENDING_VERSION,
> + LFA_ATTR_ACT_CAPABLE,
> + LFA_ATTR_ACT_PENDING,
> + LFA_ATTR_MAY_RESET_CPU,
> + LFA_ATTR_CPU_RENDEZVOUS,
> + LFA_ATTR_FORCE_CPU_RENDEZVOUS,
> + LFA_ATTR_ACTIVATE,
> + LFA_ATTR_CANCEL,
> + LFA_ATTR_NR_IMAGES
> +};
> +
> +struct image_props {
> + struct list_head image_node;
> + const char *image_name;
> + int fw_seq_id;
> + u64 current_version;
> + u64 pending_version;
> + bool activation_capable;
> + bool activation_pending;
> + bool may_reset_cpu;
> + bool cpu_rendezvous;
> + bool cpu_rendezvous_forced;
> + struct kobject *image_dir;
> + struct kobj_attribute image_attrs[LFA_ATTR_NR_IMAGES];
> +};
> +static LIST_HEAD(lfa_fw_images);
> +
> +/* A UUID split over two 64-bit registers */
> +struct uuid_regs {
> + u64 uuid_lo;
> + u64 uuid_hi;
> +};
> +
> +static const struct fw_image_uuid {
> + const char *name;
> + const char *uuid;
> +} fw_images_uuids[] = {
> + {
> + .name = "TF-A BL31 runtime",
> + .uuid = "47d4086d-4cfe-9846-9b95-2950cbbd5a00",
> + },
> + {
> + .name = "BL33 non-secure payload",
> + .uuid = "d6d0eea7-fcea-d54b-9782-9934f234b6e4",
> + },
> + {
> + .name = "TF-RMM",
> + .uuid = "6c0762a6-12f2-4b56-92cb-ba8f633606d9",
> + },
> +};
> +
> +static struct kobject *lfa_dir;
> +static DEFINE_MUTEX(lfa_lock);
> +static struct workqueue_struct *fw_images_update_wq;
> +static struct work_struct fw_images_update_work;
> +
> +static int update_fw_images_tree(void);
> +
> +static void delete_fw_image_node(struct image_props *attrs)
> +{
> + int i;
> +
> + for (i = 0; i < LFA_ATTR_NR_IMAGES; i++)
> + sysfs_remove_file(attrs->image_dir, &attrs->image_attrs[i].attr);
> +
> + kobject_put(attrs->image_dir);
> + list_del(&attrs->image_node);
> + kfree(attrs);
> +}
> +
> +static void remove_invalid_fw_images(struct work_struct *work)
> +{
> + struct image_props *attrs, *tmp;
> +
> + mutex_lock(&lfa_lock);
> +
> + /*
> + * Remove firmware images including directories that are no longer
> + * present in the LFA agent after updating the existing ones.
> + */
> + list_for_each_entry_safe(attrs, tmp, &lfa_fw_images, image_node) {
> + if (attrs->fw_seq_id == -1)
> + delete_fw_image_node(attrs);
> + }
> +
> + mutex_unlock(&lfa_lock);
> +}
> +
> +static void set_image_flags(struct image_props *attrs, int seq_id,
> + u32 image_flags, u64 reg_current_ver,
> + u64 reg_pending_ver)
> +{
> + attrs->fw_seq_id = seq_id;
> + attrs->current_version = reg_current_ver;
> + attrs->pending_version = reg_pending_ver;
> + attrs->activation_capable = !!(image_flags & BIT(0));
> + attrs->activation_pending = !!(image_flags & BIT(1));
> + attrs->may_reset_cpu = !!(image_flags & BIT(2));
> + /* cpu_rendezvous_optional bit has inverse logic in the spec */
> + attrs->cpu_rendezvous = !(image_flags & BIT(3));
> +}
> +
> +static unsigned long get_nr_lfa_components(void)
> +{
> + struct arm_smccc_1_2_regs reg = { 0 };
> +
> + reg.a0 = LFA_1_0_FN_GET_INFO;
> + reg.a1 = 0; /* lfa_info_selector = 0 */
> +
> + arm_smccc_1_2_invoke(®, ®);
> + if (reg.a0 != LFA_SUCCESS)
> + return reg.a0;
> +
> + return reg.a1;
> +}
> +
> +static int lfa_cancel(void *data)
> +{
> + struct image_props *attrs = data;
> + struct arm_smccc_1_2_regs reg = { 0 };
> +
> + reg.a0 = LFA_1_0_FN_CANCEL;
> + reg.a1 = attrs->fw_seq_id;
> + arm_smccc_1_2_invoke(®, ®);
> +
> + /*
> + * When firmware activation is called with "skip_cpu_rendezvous=1",
> + * LFA_CANCEL can fail with LFA_BUSY if the activation could not be
> + * cancelled.
> + */
> + if (reg.a0 == LFA_SUCCESS) {
> + pr_info("Activation cancelled for image %s\n",
> + attrs->image_name);
> + } else {
> + pr_err("Firmware activation could not be cancelled: %s\n",
> + lfa_error_strings[-reg.a0]);
> + return -EINVAL;
> + }
> +
> + return reg.a0;
> +}
> +
> +static int call_lfa_activate(void *data)
> +{
> + struct image_props *attrs = data;
> + struct arm_smccc_1_2_regs reg = { 0 };
> +
> + reg.a0 = LFA_1_0_FN_ACTIVATE;
> + reg.a1 = attrs->fw_seq_id; /* fw_seq_id under consideration */
> + /*
> + * As we do not support updates requiring a CPU reset (yet),
> + * we pass 0 in reg.a3 and reg.a4, holding the entry point and context
> + * ID respectively.
> + * cpu_rendezvous_forced is set by the administrator, via sysfs,
> + * cpu_rendezvous is dictated by each firmware component.
> + */
> + reg.a2 = !(attrs->cpu_rendezvous_forced || attrs->cpu_rendezvous);
> +
> + for (;;) {
> + arm_smccc_1_2_invoke(®, ®);
> +
> + if ((long)reg.a0 < 0) {
> + pr_err("ACTIVATE for image %s failed: %s\n",
> + attrs->image_name, lfa_error_strings[-reg.a0]);
We should not let an externally provided value (the return value in a0) to
index an array. Future (or rogue?) versions of an LFA agent could use
higher error numbers, so the value must be checked before being used as an
index.
> + return reg.a0;
> + }
> + if (!(reg.a1 & LFA_ACTIVATE_CALL_AGAIN))
> + break; /* ACTIVATE successful */
> + }
> +
> + return reg.a0;
> +}
> +
> +static int activate_fw_image(struct image_props *attrs)
> +{
> + int ret;
> +
> + mutex_lock(&lfa_lock);
> + if (attrs->cpu_rendezvous_forced || attrs->cpu_rendezvous)
> + ret = stop_machine(call_lfa_activate, attrs, cpu_online_mask);
> + else
> + ret = call_lfa_activate(attrs);
> +
> + if (ret != 0) {
> + mutex_unlock(&lfa_lock);
> + return lfa_cancel(attrs);
> + }
> +
> + /*
> + * Invalidate fw_seq_ids (-1) for all images as the seq_ids and the
> + * number of firmware images in the LFA agent may change after a
> + * successful activation attempt. Negate all image flags as well.
> + */
> + attrs = NULL;
> + list_for_each_entry(attrs, &lfa_fw_images, image_node) {
> + set_image_flags(attrs, -1, 0b1000, 0, 0);
> + }
> +
> + update_fw_images_tree();
> +
> + /*
> + * Removing non-valid image directories at the end of an activation.
> + * We can't remove the sysfs attributes while in the respective
> + * _store() handler, so have to postpone the list removal to a
> + * workqueue.
> + */
> + INIT_WORK(&fw_images_update_work, remove_invalid_fw_images);
> + queue_work(fw_images_update_wq, &fw_images_update_work);
> + mutex_unlock(&lfa_lock);
> +
> + return ret;
> +}
> +
> +static int prime_fw_image(struct image_props *attrs)
> +{
> + struct arm_smccc_1_2_regs reg = { 0 };
> + int ret;
> +
> + mutex_lock(&lfa_lock);
> + /* Avoid SMC calls on invalid firmware images */
> + if (attrs->fw_seq_id == -1) {
> + pr_err("Arm LFA: Invalid firmware sequence id\n");
> + mutex_unlock(&lfa_lock);
> +
> + return -ENODEV;
> + }
> +
> + if (attrs->may_reset_cpu) {
> + pr_err("CPU reset not supported by kernel driver\n");
> + mutex_unlock(&lfa_lock);
> +
> + return -EINVAL;
> + }
> +
> + /*
> + * LFA_PRIME/ACTIVATE will return 1 in reg.a1 if the firmware
> + * priming/activation is still in progress. In that case
> + * LFA_PRIME/ACTIVATE will need to be called again.
> + * reg.a1 will become 0 once the prime/activate process completes.
> + */
> + reg.a0 = LFA_1_0_FN_PRIME;
> + reg.a1 = attrs->fw_seq_id; /* fw_seq_id under consideration */
> + for (;;) {
> + arm_smccc_1_2_invoke(®, ®);
> +
> + if ((long)reg.a0 < 0) {
> + pr_err("LFA_PRIME for image %s failed: %s\n",
> + attrs->image_name, lfa_error_strings[-reg.a0]);
> + mutex_unlock(&lfa_lock);
> +
> + return reg.a0;
> + }
> + if (!(reg.a1 & LFA_PRIME_CALL_AGAIN)) {
> + ret = 0;
> + break; /* PRIME successful */
> + }
> + }
> +
> + mutex_unlock(&lfa_lock);
> + return ret;
> +}
> +
> +static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_NAME]);
> +
> + return sysfs_emit(buf, "%s\n", attrs->image_name);
> +}
> +
> +static ssize_t activation_capable_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_ACT_CAPABLE]);
> +
> + return sysfs_emit(buf, "%d\n", attrs->activation_capable);
> +}
> +
> +static ssize_t activation_pending_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_ACT_PENDING]);
> + struct arm_smccc_1_2_regs reg = { 0 };
> +
> + /*
> + * Activation pending status can change anytime thus we need to update
> + * and return its current value
> + */
> + reg.a0 = LFA_1_0_FN_GET_INVENTORY;
> + reg.a1 = attrs->fw_seq_id;
> + arm_smccc_1_2_invoke(®, ®);
> + if (reg.a0 == LFA_SUCCESS)
> + attrs->activation_pending = !!(reg.a3 & BIT(1));
> +
> + return sysfs_emit(buf, "%d\n", attrs->activation_pending);
> +}
> +
> +static ssize_t may_reset_cpu_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_MAY_RESET_CPU]);
> +
> + return sysfs_emit(buf, "%d\n", attrs->may_reset_cpu);
> +}
> +
> +static ssize_t cpu_rendezvous_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_CPU_RENDEZVOUS]);
> +
> + return sysfs_emit(buf, "%d\n", attrs->cpu_rendezvous);
> +}
> +
> +static ssize_t force_cpu_rendezvous_store(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_FORCE_CPU_RENDEZVOUS]);
> + int ret;
> +
> + ret = kstrtobool(buf, &attrs->cpu_rendezvous_forced);
> + if (ret)
> + return ret;
> +
> + return count;
> +}
> +
> +static ssize_t force_cpu_rendezvous_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_FORCE_CPU_RENDEZVOUS]);
> +
> + return sysfs_emit(buf, "%d\n", attrs->cpu_rendezvous_forced);
> +}
> +
> +static ssize_t current_version_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_CURRENT_VERSION]);
> + u32 maj, min;
> +
> + maj = attrs->current_version >> 32;
> + min = attrs->current_version & 0xffffffff;
> + return sysfs_emit(buf, "%u.%u\n", maj, min);
> +}
> +
> +static ssize_t pending_version_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_ACT_PENDING]);
This should be LFA_ATTR_PENDING_VERSION as the index.
> + struct arm_smccc_1_2_regs reg = { 0 };
> + u32 maj, min;
> +
> + /*
> + * Similar to activation pending, this value can change following an
> + * update, we need to retrieve fresh info instead of stale information.
> + */
> + reg.a0 = LFA_1_0_FN_GET_INVENTORY;
> + reg.a1 = attrs->fw_seq_id;
> + arm_smccc_1_2_invoke(®, ®);
> + if (reg.a0 == LFA_SUCCESS) {
> + if (reg.a5 != 0 && attrs->activation_pending)
> + {
> + attrs->pending_version = reg.a5;
> + maj = reg.a5 >> 32;
> + min = reg.a5 & 0xffffffff;
> + }
> + }
This leaves maj and min uninitialised, if either the call failed or the
image is not pending or doesn't provide a new version.
Could be fixed by calling sysfs_emit() in the "if" clause, then returning,
and outputting some fallback version number otherwise.
> +
> + return sysfs_emit(buf, "%u.%u\n", maj, min);
> +}
> +
> +static ssize_t activate_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_ACTIVATE]);
> + int ret;
> +
> + ret = prime_fw_image(attrs);
> + if (ret) {
> + pr_err("Firmware prime failed: %s\n",
> + lfa_error_strings[-ret]);
> + return -ECANCELED;
> + }
> +
> + ret = activate_fw_image(attrs);
> + if (ret) {
> + pr_err("Firmware activation failed: %s\n",
> + lfa_error_strings[-ret]);
> + return -ECANCELED;
> + }
> +
> + pr_info("Firmware activation succeeded\n");
> +
> + return count;
> +}
> +
> +static ssize_t cancel_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_CANCEL]);
> + int ret;
> +
> + ret = lfa_cancel(attrs);
> + if (ret != 0)
> + return ret;
> +
> + return count;
> +}
> +
> +static struct kobj_attribute image_attrs_group[LFA_ATTR_NR_IMAGES] = {
> + [LFA_ATTR_NAME] = __ATTR_RO(name),
> + [LFA_ATTR_CURRENT_VERSION] = __ATTR_RO(current_version),
> + [LFA_ATTR_PENDING_VERSION] = __ATTR_RO(pending_version),
> + [LFA_ATTR_ACT_CAPABLE] = __ATTR_RO(activation_capable),
> + [LFA_ATTR_ACT_PENDING] = __ATTR_RO(activation_pending),
> + [LFA_ATTR_MAY_RESET_CPU] = __ATTR_RO(may_reset_cpu),
> + [LFA_ATTR_CPU_RENDEZVOUS] = __ATTR_RO(cpu_rendezvous),
> + [LFA_ATTR_FORCE_CPU_RENDEZVOUS] = __ATTR_RW(force_cpu_rendezvous),
> + [LFA_ATTR_ACTIVATE] = __ATTR_WO(activate),
> + [LFA_ATTR_CANCEL] = __ATTR_WO(cancel)
> +};
> +
> +static void clean_fw_images_tree(void)
> +{
> + struct image_props *attrs, *tmp;
> +
> + list_for_each_entry_safe(attrs, tmp, &lfa_fw_images, image_node)
> + delete_fw_image_node(attrs);
> +}
> +
> +static int update_fw_image_node(char *fw_uuid, int seq_id,
> + u32 image_flags, u64 reg_current_ver,
> + u64 reg_pending_ver)
> +{
> + const char *image_name = "(unknown)";
> + struct image_props *attrs;
> + int ret;
> +
> + /*
> + * If a fw_image is already in the images list then we just update
> + * its flags and seq_id instead of trying to recreate it.
> + */
> + list_for_each_entry(attrs, &lfa_fw_images, image_node) {
> + if (!strcmp(attrs->image_dir->name, fw_uuid)) {
> + set_image_flags(attrs, seq_id, image_flags,
> + reg_current_ver, reg_pending_ver);
> + return 0;
> + }
> + }
> +
> + attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
> + if (!attrs)
> + return -ENOMEM;
> +
> + for (int i = 0; i < ARRAY_SIZE(fw_images_uuids); i++) {
> + if (!strcmp(fw_images_uuids[i].uuid, fw_uuid))
> + image_name = fw_images_uuids[i].name;
> + }
> +
> + attrs->image_dir = kobject_create_and_add(fw_uuid, lfa_dir);
> + if (!attrs->image_dir)
> + return -ENOMEM;
that leaks attrs, allocated above
> +
> + INIT_LIST_HEAD(&attrs->image_node);
> + attrs->image_name = image_name;
> + attrs->cpu_rendezvous_forced = 1;
> + set_image_flags(attrs, seq_id, image_flags, reg_current_ver,
> + reg_pending_ver);
> +
> + /*
> + * The attributes for each sysfs file are constant (handler functions,
> + * name and permissions are the same within each directory), but we
> + * need a per-directory copy regardless, to get a unique handle
> + * for each directory, so that container_of can do its magic.
> + * Also this requires an explicit sysfs_attr_init(), since it's a new
> + * copy, to make LOCKDEP happy.
> + */
> + memcpy(attrs->image_attrs, image_attrs_group,
> + sizeof(attrs->image_attrs));
> + for (int i = 0; i < LFA_ATTR_NR_IMAGES; i++) {
> + struct attribute *attr = &attrs->image_attrs[i].attr;
> +
> + sysfs_attr_init(attr);
> + ret = sysfs_create_file(attrs->image_dir, attr);
> + if (ret) {
> + pr_err("creating sysfs file for uuid %s: %d\n",
> + fw_uuid, ret);
> + clean_fw_images_tree();
> +
> + return ret;
> + }
> + }
> + list_add(&attrs->image_node, &lfa_fw_images);
> +
> + return ret;
> +}
> +
> +static int update_fw_images_tree(void)
> +{
> + struct arm_smccc_1_2_regs reg = { 0 };
> + struct uuid_regs image_uuid;
> + char image_id_str[40];
> + int ret, num_of_components;
> +
> + num_of_components = get_nr_lfa_components();
> + if (num_of_components <= 0) {
> + pr_err("Error getting number of LFA components\n");
> + return -ENODEV;
> + }
> +
> + for (int i = 0; i < num_of_components; i++) {
> + reg.a0 = LFA_1_0_FN_GET_INVENTORY;
> + reg.a1 = i; /* fw_seq_id under consideration */
> + arm_smccc_1_2_invoke(®, ®);
> + if (reg.a0 == LFA_SUCCESS) {
> + image_uuid.uuid_lo = reg.a1;
> + image_uuid.uuid_hi = reg.a2;
> +
> + snprintf(image_id_str, sizeof(image_id_str), "%pUb",
> + &image_uuid);
> + ret = update_fw_image_node(image_id_str, i,
> + reg.a3, reg.a4, reg.a5);
> + if (ret)
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int __init lfa_init(void)
> +{
> + struct arm_smccc_1_2_regs reg = { 0 };
> + int err;
> +
> + reg.a0 = LFA_1_0_FN_GET_VERSION;
> + arm_smccc_1_2_invoke(®, ®);
> + if (reg.a0 == -LFA_NOT_SUPPORTED) {
> + pr_info("Live Firmware activation: no firmware agent found\n");
> + return -ENODEV;
> + }
> +
> + fw_images_update_wq = alloc_workqueue("fw_images_update_wq",
> + WQ_UNBOUND | WQ_MEM_RECLAIM, 1);
> + if (!fw_images_update_wq) {
> + pr_err("Live Firmware Activation: Failed to allocate workqueue.\n");
> +
> + return -ENOMEM;
> + }
> +
> + pr_info("Live Firmware Activation: detected v%ld.%ld\n",
> + reg.a0 >> 16, reg.a0 & 0xffff);
> +
> + lfa_dir = kobject_create_and_add("lfa", firmware_kobj);
> + if (!lfa_dir)
> + return -ENOMEM;
That leaks the workqueue created above.
> +
> + mutex_lock(&lfa_lock);
> + err = update_fw_images_tree();
> + if (err != 0)
> + kobject_put(lfa_dir);
... and also here.
Cheers,
Andre
> +
> + mutex_unlock(&lfa_lock);
> + return err;
> +}
> +module_init(lfa_init);
> +
> +static void __exit lfa_exit(void)
> +{
> + flush_workqueue(fw_images_update_wq);
> + destroy_workqueue(fw_images_update_wq);
> +
> + mutex_lock(&lfa_lock);
> + clean_fw_images_tree();
> + mutex_unlock(&lfa_lock);
> +
> + kobject_put(lfa_dir);
> +}
> +module_exit(lfa_exit);
> +
> +MODULE_DESCRIPTION("ARM Live Firmware Activation (LFA)");
> +MODULE_LICENSE("GPL");
^ permalink raw reply
* Re: [RFC PATCH v5 05/45] KVM: TDX: Drop kvm_x86_ops.link_external_spt(), use .set_external_spte() for all
From: Sean Christopherson @ 2026-02-20 17:36 UTC (permalink / raw)
To: Rick P Edgecombe
Cc: Yan Y Zhao, kvm@vger.kernel.org, linux-coco@lists.linux.dev,
Xiaoyao Li, Kai Huang, dave.hansen@linux.intel.com,
kas@kernel.org, binbin.wu@linux.intel.com, mingo@redhat.com,
pbonzini@redhat.com, ackerleytng@google.com,
linux-kernel@vger.kernel.org, Isaku Yamahata, sagis@google.com,
tglx@kernel.org, bp@alien8.de, Vishal Annapurve, x86@kernel.org
In-Reply-To: <75fc3f45e24309abef6dae31809012440de6d4ee.camel@intel.com>
On Wed, Feb 18, 2026, Rick P Edgecombe wrote:
> On Tue, 2026-02-03 at 20:05 +0000, Sean Christopherson wrote:
> > > And mirror_spte --> new_spte?
> >
> > Hmm, ya, I made that change later, but it can probably be shifted here.
>
> Sorry for the late comment on the tiny detail, but things seemed to have calmed
> down enough to attempt to merge these discussions into the snarl.
>
> It doesn't quite fit in this patch because the set_external_spte() op also uses
> the mirror_pte name. So then you need to either expand the scope of the patch to
> change "mirror" to "new" across the callchain, or creating a small mismatch
> between tdx_sept_set_private_spte() and tdx_sept_link_private_spt().
>
> The patch where it happens in this series needs to add the old_pte, forcing
> mirror_spte to grow some new nomenclature. So on balance I think it fits better
> there, and we should leave it alone here. We can update it in
> tdx_sept_link_private_spt() in "KVM: x86/mmu: Plumb the old_spte into
> kvm_x86_ops.set_external_spte()".
No argument from me.
^ permalink raw reply
* Re: [PATCH v4 05/24] x86/virt/seamldr: Retrieve P-SEAMLDR information
From: Huang, Kai @ 2026-02-20 9:36 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao, x86@kernel.org
Cc: Chen, Farrah, dave.hansen@linux.intel.com,
tony.lindgren@linux.intel.com, binbin.wu@linux.intel.com,
seanjc@google.com, kas@kernel.org, Chatre, Reinette,
Verma, Vishal L, nik.borisov@suse.com, mingo@redhat.com,
Weiny, Ira, hpa@zytor.com, Annapurve, Vishal, sagis@google.com,
Duan, Zhenzhong, Edgecombe, Rick P, paulmck@kernel.org,
tglx@kernel.org, yilun.xu@linux.intel.com, Williams, Dan J,
bp@alien8.de
In-Reply-To: <20260212143606.534586-6-chao.gao@intel.com>
> +int seamldr_get_info(struct seamldr_info *seamldr_info)
> +{
> + struct tdx_module_args args = { .rcx = slow_virt_to_phys(seamldr_info) };
Should we have a comment for slow_virt_to_phys()? This patch alone doesn't
really tell where is the memory from.
Btw, it it were me, I would just merge this patch with the next one. Then
it's clear the memory comes from tdx-host module's stack. The merged patch
won't be too big to review either (IMHO). You can then have this
seamldr_get_info() and its user together in one patch, with one changelog to
tell the full story.
But just my 2cents, feel free to ignore.
> +
> + return seamldr_call(P_SEAMLDR_INFO, &args);
> +}
> +EXPORT_SYMBOL_FOR_MODULES(seamldr_get_info, "tdx-host");
> --
> 2.47.3
^ permalink raw reply
* Re: [PATCH v4 04/24] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Huang, Kai @ 2026-02-20 1:12 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao, x86@kernel.org
Cc: Chen, Farrah, dave.hansen@linux.intel.com,
tony.lindgren@linux.intel.com, binbin.wu@linux.intel.com,
seanjc@google.com, kas@kernel.org, Chatre, Reinette,
Verma, Vishal L, nik.borisov@suse.com, mingo@redhat.com,
Weiny, Ira, hpa@zytor.com, Annapurve, Vishal, sagis@google.com,
Duan, Zhenzhong, Edgecombe, Rick P, paulmck@kernel.org,
tglx@kernel.org, yilun.xu@linux.intel.com, Williams, Dan J,
bp@alien8.de
In-Reply-To: <20260212143606.534586-5-chao.gao@intel.com>
On Thu, 2026-02-12 at 06:35 -0800, Chao Gao wrote:
> The TDX architecture uses the "SEAMCALL" instruction to communicate with
> SEAM mode software. Right now, the only SEAM mode software that the kernel
> communicates with is the TDX module. But, there is actually another
> component that runs in SEAM mode but it is separate from the TDX module:
> the persistent SEAM loader or "P-SEAMLDR". Right now, the only component
> that communicates with it is the BIOS which loads the TDX module itself at
> boot. But, to support updating the TDX module, the kernel now needs to be
> able to talk to it.
>
> P-SEAMLDR SEAMCALLs differ from TDX Module SEAMCALLs in areas such as
> concurrency requirements. Add a P-SEAMLDR wrapper to handle these
> differences and prepare for implementing concrete functions.
>
> Note that unlike P-SEAMLDR, there is also a non-persistent SEAM loader
> ("NP-SEAMLDR"). This is an authenticated code module (ACM) that is not
> callable at runtime. Only BIOS launches it to load P-SEAMLDR at boot;
[...]
> the kernel does not interact with it.
Nit:
Again, to me this only describes what does the kernel do today. It doesn't
describe what the kernel needs to do for runtime updating.
Maybe it can just be something like:
The kernel does not need to interact with it for runtime update.
But I don't know why do you even need to talk about NP-SEAMLDR.
>
> For details of P-SEAMLDR SEAMCALLs, see Intel® Trust Domain CPU
> Architectural Extensions, Revision 343754-002, Chapter 2.3 "INSTRUCTION
> SET REFERENCE".
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
> Link: https://cdrdv2.intel.com/v1/dl/getContent/733582 # [1]
>
[...]
> + * Serialize P-SEAMLDR calls since the hardware only allows a single CPU to
> + * interact with P-SEAMLDR simultaneously.
> + */
> +static DEFINE_RAW_SPINLOCK(seamldr_lock);
> +
> +static __maybe_unused int seamldr_call(u64 fn, struct tdx_module_args *args)
> +{
> + /*
> + * Serialize P-SEAMLDR calls and disable interrupts as the calls
> + * can be made from IRQ context.
> + */
> + guard(raw_spinlock_irqsave)(&seamldr_lock);
Why do you need to disable IRQ? A plain raw_spinlock should work with both
cases where seamldr_call() is called from IRQ disabled context and normal
task context?
> + return seamcall_prerr(fn, args);
> +}
^ permalink raw reply
* Re: [PATCH v4 03/24] coco/tdx-host: Expose TDX Module version
From: Huang, Kai @ 2026-02-20 0:40 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao, x86@kernel.org
Cc: dave.hansen@linux.intel.com, tony.lindgren@linux.intel.com,
binbin.wu@linux.intel.com, seanjc@google.com, kas@kernel.org,
Chatre, Reinette, Verma, Vishal L, nik.borisov@suse.com,
Weiny, Ira, Annapurve, Vishal, sagis@google.com, Duan, Zhenzhong,
Edgecombe, Rick P, paulmck@kernel.org, yilun.xu@linux.intel.com,
Williams, Dan J
In-Reply-To: <20260212143606.534586-4-chao.gao@intel.com>
On Thu, 2026-02-12 at 06:35 -0800, Chao Gao wrote:
> For TDX Module updates, userspace needs to select compatible update
> versions based on the current module version. This design delegates
> module selection complexity to userspace because TDX Module update
> policies are complex and version series are platform-specific.
>
> For example, the 1.5.x series is for certain platform generations, while
> the 2.0.x series is intended for others. And TDX Module 1.5.x may be
> updated to 1.5.y but not to 1.5.y+1.
>
> Expose the TDX Module version to userspace via sysfs to aid module
> selection. Since the TDX faux device will drive module updates, expose
> the version as its attribute.
>
> One bonus of exposing TDX Module version via sysfs is: TDX Module
> version information remains available even after dmesg logs are cleared.
>
> == Background ==
>
> The "faux device + device attribute" approach compares to other update
> mechanisms as follows:
This "faux device + device attribute" approach seems to be a wider design
choice instead of how to expose module version (which is the scope of this
patch). Overall, shouldn't this be in the changelog of the previous patch
which actually introduces "faux device" (albeit no attribute is introduced
in that patch)?
>
> 1. AMD SEV leverages an existing PCI device for the PSP to expose
> metadata. TDX uses a faux device as it doesn't have PCI device
> in its architecture.
E.g., this sounds to justify "why to use faux device for TDX", but not "to
expose module version via faux device attributes".
>
> 2. Microcode uses per-CPU virtual devices to report microcode revisions
> because CPUs can have different revisions. But, there is only a
> single TDX Module, so exposing the TDX Module version through a global
> TDX faux device is appropriate
This is related to exposing module version, but to me "there's only a single
TDX module" is also more like a justification to use "one faux device",
which should belong to changelog of previous patch too.
With "there's only a single TDX module" being said in previous patch
changelog, I think we can safely deduce that there's only "one module
version" but not per-cpu (thus I don't think we even need to call this out
in _this_ patch).
>
> 3. ARM's CCA implementation isn't in-tree yet, but will likely follow a
> similar faux device approach [1], though it's unclear whether they need
> to expose firmware version information
Again, I don't feel "follow a similar faux device approach" for ARM CCA
should be a justification of "exposing module version via faux attributes".
It should be a justification of "using faux device for TDX".
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
> Link: https://lore.kernel.org/all/2025073035-bulginess-rematch-b92e@gregkh/ # [1]
>
[...]
> +Description: (RO) Report the version of the loaded TDX Module. The TDX Module
> + version is formatted as x.y.z, where "x" is the major version,
> + "y" is the minor version and "z" is the update version. Versions
> + are used for bug reporting, TDX Module updates and etc.
^
Nit: No need to use "and" before "etc".
Consulting google:
No, it is not correct to say or write "and etc." Because etc. is an
abbreviation for the Latin phrase et cetera, which translates to "and
other things" or "and the rest," including "and" makes the phrase
redundant. Using "and etc." is equivalent to saying "and and the rest".
> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> index c77885392b09..0424933b2560 100644
> --- a/drivers/virt/coco/tdx-host/tdx-host.c
> +++ b/drivers/virt/coco/tdx-host/tdx-host.c
>
[...]
The actual code LGTM.
^ permalink raw reply
* Re: [PATCH v4 02/24] coco/tdx-host: Introduce a "tdx_host" device
From: Huang, Kai @ 2026-02-20 0:15 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao, x86@kernel.org
Cc: dave.hansen@linux.intel.com, tony.lindgren@linux.intel.com,
binbin.wu@linux.intel.com, seanjc@google.com, kas@kernel.org,
Chatre, Reinette, Verma, Vishal L, nik.borisov@suse.com,
mingo@redhat.com, Weiny, Ira, jonathan.cameron@huawei.com,
Annapurve, Vishal, sagis@google.com, Duan, Zhenzhong,
Edgecombe, Rick P, paulmck@kernel.org, hpa@zytor.com,
tglx@kernel.org, yilun.xu@linux.intel.com, Williams, Dan J,
bp@alien8.de
In-Reply-To: <20260212143606.534586-3-chao.gao@intel.com>
>
> A faux device is used as for TDX because the TDX module is singular within
^
"as" should be removed.
> the system and lacks associated platform resources. Using a faux device
> eliminates the need to create a stub bus.
>
> The call to tdx_get_sysinfo() ensures that the TDX Module is ready to
> provide services.
>
> Note that AMD has a PCI device for the PSP for SEV and ARM CCA will
> likely have a faux device [1].
>
> Co-developed-by: Xu Yilun <yilun.xu@linux.intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
> Link: https://lore.kernel.org/all/2025073035-bulginess-rematch-b92e@gregkh/ # [1]
Reviewed-by: Kai Huang <kai.huang@intel.com>
A nit below ..
[...]
> +config TDX_HOST_SERVICES
> + tristate "TDX Host Services Driver"
> + depends on INTEL_TDX_HOST
> + default m
> + help
> + Enable access to TDX host services like module update and
> + extensions (e.g. TDX Connect).
> +
> + Say y or m if enabling support for confidential virtual machine
> + support (CONFIG_INTEL_TDX_HOST). The module is called tdx_host.ko
.. Missing period at the end of the last sentence.
^ permalink raw reply
* Re: [PATCH v4 10/24] x86/virt/seamldr: Allocate and populate a module update request
From: Huang, Kai @ 2026-02-19 22:31 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao, x86@kernel.org
Cc: dave.hansen@linux.intel.com, tony.lindgren@linux.intel.com,
binbin.wu@linux.intel.com, seanjc@google.com, kas@kernel.org,
Chatre, Reinette, Verma, Vishal L, nik.borisov@suse.com,
mingo@redhat.com, Weiny, Ira, hpa@zytor.com, Annapurve, Vishal,
sagis@google.com, Duan, Zhenzhong, Edgecombe, Rick P,
paulmck@kernel.org, tglx@kernel.org, yilun.xu@linux.intel.com,
Williams, Dan J, bp@alien8.de
In-Reply-To: <20260212143606.534586-11-chao.gao@intel.com>
On Thu, 2026-02-12 at 06:35 -0800, Chao Gao wrote:
> P-SEAMLDR uses the SEAMLDR_PARAMS structure to describe TDX Module
> update requests. This structure contains physical addresses pointing to
> the module binary and its signature file (or sigstruct), along with an
> update scenario field.
>
> TDX Modules are distributed in the tdx_blob format defined at [1]. A
> tdx_blob contains a header, sigstruct, and module binary. This is also
> the format supplied by the userspace to the kernel.
>
> Parse the tdx_blob format and populate a SEAMLDR_PARAMS structure
> accordingly. This structure will be passed to P-SEAMLDR to initiate the
> update.
>
> Note that the sigstruct_pa field in SEAMLDR_PARAMS has been extended to
> a 4-element array. The updated "SEAM Loader (SEAMLDR) Interface
> Specification" will be published separately. The kernel does not
> validate P-SEAMLDR compatibility (for example, whether it supports 4KB
> or 16KB sigstruct);
>
Nit:
This sounds like the kernel can validate but chooses not to. But I thought
the fact is the kernel cannot validate because there's no P-SEAMLDR ABI to
enumerate such compatibility?
> userspace must ensure the P-SEAMLDR version is
> compatible with the selected TDX Module by checking the minimum
> P-SEAMLDR version requirements at [2].
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Link: https://github.com/intel/confidential-computing.tdx.tdx-module.binaries/blob/main/blob_structure.txt # [1]
> Link: https://github.com/intel/confidential-computing.tdx.tdx-module.binaries/blob/main/mapping_file.json # [2]
>
Nit:
As mentioned in v3, can the link be considered as "stable", e.g., won't
disappear couple of years later?
Not sure we should just have a documentation patch for 'tdx_blob' layout. I
suspect the content won't be changed in the future anyway, at least for
foreseeable future, given you have already updated the sigstruct part.
We can include the links to the actual doc too, and if necessarily, point
out the links may get updated in the future. We can actually update the
links if they are in some doc.
[...]
> +/*
> + * Intel TDX Module blob. Its format is defined at:
> + * https://github.com/intel/tdx-module-binaries/blob/main/blob_structure.txt
> + *
> + * Note this structure differs from the reference above: the two variable-length
> + * fields "@sigstruct" and "@module" are represented as a single "@data" field
> + * here and split programmatically using the offset_of_module value.
> + */
> +struct tdx_blob {
> + u16 version;
> + u16 checksum;
> + u32 offset_of_module;
> + u8 signature[8];
> + u32 length;
> + u32 resv0;
> + u64 resv1[509];
> + u8 data[];
> +} __packed;
Nit:
It appeared you said you will s/resv/rsvd in v3.
I don't quite mind if other people are fine with 'resv'. Or you can spell
out 'reserved' in full to match the one in 'struct seamldr_params' above.
Up to you.
The rest LGTM.
^ permalink raw reply
* Re: [PATCH v3 00/16] x86/msr: Inline rdmsr/wrmsr instructions
From: Jürgen Groß @ 2026-02-19 6:28 UTC (permalink / raw)
To: H. Peter Anvin, linux-kernel, x86, linux-coco, kvm, linux-hyperv,
virtualization, llvm
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
Kiryl Shutsemau, Rick Edgecombe, Sean Christopherson,
Paolo Bonzini, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, Long Li, Vitaly Kuznetsov, Boris Ostrovsky, xen-devel,
Ajay Kaher, Alexey Makhalov, Broadcom internal kernel review list,
Andy Lutomirski, Peter Zijlstra, Xin Li, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Josh Poimboeuf,
andy.cooper
In-Reply-To: <3D1FE2A7-F237-4232-9E39-6AFC75F3A4F0@zytor.com>
[-- Attachment #1.1.1: Type: text/plain, Size: 6721 bytes --]
On 18.02.26 21:37, H. Peter Anvin wrote:
> On February 18, 2026 12:21:17 AM PST, Juergen Gross <jgross@suse.com> wrote:
>> When building a kernel with CONFIG_PARAVIRT_XXL the paravirt
>> infrastructure will always use functions for reading or writing MSRs,
>> even when running on bare metal.
>>
>> Switch to inline RDMSR/WRMSR instructions in this case, reducing the
>> paravirt overhead.
>>
>> The first patch is a prerequisite fix for alternative patching. Its
>> is needed due to the initial indirect call needs to be padded with
>> NOPs in some cases with the following patches.
>>
>> In order to make this less intrusive, some further reorganization of
>> the MSR access helpers is done in the patches 1-6.
>>
>> The next 4 patches are converting the non-paravirt case to use direct
>> inlining of the MSR access instructions, including the WRMSRNS
>> instruction and the immediate variants of RDMSR and WRMSR if possible.
>>
>> Patches 11-13 are some further preparations for making the real switch
>> to directly patch in the native MSR instructions easier.
>>
>> Patch 14 is switching the paravirt MSR function interface from normal
>> call ABI to one more similar to the native MSR instructions.
>>
>> Patch 15 is a little cleanup patch.
>>
>> Patch 16 is the final step for patching in the native MSR instructions
>> when not running as a Xen PV guest.
>>
>> This series has been tested to work with Xen PV and on bare metal.
>>
>> Note that there is more room for improvement. This series is sent out
>> to get a first impression how the code will basically look like.
>
> Does that mean you are considering this patchset an RFC? If so, you should put that in the subject header.
It is one possible solution.
>
>> Right now the same problem is solved differently for the paravirt and
>> the non-paravirt cases. In case this is not desired, there are two
>> possibilities to merge the two implementations. Both solutions have
>> the common idea to have rather similar code for paravirt and
>> non-paravirt variants, but just use a different main macro for
>> generating the respective code. For making the code of both possible
>> scenarios more similar, the following variants are possible:
>>
>> 1. Remove the micro-optimizations of the non-paravirt case, making
>> it similar to the paravirt code in my series. This has the
>> advantage of being more simple, but might have a very small
>> negative performance impact (probably not really detectable).
>>
>> 2. Add the same micro-optimizations to the paravirt case, requiring
>> to enhance paravirt patching to support a to be patched indirect
>> call in the middle of the initial code snipplet.
>>
>> In both cases the native MSR function variants would no longer be
>> usable in the paravirt case, but this would mostly affect Xen, as it
>> would need to open code the WRMSR/RDMSR instructions to be used
>> instead the native_*msr*() functions.
>>
>> Changes since V2:
>> - switch back to the paravirt approach
>>
>> Changes since V1:
>> - Use Xin Li's approach for inlining
>> - Several new patches
>>
>> Juergen Gross (16):
>> x86/alternative: Support alt_replace_call() with instructions after
>> call
>> coco/tdx: Rename MSR access helpers
>> x86/sev: Replace call of native_wrmsr() with native_wrmsrq()
>> KVM: x86: Remove the KVM private read_msr() function
>> x86/msr: Minimize usage of native_*() msr access functions
>> x86/msr: Move MSR trace calls one function level up
>> x86/opcode: Add immediate form MSR instructions
>> x86/extable: Add support for immediate form MSR instructions
>> x86/msr: Use the alternatives mechanism for WRMSR
>> x86/msr: Use the alternatives mechanism for RDMSR
>> x86/alternatives: Add ALTERNATIVE_4()
>> x86/paravirt: Split off MSR related hooks into new header
>> x86/paravirt: Prepare support of MSR instruction interfaces
>> x86/paravirt: Switch MSR access pv_ops functions to instruction
>> interfaces
>> x86/msr: Reduce number of low level MSR access helpers
>> x86/paravirt: Use alternatives for MSR access with paravirt
>>
>> arch/x86/coco/sev/internal.h | 7 +-
>> arch/x86/coco/tdx/tdx.c | 8 +-
>> arch/x86/hyperv/ivm.c | 2 +-
>> arch/x86/include/asm/alternative.h | 6 +
>> arch/x86/include/asm/fred.h | 2 +-
>> arch/x86/include/asm/kvm_host.h | 10 -
>> arch/x86/include/asm/msr.h | 345 ++++++++++++++++------
>> arch/x86/include/asm/paravirt-msr.h | 148 ++++++++++
>> arch/x86/include/asm/paravirt.h | 67 -----
>> arch/x86/include/asm/paravirt_types.h | 57 ++--
>> arch/x86/include/asm/qspinlock_paravirt.h | 4 +-
>> arch/x86/kernel/alternative.c | 5 +-
>> arch/x86/kernel/cpu/mshyperv.c | 7 +-
>> arch/x86/kernel/kvmclock.c | 2 +-
>> arch/x86/kernel/paravirt.c | 42 ++-
>> arch/x86/kvm/svm/svm.c | 16 +-
>> arch/x86/kvm/vmx/tdx.c | 2 +-
>> arch/x86/kvm/vmx/vmx.c | 8 +-
>> arch/x86/lib/x86-opcode-map.txt | 5 +-
>> arch/x86/mm/extable.c | 35 ++-
>> arch/x86/xen/enlighten_pv.c | 52 +++-
>> arch/x86/xen/pmu.c | 4 +-
>> tools/arch/x86/lib/x86-opcode-map.txt | 5 +-
>> tools/objtool/check.c | 1 +
>> 24 files changed, 576 insertions(+), 264 deletions(-)
>> create mode 100644 arch/x86/include/asm/paravirt-msr.h
>>
>
> Could you clarify *on the high design level* what "go back to the paravirt approach" means, and the motivation for that?
This is related to V2 of this series, where I used a static branch for
special casing Xen PV.
Peter Zijlstra commented on that asking to try harder using the pv_ops
hooks for Xen PV, too.
> Note that for Xen *most* MSRs fall in one of two categories: those that are dropped entirely and those that are just passed straight on to the hardware.
>
> I don't know if anyone cares about optimizing PV Xen anymore, but at least in theory Xen can un-paravirtualize most sites.
The problem with that is, that this would need to be taken care at the
callers' sites, "poisoning" a lot of code with Xen specific paths. Or we'd
need to use the native variants explicitly at all places where Xen PV
would just use the MSR instructions itself. But please be aware, that
there are plans to introduce a hypercall for Xen to speed up MSR accesses,
which would reduce the "passed through to hardware" cases to 0.
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply
* Re: [PATCH 2/6] x86/sev: add support for enabling RMPOPT
From: Dave Hansen @ 2026-02-18 22:56 UTC (permalink / raw)
To: Kalra, Ashish, tglx, mingo, bp, dave.hansen, x86, hpa, seanjc,
peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <bab6afd4-b197-4a7b-b9d1-f518d192524f@amd.com>
On 2/18/26 14:17, Kalra, Ashish wrote:
> Yes, by default Venice platform has the NPS2 configuration enabled by default,
> so we have 'X' nodes per socket and we have to consider this NPSx configuration
> and optimize for those groups.
Why, though?
You keep saying: "We have NPS so we must configure sockets". But not *why*.
I suspect this is another premature optimization. Nodes are a bit too
small so if you configure via nodes, the later nodes will have RMPOPT
tables that cover empty address space off the end of system memory.
Honestly, I think this is all just done wrong. It doesn't need to even
consider sockets. Sockets might even be the wrong thing to look at.
Basically, RMPOPT gives you a 2TB window of potentially "fast" memory.
The rest of memory is "slow". If you're lucky, the memory that's fast
because of RMPOPT is also in a low-distance NUMA node.
Sockets are a good thing to use, for sure. But they're not even optimal!
Just imagine what's going to happen if you have more than 2TB in a
socket. You just turn off the per-socket optimization. If that happens,
the last node in the socket will end up with an RMPOPT table that has
itself at the beginning, but probably a nonzero amount of off-socket memory.
I'd probably just do something like this:
Given a NUMA node, go through each 1GB of memory in the system and see
what the average NUMA distance of that 2TB window of memory is. Find the
2TB window with the lowest average distance. That'll give you a more or
less optimal RMPOPT window. It'll work with NPS or regular NUMA or
whatever bonkers future fancy thing shows up.
But that's all optimization territory. Please squirrel that away to go
look at in 6 months once you get the rest of this merged.
^ permalink raw reply
* [Invitation] bi-weekly guest_memfd upstream call on 2026-02-19
From: David Hildenbrand (Arm) @ 2026-02-18 22:46 UTC (permalink / raw)
To: linux-coco@lists.linux.dev, linux-mm@kvack.org, KVM
Hi,
Our next guest_memfd upstream call is scheduled for Thursday,
2026-02-19 at 8:00 - 9:00am (GMT-08:00) Pacific Time - Vancouver.
We'll be using the following Google meet:
http://meet.google.com/wxp-wtju-jzw
The meeting notes can be found at [1], where we also link recordings and
collect current guest_memfd upstream proposals. If you want an google
calendar invitation that also covers all future meetings, just write me
or Ackerley a mail.
In this meeting, we'll discuss how to best handle private MMIO, and I'm
sure some other topics will pop up :)
To put something to discuss onto the agenda, reply to this mail or add
them to the "Topics/questions for next meeting(s)" section in the
meeting notes as a comment.
[1]
https://docs.google.com/document/d/1M6766BzdY1Lhk7LiR5IqVR8B8mG3cr-cxTxOrAosPOk/edit?usp=sharing
[2] https://lore.kernel.org/all/20251203144159.6131-1-itazur@amazon.com/
--
Cheers,
David
^ permalink raw reply
* Re: [PATCH 2/6] x86/sev: add support for enabling RMPOPT
From: Kalra, Ashish @ 2026-02-18 22:17 UTC (permalink / raw)
To: Dave Hansen, tglx, mingo, bp, dave.hansen, x86, hpa, seanjc,
peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <10baddd3-add6-4771-a1ce-f759d3ec69d2@intel.com>
Hello Dave,
On 2/17/2026 4:06 PM, Dave Hansen wrote:
>> +#define RMPOPT_TABLE_MAX_LIMIT_IN_TB 2
>> +#define NUM_TB(pfn_min, pfn_max) \
>> + (((pfn_max) - (pfn_min)) / (1 << (40 - PAGE_SHIFT)))
>
> IMNHO, you should just keep these in bytes. No reason to keep them in TB.
>
>> +struct rmpopt_socket_config {
>> + unsigned long start_pfn, end_pfn;
>> + cpumask_var_t cpulist;
>> + int *node_id;
>> + int current_node_idx;
>> +};
>
> This looks like optimization complexity before the groundwork is in
> place. Also, don't we *have* CPU lists for NUMA nodes? This seems rather
> redundant.
>
Yes, we do have CPU lists for NUMA nodes, but we need a socket specific
cpumask, let me explain more about that below.
>> +/*
>> + * Build a cpumask of online primary threads, accounting for primary threads
>> + * that have been offlined while their secondary threads are still online.
>> + */
>> +static void get_cpumask_of_primary_threads(cpumask_var_t cpulist)
>> +{
>> + cpumask_t cpus;
>> + int cpu;
>> +
>> + cpumask_copy(&cpus, cpu_online_mask);
>> + for_each_cpu(cpu, &cpus) {
>> + cpumask_set_cpu(cpu, cpulist);
>> + cpumask_andnot(&cpus, &cpus, cpu_smt_mask(cpu));
>> + }
>> +}
>
> Don't we have a primary thread mask already? I thought we did.
>
Already discussed this.
>> +static void __configure_rmpopt(void *val)
>> +{
>> + u64 rmpopt_base = ((u64)val & PUD_MASK) | MSR_AMD64_RMPOPT_ENABLE;
>> +
>> + wrmsrq(MSR_AMD64_RMPOPT_BASE, rmpopt_base);
>> +}
>
> I'd honestly just make the callers align the address..
>
>> +static void configure_rmpopt_non_numa(cpumask_var_t primary_threads_cpulist)
>> +{
>> + on_each_cpu_mask(primary_threads_cpulist, __configure_rmpopt, (void *)0, true);
>> +}
>> +
>> +static void free_rmpopt_socket_config(struct rmpopt_socket_config *socket)
>> +{
>> + int i;
>> +
>> + if (!socket)
>> + return;
>> +
>> + for (i = 0; i < topology_max_packages(); i++) {
>> + free_cpumask_var(socket[i].cpulist);
>> + kfree(socket[i].node_id);
>> + }
>> +
>> + kfree(socket);
>> +}
>> +DEFINE_FREE(free_rmpopt_socket_config, struct rmpopt_socket_config *, free_rmpopt_socket_config(_T))
>
> Looking at all this, I really think you need a more organized series.
>
> Make something that's _functional_ and works for all <2TB configs. Then,
> go add all this NUMA complexity in a follow-on patch or patches. There's
> too much going on here.
Sure.
>
>> +static void configure_rmpopt_large_physmem(cpumask_var_t primary_threads_cpulist)
>> +{
>> + struct rmpopt_socket_config *socket __free(free_rmpopt_socket_config) = NULL;
>> + int max_packages = topology_max_packages();
>> + struct rmpopt_socket_config *sc;
>> + int cpu, i;
>> +
>> + socket = kcalloc(max_packages, sizeof(struct rmpopt_socket_config), GFP_KERNEL);
>> + if (!socket)
>> + return;
>> +
>> + for (i = 0; i < max_packages; i++) {
>> + sc = &socket[i];
>> + if (!zalloc_cpumask_var(&sc->cpulist, GFP_KERNEL))
>> + return;
>> + sc->node_id = kcalloc(nr_node_ids, sizeof(int), GFP_KERNEL);
>> + if (!sc->node_id)
>> + return;
>> + sc->current_node_idx = -1;
>> + }
>> +
>> + /*
>> + * Handle case of virtualized NUMA software domains, such as AMD Nodes Per Socket(NPS)
>> + * configurations. The kernel does not have an abstraction for physical sockets,
>> + * therefore, enumerate the physical sockets and Nodes Per Socket(NPS) information by
>> + * walking the online CPU list.
>> + */
>
> By this point, I've forgotten why sockets are important here.
>
> Why are they important?
Because, Nodes per Socket (NPS) configuration is enabled by default, therefore, we have to
look at Sockets instead of simply NUMA nodes, and collect/aggregate all the Node data per Socket
and then accordingly setup the RMPOPT tables, so that the 2TB limit of RMPOPT tables is covered
appropriately and we try to map the maximum possible memory in RMPOPT tables per-Socket rather
than per-Node.
And as there is no per-Socket information available in kernel, we walk through the online
CPU list and collect all this per-Socket information (including socket's start, end addresses,
NUMA nodes in the socket, cpumask of the socket, etc.)
>
>> + for_each_cpu(cpu, primary_threads_cpulist) {
>> + int socket_id, nid;
>> +
>> + socket_id = topology_logical_package_id(cpu);
>> + nid = cpu_to_node(cpu);
>> + sc = &socket[socket_id];
>> +
>> + /*
>> + * For each socket, determine the corresponding nodes and the socket's start
>> + * and end PFNs.
>> + * Record the node and the start and end PFNs of the first node found on the
>> + * socket, then record each subsequent node and update the end PFN for that
>> + * socket as additional nodes are found.
>> + */
>> + if (sc->current_node_idx == -1) {
>> + sc->current_node_idx = 0;
>> + sc->node_id[sc->current_node_idx] = nid;
>> + sc->start_pfn = node_start_pfn(nid);
>> + sc->end_pfn = node_end_pfn(nid);
>> + } else if (sc->node_id[sc->current_node_idx] != nid) {
>> + sc->current_node_idx++;
>> + sc->node_id[sc->current_node_idx] = nid;
>> + sc->end_pfn = node_end_pfn(nid);
>> + }
>> +
>> + cpumask_set_cpu(cpu, sc->cpulist);
>> + }
>> +
>> + /*
>> + * If the "physical" socket has up to 2TB of memory, the per-CPU RMPOPT tables are
>> + * configured to the starting physical address of the socket, otherwise the tables
>> + * are configured per-node.
>> + */
>> + for (i = 0; i < max_packages; i++) {
>> + int num_tb_socket;
>> + phys_addr_t pa;
>> + int j;
>> +
>> + sc = &socket[i];
>> + num_tb_socket = NUM_TB(sc->start_pfn, sc->end_pfn) + 1;
>> +
>> + pr_debug("socket start_pfn 0x%lx, end_pfn 0x%lx, socket cpu mask %*pbl\n",
>> + sc->start_pfn, sc->end_pfn, cpumask_pr_args(sc->cpulist));
>> +
>> + if (num_tb_socket <= RMPOPT_TABLE_MAX_LIMIT_IN_TB) {
>> + pa = PFN_PHYS(sc->start_pfn);
>> + on_each_cpu_mask(sc->cpulist, __configure_rmpopt, (void *)pa, true);
>> + continue;
>> + }
>> +
>> + for (j = 0; j <= sc->current_node_idx; j++) {
>> + int nid = sc->node_id[j];
>> + struct cpumask node_mask;
>> +
>> + cpumask_and(&node_mask, cpumask_of_node(nid), sc->cpulist);
>> + pa = PFN_PHYS(node_start_pfn(nid));
>> +
>> + pr_debug("RMPOPT_BASE MSR on nodeid %d cpu mask %*pbl set to 0x%llx\n",
>> + nid, cpumask_pr_args(&node_mask), pa);
>> + on_each_cpu_mask(&node_mask, __configure_rmpopt, (void *)pa, true);
>> + }
>> + }
>> +}
>
> Ahh, so you're not optimizing by NUMA itself: you're assuming that there
> are groups of NUMA nodes in a socket and then optimizing for those groups.
>
Yes, by default Venice platform has the NPS2 configuration enabled by default,
so we have 'X' nodes per socket and we have to consider this NPSx configuration
and optimize for those groups.
> It would have been nice to say that. It would make great material for
> the changelog for your broken out patches.
Ok.
>
> I have the feeling that the structure here could be one of these in a patch:
>
> 1. Support systems with <2TB of memory
> 2. Support a RMPOPT range per NUMA node
> 3. Group NUMA nodes at socket boundaries and have them share a common
> RMPOPT config.
>
> Right?
Yes, sure.
>
>> +static __init void configure_and_enable_rmpopt(void)
>> +{
>> + cpumask_var_t primary_threads_cpulist;
>> + int num_tb;
>> +
>> + if (!cpu_feature_enabled(X86_FEATURE_RMPOPT)) {
>> + pr_debug("RMPOPT not supported on this platform\n");
>> + return;
>> + }
>> +
>> + if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) {
>> + pr_debug("RMPOPT optimizations not enabled as SNP support is not enabled\n");
>> + return;
>> + }
>> +
>> + if (!(rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED)) {
>> + pr_info("RMPOPT optimizations not enabled, segmented RMP required\n");
>> + return;
>> + }
>> +
>> + if (!zalloc_cpumask_var(&primary_threads_cpulist, GFP_KERNEL))
>> + return;
>> +
>> + num_tb = NUM_TB(min_low_pfn, max_pfn) + 1;
>> + pr_debug("NUM_TB pages in system %d\n", num_tb);
>
> This looks wrong. Earlier, you program 0 as the base RMPOPT address into
> the MSR. But this uses 'min_low_pfn'. Why not 0?
You are right, we should have used min_low_pfn earlier to program the
base RMPOPT address into the MSR.
>
>> + /* Only one thread per core needs to set RMPOPT_BASE MSR as it is per-core */
>> + get_cpumask_of_primary_threads(primary_threads_cpulist);
>> +
>> + /*
>> + * Per-CPU RMPOPT tables support at most 2 TB of addressable memory for RMP optimizations.
>> + *
>> + * Fastpath RMPOPT configuration and setup:
>> + * For systems with <= 2 TB of RAM, configure each per-core RMPOPT base to 0,
>> + * ensuring all system RAM is RMP-optimized on all CPUs.
>> + */
>> + if (num_tb <= RMPOPT_TABLE_MAX_LIMIT_IN_TB)
>> + configure_rmpopt_non_numa(primary_threads_cpulist);
>
> this part:
>
>> + else
>> + configure_rmpopt_large_physmem(primary_threads_cpulist);
>
> ^^ needs to be broken out into a separate optimization patch.
>
Ok.
Thanks,
Ashish
^ permalink raw reply
* Re: [PATCH 0/6] Add RMPOPT support.
From: Kalra, Ashish @ 2026-02-18 21:09 UTC (permalink / raw)
To: Dave Hansen, tglx, mingo, bp, dave.hansen, x86, hpa, seanjc,
peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <2398df04-082d-4d98-beca-f85de385941a@intel.com>
Hello Dave,
On 2/18/2026 11:15 AM, Dave Hansen wrote:
> On 2/18/26 09:03, Kalra, Ashish wrote:
>>> They are known not to contain any SEV-SNP guest memory at the
>>> moment snp_rmptable_init() finishes, no?
>> Yes, but RMP checks are still performed and they affect performance.
>>
>> Testing a bit in the per‑CPU RMPOPT table to avoid RMP checks
>> significantly improves performance.
>
> Sorry, Ashish, I don't think I'm explaining myself very well. Let me try
> again, please.
>
> First, my goal here is to ensure that the system has a whole has good
> performance, with minimal kernel code, and in the most common
> configurations.
>
> I would wager that the most common SEV-SNP configuration in the whole
> world is a system that has booted, enabled SEV-SNP, and has never run an
> SEV-SNP guest. If it's not *the* most common, it's certainly going to be
> common enough to care about deeply.
>
> Do you agree?
Yes.
>
> If you agree, I hope we can also agree that a "SNP enabled but never ran
> a guest" state is deserving of good performance with minimal kernel code.
>
> My assumption (which is maybe a bad one) is that there is a natural
> point when SEV-SNP is enabled on the system when the system as a whole
> can easily assert that no SEV-SNP guest has ever run. I'm assuming that
> there is *a* point where, for instance, the RMP table gets atomically
> flipped from being unprotected to being protected. At that point, its
> state *must* be known. It must also be naturally obvious that no guest
> has had a chance to run at this point.
>
> If that point can be leveraged, and the RMPOPT optimization can be
> applied at SEV-SNP enabled time, then an important SEV-SNP configuration
> would be optimized by default and with zero or little kernel code needed
> to drive it.
>
> To me, that seems like a valuable goal.
>
> Do you agree?
Now, RMP gets protected at the *same* point where SNP is enabled and then
RMP checking is started. And this is the same point at which RMPOPT
optimizations are enabled with this patch.
I believe you are talking about the hardware doing it as part of SNP enablement,
but that isn't how it is implemented and the reasons for that are it would take
a long time (in CPU terms) for a single WRMSR, and we don't support that.
And if RMP has been allocated means that you are going to be running SNP guests,
otherwise you wouldn't have allocated the RMP and enabled SNP in BIOS.
The RMPOPT feature address the RMP checks associated with non-SNP guests and the
hypervisor itself, theoretically, a cloud provider has good memory placement for
guests and can benefit even when launching/running SNP guests.
We can simplify this initial series to just using this RMPOPT feature and enabling
RMP optimizations for 0 to 2TB across the system and then do the optimizations
for/or supporting larger systems as a follow on series.
That will address your concerns of performing the RMPOPT optimizations at
SEV-SNP enabled time, and having the important SEV-SNP configuration
optimized by default and with little kernel code needed to drive it.
Thanks,
Ashish
^ permalink raw reply
* Re: [PATCH v3 00/16] x86/msr: Inline rdmsr/wrmsr instructions
From: H. Peter Anvin @ 2026-02-18 20:37 UTC (permalink / raw)
To: Juergen Gross, linux-kernel, x86, linux-coco, kvm, linux-hyperv,
virtualization, llvm
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
Kiryl Shutsemau, Rick Edgecombe, Sean Christopherson,
Paolo Bonzini, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, Long Li, Vitaly Kuznetsov, Boris Ostrovsky, xen-devel,
Ajay Kaher, Alexey Makhalov, Broadcom internal kernel review list,
Andy Lutomirski, Peter Zijlstra, Xin Li, Nathan Chancellor,
Nick Desaulniers, Bill Wendling, Justin Stitt, Josh Poimboeuf,
andy.cooper
In-Reply-To: <20260218082133.400602-1-jgross@suse.com>
On February 18, 2026 12:21:17 AM PST, Juergen Gross <jgross@suse.com> wrote:
>When building a kernel with CONFIG_PARAVIRT_XXL the paravirt
>infrastructure will always use functions for reading or writing MSRs,
>even when running on bare metal.
>
>Switch to inline RDMSR/WRMSR instructions in this case, reducing the
>paravirt overhead.
>
>The first patch is a prerequisite fix for alternative patching. Its
>is needed due to the initial indirect call needs to be padded with
>NOPs in some cases with the following patches.
>
>In order to make this less intrusive, some further reorganization of
>the MSR access helpers is done in the patches 1-6.
>
>The next 4 patches are converting the non-paravirt case to use direct
>inlining of the MSR access instructions, including the WRMSRNS
>instruction and the immediate variants of RDMSR and WRMSR if possible.
>
>Patches 11-13 are some further preparations for making the real switch
>to directly patch in the native MSR instructions easier.
>
>Patch 14 is switching the paravirt MSR function interface from normal
>call ABI to one more similar to the native MSR instructions.
>
>Patch 15 is a little cleanup patch.
>
>Patch 16 is the final step for patching in the native MSR instructions
>when not running as a Xen PV guest.
>
>This series has been tested to work with Xen PV and on bare metal.
>
>Note that there is more room for improvement. This series is sent out
>to get a first impression how the code will basically look like.
Does that mean you are considering this patchset an RFC? If so, you should put that in the subject header.
>Right now the same problem is solved differently for the paravirt and
>the non-paravirt cases. In case this is not desired, there are two
>possibilities to merge the two implementations. Both solutions have
>the common idea to have rather similar code for paravirt and
>non-paravirt variants, but just use a different main macro for
>generating the respective code. For making the code of both possible
>scenarios more similar, the following variants are possible:
>
>1. Remove the micro-optimizations of the non-paravirt case, making
> it similar to the paravirt code in my series. This has the
> advantage of being more simple, but might have a very small
> negative performance impact (probably not really detectable).
>
>2. Add the same micro-optimizations to the paravirt case, requiring
> to enhance paravirt patching to support a to be patched indirect
> call in the middle of the initial code snipplet.
>
>In both cases the native MSR function variants would no longer be
>usable in the paravirt case, but this would mostly affect Xen, as it
>would need to open code the WRMSR/RDMSR instructions to be used
>instead the native_*msr*() functions.
>
>Changes since V2:
>- switch back to the paravirt approach
>
>Changes since V1:
>- Use Xin Li's approach for inlining
>- Several new patches
>
>Juergen Gross (16):
> x86/alternative: Support alt_replace_call() with instructions after
> call
> coco/tdx: Rename MSR access helpers
> x86/sev: Replace call of native_wrmsr() with native_wrmsrq()
> KVM: x86: Remove the KVM private read_msr() function
> x86/msr: Minimize usage of native_*() msr access functions
> x86/msr: Move MSR trace calls one function level up
> x86/opcode: Add immediate form MSR instructions
> x86/extable: Add support for immediate form MSR instructions
> x86/msr: Use the alternatives mechanism for WRMSR
> x86/msr: Use the alternatives mechanism for RDMSR
> x86/alternatives: Add ALTERNATIVE_4()
> x86/paravirt: Split off MSR related hooks into new header
> x86/paravirt: Prepare support of MSR instruction interfaces
> x86/paravirt: Switch MSR access pv_ops functions to instruction
> interfaces
> x86/msr: Reduce number of low level MSR access helpers
> x86/paravirt: Use alternatives for MSR access with paravirt
>
> arch/x86/coco/sev/internal.h | 7 +-
> arch/x86/coco/tdx/tdx.c | 8 +-
> arch/x86/hyperv/ivm.c | 2 +-
> arch/x86/include/asm/alternative.h | 6 +
> arch/x86/include/asm/fred.h | 2 +-
> arch/x86/include/asm/kvm_host.h | 10 -
> arch/x86/include/asm/msr.h | 345 ++++++++++++++++------
> arch/x86/include/asm/paravirt-msr.h | 148 ++++++++++
> arch/x86/include/asm/paravirt.h | 67 -----
> arch/x86/include/asm/paravirt_types.h | 57 ++--
> arch/x86/include/asm/qspinlock_paravirt.h | 4 +-
> arch/x86/kernel/alternative.c | 5 +-
> arch/x86/kernel/cpu/mshyperv.c | 7 +-
> arch/x86/kernel/kvmclock.c | 2 +-
> arch/x86/kernel/paravirt.c | 42 ++-
> arch/x86/kvm/svm/svm.c | 16 +-
> arch/x86/kvm/vmx/tdx.c | 2 +-
> arch/x86/kvm/vmx/vmx.c | 8 +-
> arch/x86/lib/x86-opcode-map.txt | 5 +-
> arch/x86/mm/extable.c | 35 ++-
> arch/x86/xen/enlighten_pv.c | 52 +++-
> arch/x86/xen/pmu.c | 4 +-
> tools/arch/x86/lib/x86-opcode-map.txt | 5 +-
> tools/objtool/check.c | 1 +
> 24 files changed, 576 insertions(+), 264 deletions(-)
> create mode 100644 arch/x86/include/asm/paravirt-msr.h
>
Could you clarify *on the high design level* what "go back to the paravirt approach" means, and the motivation for that?
Note that for Xen *most* MSRs fall in one of two categories: those that are dropped entirely and those that are just passed straight on to the hardware.
I don't know if anyone cares about optimizing PV Xen anymore, but at least in theory Xen can un-paravirtualize most sites.
^ permalink raw reply
* Re: [RFC PATCH v5 05/45] KVM: TDX: Drop kvm_x86_ops.link_external_spt(), use .set_external_spte() for all
From: Edgecombe, Rick P @ 2026-02-18 19:37 UTC (permalink / raw)
To: seanjc@google.com, Zhao, Yan Y
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Li, Xiaoyao,
Huang, Kai, dave.hansen@linux.intel.com, kas@kernel.org,
binbin.wu@linux.intel.com, mingo@redhat.com, pbonzini@redhat.com,
ackerleytng@google.com, linux-kernel@vger.kernel.org,
Yamahata, Isaku, sagis@google.com, tglx@kernel.org, bp@alien8.de,
Annapurve, Vishal, x86@kernel.org
In-Reply-To: <aYJU8Som706YkIEO@google.com>
On Tue, 2026-02-03 at 20:05 +0000, Sean Christopherson wrote:
> > And mirror_spte --> new_spte?
>
> Hmm, ya, I made that change later, but it can probably be shifted here.
Sorry for the late comment on the tiny detail, but things seemed to have calmed
down enough to attempt to merge these discussions into the snarl.
It doesn't quite fit in this patch because the set_external_spte() op also uses
the mirror_pte name. So then you need to either expand the scope of the patch to
change "mirror" to "new" across the callchain, or creating a small mismatch
between tdx_sept_set_private_spte() and tdx_sept_link_private_spt().
The patch where it happens in this series needs to add the old_pte, forcing
mirror_spte to grow some new nomenclature. So on balance I think it fits better
there, and we should leave it alone here. We can update it in
tdx_sept_link_private_spt() in "KVM: x86/mmu: Plumb the old_spte into
kvm_x86_ops.set_external_spte()".
^ permalink raw reply
* Re: [PATCH 2/6] x86/sev: add support for enabling RMPOPT
From: Dave Hansen @ 2026-02-18 17:17 UTC (permalink / raw)
To: Kalra, Ashish, K Prateek Nayak, tglx, mingo, bp, dave.hansen, x86,
hpa, seanjc, peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, Tycho.Andersen, Nathan.Fontenot,
jackyli, pgonda, rientjes, jacobhxu, xin, pawan.kumar.gupta,
babu.moger, dyoung, nikunj, john.allen, darwi, linux-kernel,
linux-crypto, kvm, linux-coco
In-Reply-To: <e774a45a-c31d-4051-93f7-3848de539888@amd.com>
On 2/18/26 09:07, Kalra, Ashish wrote:
> On 2/18/2026 11:01 AM, Dave Hansen wrote:
>> On 2/18/26 08:55, Kalra, Ashish wrote:
>>> Because, setting RMPOPT_BASE MSR (which is a per-core MSR) and
>>> RMPOPT instruction need to be issued on only one thread per core. If
>>> the primary thread is offlined and secondary thread is not
>>> considered, we will miss/skip setting either the RMPOPT_BASE MSR or
>>> not issuing the RMPOPT instruction for that physical CPU, which
>>> means no RMP optimizations enabled for that physical CPU.
>> What is the harm of issuing it twice per core?
> Why to issue it if we can avoid it.
>
> It is not that complex to setup a cpumask containing the online primary
> or secondary thread and then issue the RMPOPT instruction only once per
> thread.
It's a non-zero amount of error-prone kernel code. It *is* complex. It
has to be reviewed and maintained.
Please remove this unnecessary optimization from the series. If you
would like to add it back, please do it in a patch at the end so it can
be evaluated on its own. Include performance numbers so the code
complexity can be balanced against the performance gain.
^ permalink raw reply
* Re: [PATCH 0/6] Add RMPOPT support.
From: Dave Hansen @ 2026-02-18 17:15 UTC (permalink / raw)
To: Kalra, Ashish, tglx, mingo, bp, dave.hansen, x86, hpa, seanjc,
peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <cc930514-b0c0-4b9f-8287-aaee2878e668@amd.com>
On 2/18/26 09:03, Kalra, Ashish wrote:
>> They are known not to contain any SEV-SNP guest memory at the
>> moment snp_rmptable_init() finishes, no?
> Yes, but RMP checks are still performed and they affect performance.
>
> Testing a bit in the per‑CPU RMPOPT table to avoid RMP checks
> significantly improves performance.
Sorry, Ashish, I don't think I'm explaining myself very well. Let me try
again, please.
First, my goal here is to ensure that the system has a whole has good
performance, with minimal kernel code, and in the most common
configurations.
I would wager that the most common SEV-SNP configuration in the whole
world is a system that has booted, enabled SEV-SNP, and has never run an
SEV-SNP guest. If it's not *the* most common, it's certainly going to be
common enough to care about deeply.
Do you agree?
If you agree, I hope we can also agree that a "SNP enabled but never ran
a guest" state is deserving of good performance with minimal kernel code.
My assumption (which is maybe a bad one) is that there is a natural
point when SEV-SNP is enabled on the system when the system as a whole
can easily assert that no SEV-SNP guest has ever run. I'm assuming that
there is *a* point where, for instance, the RMP table gets atomically
flipped from being unprotected to being protected. At that point, its
state *must* be known. It must also be naturally obvious that no guest
has had a chance to run at this point.
If that point can be leveraged, and the RMPOPT optimization can be
applied at SEV-SNP enabled time, then an important SEV-SNP configuration
would be optimized by default and with zero or little kernel code needed
to drive it.
To me, that seems like a valuable goal.
Do you agree?
^ permalink raw reply
* Re: [PATCH 2/6] x86/sev: add support for enabling RMPOPT
From: Kalra, Ashish @ 2026-02-18 17:07 UTC (permalink / raw)
To: Dave Hansen, K Prateek Nayak, tglx, mingo, bp, dave.hansen, x86,
hpa, seanjc, peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, Tycho.Andersen, Nathan.Fontenot,
jackyli, pgonda, rientjes, jacobhxu, xin, pawan.kumar.gupta,
babu.moger, dyoung, nikunj, john.allen, darwi, linux-kernel,
linux-crypto, kvm, linux-coco
In-Reply-To: <eaa17a3f-e481-49dc-8d7b-bd5247d3ed57@intel.com>
On 2/18/2026 11:01 AM, Dave Hansen wrote:
> On 2/18/26 08:55, Kalra, Ashish wrote:
>> Because, setting RMPOPT_BASE MSR (which is a per-core MSR) and
>> RMPOPT instruction need to be issued on only one thread per core. If
>> the primary thread is offlined and secondary thread is not
>> considered, we will miss/skip setting either the RMPOPT_BASE MSR or
>> not issuing the RMPOPT instruction for that physical CPU, which
>> means no RMP optimizations enabled for that physical CPU.
> What is the harm of issuing it twice per core?
Why to issue it if we can avoid it.
It is not that complex to setup a cpumask containing the online primary
or secondary thread and then issue the RMPOPT instruction only once per
thread.
Thanks,
Ashish
^ permalink raw reply
* Re: [PATCH 0/6] Add RMPOPT support.
From: Kalra, Ashish @ 2026-02-18 17:03 UTC (permalink / raw)
To: Dave Hansen, tglx, mingo, bp, dave.hansen, x86, hpa, seanjc,
peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <31b42ba3-dd0c-42e7-ad1e-800c5cd2bcf8@intel.com>
On 2/18/2026 9:03 AM, Dave Hansen wrote:
> On 2/17/26 20:12, Kalra, Ashish wrote:
>>> That's not awful.
>>>
>>> To be honest, though, I think this is misdesigned. Shouldn't the CPU
>>> *boot* in a state where it is optimized? Why should software have to
>>> tell it that coming out of reset, there is no SEV-SNP memory?
>> When the CPU boots, the RMP checks are not done and therefore the CPU
>> is booting in a state where it is optimized.
>>
>> The RMP checks are not enabled till SEV-SNP is enabled and SNP is enabled
>> during kernel boot (as part of iommu_snp_enable() -> snp_rmptable_init()).
>>
>> Once SNP is enabled as part of kernel boot, hypervisor and non-SNP guests are
>> subject to RMP checks on writes to provide integrity of SEV-SNP guest memory.
>>
>> Therefore, we need to enable these RMP optimizations after SNP has been
>> enabled to indicate which 1GB regions of memory are known to not contain any
>> SEV-SNP guest memory.
>
> They are known not to contain any SEV-SNP guest memory at the moment
> snp_rmptable_init() finishes, no?
Yes, but RMP checks are still performed and they affect performance.
Testing a bit in the per‑CPU RMPOPT table to avoid RMP checks significantly improves performance.
Thanks,
Ashish
^ permalink raw reply
* Re: [PATCH 2/6] x86/sev: add support for enabling RMPOPT
From: Dave Hansen @ 2026-02-18 17:01 UTC (permalink / raw)
To: Kalra, Ashish, K Prateek Nayak, tglx, mingo, bp, dave.hansen, x86,
hpa, seanjc, peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, Tycho.Andersen, Nathan.Fontenot,
jackyli, pgonda, rientjes, jacobhxu, xin, pawan.kumar.gupta,
babu.moger, dyoung, nikunj, john.allen, darwi, linux-kernel,
linux-crypto, kvm, linux-coco
In-Reply-To: <4e912046-8ae0-4cb2-b2cb-11c754df7536@amd.com>
On 2/18/26 08:55, Kalra, Ashish wrote:
> Because, setting RMPOPT_BASE MSR (which is a per-core MSR) and
> RMPOPT instruction need to be issued on only one thread per core. If
> the primary thread is offlined and secondary thread is not
> considered, we will miss/skip setting either the RMPOPT_BASE MSR or
> not issuing the RMPOPT instruction for that physical CPU, which
> means no RMP optimizations enabled for that physical CPU.
What is the harm of issuing it twice per core?
^ permalink raw reply
* Re: [PATCH 2/6] x86/sev: add support for enabling RMPOPT
From: Kalra, Ashish @ 2026-02-18 16:55 UTC (permalink / raw)
To: Dave Hansen, K Prateek Nayak, tglx, mingo, bp, dave.hansen, x86,
hpa, seanjc, peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, Tycho.Andersen, Nathan.Fontenot,
jackyli, pgonda, rientjes, jacobhxu, xin, pawan.kumar.gupta,
babu.moger, dyoung, nikunj, john.allen, darwi, linux-kernel,
linux-crypto, kvm, linux-coco
In-Reply-To: <59c0b0f0-26b0-4311-82a9-a5f8392ec4c6@intel.com>
On 2/18/2026 8:59 AM, Dave Hansen wrote:
> On 2/17/26 19:08, K Prateek Nayak wrote:
>> Hello Dave,
>>
>> On 2/18/2026 3:36 AM, Dave Hansen wrote:
>>>> +/*
>>>> + * Build a cpumask of online primary threads, accounting for primary threads
>>>> + * that have been offlined while their secondary threads are still online.
>>>> + */
>>>> +static void get_cpumask_of_primary_threads(cpumask_var_t cpulist)
>>>> +{
>>>> + cpumask_t cpus;
>>>> + int cpu;
>>>> +
>>>> + cpumask_copy(&cpus, cpu_online_mask);
>>>> + for_each_cpu(cpu, &cpus) {
>>>> + cpumask_set_cpu(cpu, cpulist);
>>>> + cpumask_andnot(&cpus, &cpus, cpu_smt_mask(cpu));
>>>> + }
>>>> +}
>>>
>>> Don't we have a primary thread mask already? I thought we did.
>>
>> If you are referring to cpu_primary_thread_mask(), the CPUs are set on it
>> based on the LSB of APICID, specifically:
>>
>> !(apicid & (__max_threads_per_core - 1))
>>
>> It can so happen, the primary thread ((apicid & 1) == 0) of the core is
>> offline while the secondary thread ((apicid & 1) == 1) is online but the
>> traversal of (cpu_primary_thread_mask() & cpu_online_mask()) will simply
>> skip these cores.
>>
>> Is there an equivalent mask that sets the first online CPU of each core?
>
> No I don't think we have that sitting around.
>
> But, stepping back, why is this even necessary? Is it just saving a few
> IPIs in the super rare case that someone has offlined the primary thread
> but not a secondary one?
>
> Why bother?
Because, setting RMPOPT_BASE MSR (which is a per-core MSR) and RMPOPT instruction
need to be issued on only one thread per core. If the primary thread is offlined
and secondary thread is not considered, we will miss/skip setting either the
RMPOPT_BASE MSR or not issuing the RMPOPT instruction for that physical CPU, which means
no RMP optimizations enabled for that physical CPU.
Thanks,
Ashish
^ permalink raw reply
* Re: [PATCH 3/6] x86/sev: add support for RMPOPT instruction
From: Uros Bizjak @ 2026-02-18 16:28 UTC (permalink / raw)
To: Ashish Kalra, tglx, mingo, bp, dave.hansen, x86, hpa, seanjc,
peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <66348e8ad761a1b0ccb26c8027efedf46329db54.1771321114.git.ashish.kalra@amd.com>
On 2/17/26 21:10, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
>
> As SEV-SNP is enabled by default on boot when an RMP table is
> allocated by BIOS, the hypervisor and non-SNP guests are subject to
> RMP write checks to provide integrity of SNP guest memory.
>
> RMPOPT is a new instruction that minimizes the performance overhead of
> RMP checks on the hypervisor and on non-SNP guests by allowing RMP
> checks to be skipped for 1GB regions of memory that are known not to
> contain any SEV-SNP guest memory.
>
> Enable RMPOPT optimizations globally for all system RAM at RMP
> initialization time. RMP checks can initially be skipped for 1GB memory
> ranges that do not contain SEV-SNP guest memory (excluding preassigned
> pages such as the RMP table and firmware pages). As SNP guests are
> launched, RMPUPDATE will disable the corresponding RMPOPT optimizations.
>
> Suggested-by: Thomas Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
> arch/x86/virt/svm/sev.c | 84 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 84 insertions(+)
>
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index e6b784d26c33..a0d38fc50698 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -19,6 +19,7 @@
> #include <linux/iommu.h>
> #include <linux/amd-iommu.h>
> #include <linux/nospec.h>
> +#include <linux/kthread.h>
>
> #include <asm/sev.h>
> #include <asm/processor.h>
> @@ -127,10 +128,17 @@ static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
>
> static unsigned long snp_nr_leaked_pages;
>
> +enum rmpopt_function {
> + RMPOPT_FUNC_VERIFY_AND_REPORT_STATUS,
> + RMPOPT_FUNC_REPORT_STATUS
> +};
> +
> #define RMPOPT_TABLE_MAX_LIMIT_IN_TB 2
> #define NUM_TB(pfn_min, pfn_max) \
> (((pfn_max) - (pfn_min)) / (1 << (40 - PAGE_SHIFT)))
>
> +static struct task_struct *rmpopt_task;
> +
> struct rmpopt_socket_config {
> unsigned long start_pfn, end_pfn;
> cpumask_var_t cpulist;
> @@ -527,6 +535,66 @@ static void get_cpumask_of_primary_threads(cpumask_var_t cpulist)
> }
> }
>
> +/*
> + * 'val' is a system physical address aligned to 1GB OR'ed with
> + * a function selection. Currently supported functions are 0
> + * (verify and report status) and 1 (report status).
> + */
> +static void rmpopt(void *val)
> +{
> + asm volatile(".byte 0xf2, 0x0f, 0x01, 0xfc\n\t"
There is no need for \n\t instruction delimiter with single instruction
in the asm template, it will just confuse compiler's insn count estimator.
Uros.
> + : : "a" ((u64)val & PUD_MASK), "c" ((u64)val & 0x1)
> + : "memory", "cc");
> +}
> +
> +static int rmpopt_kthread(void *__unused)
> +{
> + phys_addr_t pa_start, pa_end;
> + cpumask_var_t cpus;
> +
> + if (!zalloc_cpumask_var(&cpus, GFP_KERNEL))
> + return -ENOMEM;
> +
> + pa_start = ALIGN_DOWN(PFN_PHYS(min_low_pfn), PUD_SIZE);
> + pa_end = ALIGN(PFN_PHYS(max_pfn), PUD_SIZE);
> +
> + while (!kthread_should_stop()) {
> + phys_addr_t pa;
> +
> + pr_info("RMP optimizations enabled on physical address range @1GB alignment [0x%016llx - 0x%016llx]\n",
> + pa_start, pa_end);
> +
> + /* Only one thread per core needs to issue RMPOPT instruction */
> + get_cpumask_of_primary_threads(cpus);
> +
> + /*
> + * RMPOPT optimizations skip RMP checks at 1GB granularity if this range of
> + * memory does not contain any SNP guest memory.
> + */
> + for (pa = pa_start; pa < pa_end; pa += PUD_SIZE) {
> + /* Bit zero passes the function to the RMPOPT instruction. */
> + on_each_cpu_mask(cpus, rmpopt,
> + (void *)(pa | RMPOPT_FUNC_VERIFY_AND_REPORT_STATUS),
> + true);
> +
> + /* Give a chance for other threads to run */
> + cond_resched();
> + }
> +
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule();
> + }
> +
> + free_cpumask_var(cpus);
> + return 0;
> +}
> +
> +static void rmpopt_all_physmem(void)
> +{
> + if (rmpopt_task)
> + wake_up_process(rmpopt_task);
> +}
> +
> static void __configure_rmpopt(void *val)
> {
> u64 rmpopt_base = ((u64)val & PUD_MASK) | MSR_AMD64_RMPOPT_ENABLE;
> @@ -687,6 +755,22 @@ static __init void configure_and_enable_rmpopt(void)
> else
> configure_rmpopt_large_physmem(primary_threads_cpulist);
>
> + rmpopt_task = kthread_create(rmpopt_kthread, NULL, "rmpopt_kthread");
> + if (IS_ERR(rmpopt_task)) {
> + pr_warn("Unable to start RMPOPT kernel thread\n");
> + rmpopt_task = NULL;
> + goto free_cpumask;
> + }
> +
> + pr_info("RMPOPT worker thread created with PID %d\n", task_pid_nr(rmpopt_task));
> +
> + /*
> + * Once all per-CPU RMPOPT tables have been configured, enable RMPOPT
> + * optimizations on all physical memory.
> + */
> + rmpopt_all_physmem();
> +
> +free_cpumask:
> free_cpumask_var(primary_threads_cpulist);
> }
>
^ permalink raw reply
* Re: [PATCH 5/6] x86/sev: Use configfs to re-enable RMP optimizations.
From: Dave Hansen @ 2026-02-18 15:10 UTC (permalink / raw)
To: Kalra, Ashish, tglx, mingo, bp, dave.hansen, x86, hpa, seanjc,
peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <e72165ed-c65d-4d21-bff6-9981b46311cf@amd.com>
On 2/17/26 19:34, Kalra, Ashish wrote:
...
> As currently, i.e, as part of this patch series, there is no
> mechanism to re-issue RMPOPT automatically as part of SNP guest
> cleanup, therefore this support exists to doing it manually at
> runtime via configfs.
I think you need a mechanism that re-enable RMP optimizations
automatically for this feature to go upstream. It's just dead code
otherwise, and we don't merge dead code.
A configfs hack doesn't really count.
^ permalink raw reply
* Re: [PATCH 0/6] Add RMPOPT support.
From: Dave Hansen @ 2026-02-18 15:03 UTC (permalink / raw)
To: Kalra, Ashish, tglx, mingo, bp, dave.hansen, x86, hpa, seanjc,
peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <65986f9e-59e8-4f1c-aaa7-1edf45af24d8@amd.com>
On 2/17/26 20:12, Kalra, Ashish wrote:
>> That's not awful.
>>
>> To be honest, though, I think this is misdesigned. Shouldn't the CPU
>> *boot* in a state where it is optimized? Why should software have to
>> tell it that coming out of reset, there is no SEV-SNP memory?
> When the CPU boots, the RMP checks are not done and therefore the CPU
> is booting in a state where it is optimized.
>
> The RMP checks are not enabled till SEV-SNP is enabled and SNP is enabled
> during kernel boot (as part of iommu_snp_enable() -> snp_rmptable_init()).
>
> Once SNP is enabled as part of kernel boot, hypervisor and non-SNP guests are
> subject to RMP checks on writes to provide integrity of SEV-SNP guest memory.
>
> Therefore, we need to enable these RMP optimizations after SNP has been
> enabled to indicate which 1GB regions of memory are known to not contain any
> SEV-SNP guest memory.
They are known not to contain any SEV-SNP guest memory at the moment
snp_rmptable_init() finishes, no?
^ permalink raw reply
* Re: [PATCH 2/6] x86/sev: add support for enabling RMPOPT
From: Dave Hansen @ 2026-02-18 14:59 UTC (permalink / raw)
To: K Prateek Nayak, Ashish Kalra, tglx, mingo, bp, dave.hansen, x86,
hpa, seanjc, peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, Tycho.Andersen, Nathan.Fontenot,
jackyli, pgonda, rientjes, jacobhxu, xin, pawan.kumar.gupta,
babu.moger, dyoung, nikunj, john.allen, darwi, linux-kernel,
linux-crypto, kvm, linux-coco
In-Reply-To: <b860e5f4-4111-4de7-acc7-aec4a3f23908@amd.com>
On 2/17/26 19:08, K Prateek Nayak wrote:
> Hello Dave,
>
> On 2/18/2026 3:36 AM, Dave Hansen wrote:
>>> +/*
>>> + * Build a cpumask of online primary threads, accounting for primary threads
>>> + * that have been offlined while their secondary threads are still online.
>>> + */
>>> +static void get_cpumask_of_primary_threads(cpumask_var_t cpulist)
>>> +{
>>> + cpumask_t cpus;
>>> + int cpu;
>>> +
>>> + cpumask_copy(&cpus, cpu_online_mask);
>>> + for_each_cpu(cpu, &cpus) {
>>> + cpumask_set_cpu(cpu, cpulist);
>>> + cpumask_andnot(&cpus, &cpus, cpu_smt_mask(cpu));
>>> + }
>>> +}
>>
>> Don't we have a primary thread mask already? I thought we did.
>
> If you are referring to cpu_primary_thread_mask(), the CPUs are set on it
> based on the LSB of APICID, specifically:
>
> !(apicid & (__max_threads_per_core - 1))
>
> It can so happen, the primary thread ((apicid & 1) == 0) of the core is
> offline while the secondary thread ((apicid & 1) == 1) is online but the
> traversal of (cpu_primary_thread_mask() & cpu_online_mask()) will simply
> skip these cores.
>
> Is there an equivalent mask that sets the first online CPU of each core?
No I don't think we have that sitting around.
But, stepping back, why is this even necessary? Is it just saving a few
IPIs in the super rare case that someone has offlined the primary thread
but not a secondary one?
Why bother?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox