* Re: [PATCH v2 3/5] x86/virt/tdx: Add SEAMCALL wrapper for TDH.SYS.DISABLE
From: Kiryl Shutsemau @ 2026-04-01 9:26 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: Verma, Vishal L, seanjc@google.com, bp@alien8.de, x86@kernel.org,
hpa@zytor.com, mingo@redhat.com, linux-kernel@vger.kernel.org,
dave.hansen@linux.intel.com, tglx@kernel.org, pbonzini@redhat.com,
linux-coco@lists.linux.dev, kvm@vger.kernel.org
In-Reply-To: <944b3fe5fa8955319030a7dfc0ea164bb0266e68.camel@intel.com>
On Tue, Mar 31, 2026 at 09:36:03PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2026-03-31 at 18:22 +0000, Verma, Vishal L wrote:
> > >
> > > I guess the actual behaviour is dependant on the return code. It is
> > > obviously going to be the case for TDX_SUCCESS. And from the discussion,
> > > I guess that's true for TDX_SYS_BUSY and TDX_INTERRUPTED_RESUMABLE.
> > >
> > > What about other cases? The spec draft also lists TDX_SYS_NOT_READY and
> > > TDX_SYS_SHUTDOWN.
> >
> > I think these are safe too - TDX_SYS_SHUTDOWN means the module has
> > already been shutdown, which this seamcall would've done, so things
> > should be in the same state either way.
> >
> > TDX_SYS_NOT_READY means the module hasn't been initialized yet. This
> > seamcall should just exit, and the module is already blocking any
> > seamcall that need the module to be initialized. The seamcalls to
> > initialize the module will be allowed, as they are after a sys_disable
> > call anyway.
>
> Should the seamcall return success in the case where it would return
> TDX_SYS_NOT_READY? It is in basically a reset state right? The errors we care
> about are actual errors (TDX_SW_ERROR), so it makes no difference to the code in
> the patch. But it might be a nicer API for the seamcall?
I am not sure. TDX_SYS_NOT_READY can be useful as might indicate
mismatch of system state understanding between kernel and TDX module.
> > > I wounder if it can affect the kernel. Consider the case when kexec
> > > (crash kernel start) happens due to crash on TDX module.
> > >
> > > Will we be able to shutdown TDX module cleanly and make kexec safe?
> >
> > Hm -are the semantics for what happens if there is a crash in the
> > module defined?
I meant kernel crash around/before TDX module initialization. Sorry for
confusion.
> > I think Linux should expect that sys_disable should
> > either start doing its shutdown work, or exit with one of the other
> > defined exit statuses. Anything else would be considered a module bug.
>
> We often have the question come up about how much we should to guard against
> bugs in the TDX module. I tend to also think we should not do defensive
> programming, same as we do for the kernel. If it's easy to handle something or
> emit a warning it's nice, but otherwise the solution for such cases should be to
> fix the TDX module bug.
>
> But for the kdump case, we don't actually need sys disable to succeed. The kdump
> kernel will not load the TDX module.
AFAIK, it is possible to start a normal kernel after kdump is done with
kexec (requires memmap= tricks). And the normal kernel might want to use
TDX again.
Not sure if it is done in practice. I would rather go full reboot path
after crash.
> And as for the errata, this already needs a
> special situation to be a problem. But even if it happens, I'd think better to
> try to the kdump. Not sure what the fix would be for that scenario, even if we
> allowed for a large complexity budget. So best effort seems good.
>
> Does it seem reasonable?
I am probably too picky here. We want to start from make basic kexec
functionality to work for start.
Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH 1/2] x86/tdx: Fix off-by-one in port I/O handling
From: Kiryl Shutsemau @ 2026-04-01 8:34 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H . Peter Anvin, Rick Edgecombe, Borys Tsyrulnikov, linux-kernel,
linux-coco, kvm, stable
In-Reply-To: <ee096f1e-b994-4d56-a78c-cb0e867ea047@linux.intel.com>
On Tue, Mar 31, 2026 at 02:57:32PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Hi Kirill,
>
> On 3/31/2026 4:24 AM, Kiryl Shutsemau (Meta) wrote:
> > handle_in() and handle_out() in arch/x86/coco/tdx/tdx.c use:
> >
> > u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
> >
> > GENMASK(h, l) includes bit h. For size=1 (INB), this produces
> > GENMASK(8, 0) = 0x1FF (9 bits) instead of GENMASK(7, 0) = 0xFF (8
> > bits). The mask is one bit too wide for all I/O sizes.
> >
> > Fix the mask calculation.
> >
> > Fixes: 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")
> > Reported-by: Borys Tsyrulnikov <tsyrulnikov.borys@gmail.com>
> > Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
>
> LGTM. Can you include a link to the bug report or related discussion in
> the commit log? It will help understand the impact of this issue.
Link: https://lore.kernel.org/all/CAKw_Dz96rfSQc6Rn+9QBcUFHhmkK+9zu+P=bxowfZwxrATCBRg@mail.gmail.com/
It is relevant for both.
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v7 08/22] x86/virt/seamldr: Allocate and populate a module update request
From: Chao Gao @ 2026-04-01 8:27 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-kernel, linux-coco, kvm, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, kas, nik.borisov, paulmck,
pbonzini, reinette.chatre, rick.p.edgecombe, sagis, seanjc,
tony.lindgren, vannapurve, vishal.l.verma, yilun.xu, xiaoyao.li,
yan.y.zhao, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin
In-Reply-To: <6bec316e-3216-425c-89e2-fcf32fd4e15a@intel.com>
On Tue, Mar 31, 2026 at 08:44:36AM -0700, Dave Hansen wrote:
>On 3/31/26 05:41, Chao Gao wrote:
>> + ptr = sig;
>> + for (i = 0; i < sig_size / SZ_4K; i++) {
>> + /*
>> + * @sig is 4KB-aligned, but that does not imply PAGE_SIZE
>> + * alignment when PAGE_SIZE != SZ_4K. Always include the
>> + * in-page offset.
>> + */
>> + params->sigstruct_pa[i] = (vmalloc_to_pfn(ptr) << PAGE_SHIFT) +
>> + ((unsigned long)ptr & ~PAGE_MASK);
>> + ptr += SZ_4K;
>> + }
>
>There are a billion things the mainline kernel _could_ do. Like a 32-bit
>4/4 split or a non-4k PAGE_SIZE on x86. But the mainline kernel doesn't
>*do* this.
>
>Why add complexity to deal with something that doesn't exist?
The intent was to avoid sprinkling subtle assumptions that PAGE_SIZE == SZ_4K.
But I agree that on x86 that's not a realistic concern today. I will just
assume 4KB page size.
And I assume we don't need WARN_ON_ONCE(PAGE_SIZE != SZ_4K) since this is
unlikely to break soon and shouldn't be very hard to debug and fix if it
does.
^ permalink raw reply
* Re: [PATCH v7 07/22] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates
From: Chao Gao @ 2026-04-01 7:49 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-kernel, linux-coco, kvm, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, kas, nik.borisov, paulmck,
pbonzini, reinette.chatre, rick.p.edgecombe, sagis, seanjc,
tony.lindgren, vannapurve, vishal.l.verma, yilun.xu, xiaoyao.li,
yan.y.zhao, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin
In-Reply-To: <01fc8946-eb84-46fa-9458-f345dd3f6033@intel.com>
On Tue, Mar 31, 2026 at 08:11:22AM -0700, Dave Hansen wrote:
>On 3/31/26 05:41, Chao Gao wrote:
>> +static enum fw_upload_err tdx_fw_poll_complete(struct fw_upload *fwl)
>> +{
>> + /*
>> + * TDX module updates are completed in the previous phase
>> + * (tdx_fw_write()). If any error occurred, the previous phase
>> + * would return an error code to abort the update process. In
>> + * other words, reaching this point means the update succeeded.
>> + */
>> + return FW_UPLOAD_ERR_NONE;
>> +}
>> +
>
>This will be the seventh 'fw_upload_ops' and the third that has this
>pattern of not needing a ->poll_complete() implementation. It seems like
>allowing a NULL ->poll_complete or having a common stub for this pattern
>would be worthwhile.
Thanks for this suggestion. allowing a NULL ->poll_complete looks good.
I will post a separate series for this. the core change would be:
From b0d02c337db9df040b702f57a28e960da04bd9e3 Mon Sep 17 00:00:00 2001
From: Chao Gao <chao.gao@intel.com>
Date: Tue, 31 Mar 2026 22:17:36 -0700
Subject: [PATCH] firmware_loader: Make fw_upload_ops::poll_complete() optional
mpfs-auto-update.c and thp7312.c implement poll_complete() as a trivial
stub that simply returns FW_UPLOAD_ERR_NONE, because their write() is
synchronous and the upload completes before fw_upload_ops::write() returns.
Make poll_complete() optional so that drivers with synchronous write()
don't need to duplicate this trivial stub. When the callback is not
provided, assume the upload has completed and simply return
FW_UPLOAD_ERR_NONE.
An alternative would be defining and exporting a common stub, but
exporting a stub seems unnecessary and differs from the optional
fw_upload_ops::cleanup() handling.
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
Link: https://lore.kernel.org/kvm/01fc8946-eb84-46fa-9458-f345dd3f6033@intel.com/
---
drivers/base/firmware_loader/sysfs_upload.c | 12 +++++++++---
include/linux/firmware.h | 2 +-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
index f59a7856934c..06788a6d7227 100644
--- a/drivers/base/firmware_loader/sysfs_upload.c
+++ b/drivers/base/firmware_loader/sysfs_upload.c
@@ -158,6 +158,13 @@ static void fw_upload_prog_complete(struct fw_upload_priv *fwlp)
mutex_unlock(&fwlp->lock);
}
+static enum fw_upload_err fw_upload_poll_complete(struct fw_upload_priv *fwlp)
+{
+ if (!fwlp->ops->poll_complete)
+ return FW_UPLOAD_ERR_NONE;
+ return fwlp->ops->poll_complete(fwlp->fw_upload);
+}
+
static void fw_upload_main(struct work_struct *work)
{
struct fw_upload_priv *fwlp;
@@ -197,7 +204,7 @@ static void fw_upload_main(struct work_struct *work)
}
fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PROGRAMMING);
- ret = fwlp->ops->poll_complete(fwl);
+ ret = fw_upload_poll_complete(fwlp);
if (ret != FW_UPLOAD_ERR_NONE)
fw_upload_set_error(fwlp, ret);
@@ -306,8 +313,7 @@ firmware_upload_register(struct module *module, struct device *parent,
if (!name || name[0] == '\0')
return ERR_PTR(-EINVAL);
- if (!ops || !ops->cancel || !ops->prepare ||
- !ops->write || !ops->poll_complete) {
+ if (!ops || !ops->cancel || !ops->prepare || !ops->write) {
dev_err(parent, "Attempt to register without all required ops\n");
return ERR_PTR(-EINVAL);
}
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index aae1b85ffc10..c320e8b3e708 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -57,7 +57,7 @@ struct fw_upload {
* size written or a negative error code. The write()
* op will be called repeatedly until all data is
* written.
- * @poll_complete: Required: Check for the completion of the
+ * @poll_complete: Optional: Check for the completion of the
* HW authentication/programming process.
* @cancel: Required: Request cancellation of update. This op
* is called from the context of a different kernel
--
2.47.3
>
>I also don't think you need to be that verbose. This would be fine:
>
> /*
> * The upload completed during tdx_fw_write().
> * Never poll for completion.
> */
Sure. thanks.
>
>
>
^ permalink raw reply related
* Re: [PATCH v2 03/31] x86/virt/tdx: Add tdx_page_array helpers for new TDX Module objects
From: Tony Lindgren @ 2026-04-01 7:25 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: yilun.xu@linux.intel.com, Gao, Chao, Xu, Yilun, x86@kernel.org,
kas@kernel.org, baolu.lu@linux.intel.com,
dave.hansen@linux.intel.com, Li, Xiaoyao, Williams, Dan J,
Jiang, Dave, linux-pci@vger.kernel.org,
linux-coco@lists.linux.dev, linux-kernel@vger.kernel.org,
Duan, Zhenzhong, Verma, Vishal L, kvm@vger.kernel.org
In-Reply-To: <23b92a154c859450d106de6c9badbe284e3aa19f.camel@intel.com>
On Mon, Mar 30, 2026 at 11:25:08PM +0000, Edgecombe, Rick P wrote:
> On Mon, 2026-03-30 at 18:25 +0800, Xu Yilun wrote:
> > The caller is not aware of singleton mode. Actually, I'm trying to make
> > the tdx_page_array independent of HPA_ARRAY_T or HPA_LIST_INFO details
> > when allocating/populating, root page is still populated even not needed
> > for singleton mode. The differences only happen when collaping the struct
> > into u64 SEAMCALL parameters.
>
> It seems tdx_page_array combines two concepts. An array of pages, and the method
> that the pages get handed to the TDX module. What if we broke apart these
> concepts?
We could add an enum for the LIST_INFO type to intialized the tdx_page_array?
Then the code using the tdx_page_array could initialize the root page based on
the LIST_INFO type for the SEAMCALL.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v7 07/22] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates
From: Chao Gao @ 2026-04-01 3:10 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-kernel, linux-coco, kvm, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, kas, nik.borisov, paulmck,
pbonzini, reinette.chatre, rick.p.edgecombe, sagis, seanjc,
tony.lindgren, vannapurve, vishal.l.verma, yilun.xu, xiaoyao.li,
yan.y.zhao, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin
In-Reply-To: <2efa9f58-af65-4e77-9667-f66dc48a61c7@intel.com>
On Tue, Mar 31, 2026 at 08:04:19AM -0700, Dave Hansen wrote:
>On 3/31/26 05:41, Chao Gao wrote:
>> +static enum fw_upload_err tdx_fw_write(struct fw_upload *fwl, const u8 *data,
>> + u32 offset, u32 size, u32 *written)
>> +{
>> + int ret;
>> +
>> + /*
>> + * tdx_fw_write() always processes all data on the first call with
>> + * offset == 0. Since it never returns partial success (it either
>> + * succeeds completely or fails), there is no subsequent call with
>> + * non-zero offsets.
>> + */
>> + WARN_ON_ONCE(offset);
>
>How would this non-zero offset be triggered? Wouldn't it take a bug in
>the firmware upload code?
Yes, a bug in firmware upload code or tdx_fw_write() returns partial success.
>If so, why bother? It's not like something bad
>happens here.
the comment and WARN() was added to explain why @offset isn't used at all
in this function. But if you feel this detail is obvious or doesn't help
readability, let's drop it.
^ permalink raw reply
* Re: [PATCH v7 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Chao Gao @ 2026-04-01 2:25 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-coco, kvm, linux-kernel, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, kas, nik.borisov, paulmck,
pbonzini, reinette.chatre, rick.p.edgecombe, sagis, seanjc,
tony.lindgren, vannapurve, vishal.l.verma, yilun.xu, xiaoyao.li,
yan.y.zhao, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin
In-Reply-To: <95126568-3031-4d0a-85c8-d3934b6fec3e@intel.com>
On Tue, Mar 31, 2026 at 07:58:14AM -0700, Dave Hansen wrote:
>On 3/31/26 05:41, Chao Gao wrote:
>> +/*
>> + * Open-code DEVICE_ATTR_ADMIN_RO to specify a different 'show' function
>> + * for P-SEAMLDR version as version_show() is used for TDX module version.
>> + *
>> + * Admin-only readable as reading these attributes calls into P-SEAMLDR,
>> + * which may have potential performance and system impact.
>> + */
>> +static struct device_attribute dev_attr_seamldr_version =
>> + __ATTR(version, 0400, seamldr_version_show, NULL);
>> +static DEVICE_ATTR_ADMIN_RO(num_remaining_updates);
>
>I don't like that these are really *exactly* the same, except for the
>name but are defined so differently. I see three alternatives that are
>better:
>
>1. Open code *both* so they look the same
>2. Define a macro that both can use that has a seamldr_## prefix
>3. Move the code to a new file to avoid symbol conflicts.
>
>I'd honestly be fine with any of those.
>
>The other option is to just put the module information at the top
>directory so the sysfs name differentiates things.
>
>For real, how many things are going to be in the seamldr directory? If
>it's just two, is it worth having a directory?
Good question. There are two for this series, and I don't expect there will be
more soon.
The only reason to have a separate directory is that P-SEAMLDR is a different
component from the TDX module. But I think that's a bit weak. By renaming the
attribute to seamldr_version instead of version, there's no symbol conflict.
I will apply this incremental change:
diff --git a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
index f7221f2e5fec..1ace37b7f0e9 100644
--- a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
+++ b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
@@ -5,14 +5,14 @@ Description: (RO) Report the version of the loaded TDX module. The TDX module
"y" is the minor version and "z" is the update version. Versions
are used for bug reporting, TDX module updates etc.
-What: /sys/devices/faux/tdx_host/seamldr/version
+What: /sys/devices/faux/tdx_host/seamldr_version
Contact: linux-coco@lists.linux.dev
Description: (RO) Report the version of the loaded SEAM loader. The SEAM
loader 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 and compatibility checks.
-What: /sys/devices/faux/tdx_host/seamldr/num_remaining_updates
+What: /sys/devices/faux/tdx_host/num_remaining_updates
Contact: linux-coco@lists.linux.dev
Description: (RO) Report the number of remaining updates. TDX maintains a
log about each TDX module that has been loaded. This log has
diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
index 5a672126f372..cd84108094cf 100644
--- a/drivers/virt/coco/tdx-host/tdx-host.c
+++ b/drivers/virt/coco/tdx-host/tdx-host.c
@@ -75,15 +75,7 @@ static ssize_t num_remaining_updates_show(struct device *dev,
return sysfs_emit(buf, "%u\n", info.num_remaining_updates);
}
-/*
- * Open-code DEVICE_ATTR_ADMIN_RO to specify a different 'show' function
- * for P-SEAMLDR version as version_show() is used for TDX module version.
- *
- * Admin-only readable as reading these attributes calls into P-SEAMLDR,
- * which may have potential performance and system impact.
- */
-static struct device_attribute dev_attr_seamldr_version =
- __ATTR(version, 0400, seamldr_version_show, NULL);
+static DEVICE_ATTR_ADMIN_RO(seamldr_version);
static DEVICE_ATTR_ADMIN_RO(num_remaining_updates);
static struct attribute *seamldr_attrs[] = {
@@ -105,7 +97,6 @@ static bool seamldr_group_visible(struct kobject *kobj)
DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(seamldr);
static const struct attribute_group seamldr_group = {
- .name = "seamldr",
.attrs = seamldr_attrs,
.is_visible = SYSFS_GROUP_VISIBLE(seamldr),
};
^ permalink raw reply related
* Re: [PATCH v7 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Chao Gao @ 2026-04-01 1:57 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-coco, kvm, linux-kernel, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, kas, nik.borisov, paulmck,
pbonzini, reinette.chatre, rick.p.edgecombe, sagis, seanjc,
tony.lindgren, vannapurve, vishal.l.verma, yilun.xu, xiaoyao.li,
yan.y.zhao, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin
In-Reply-To: <114e44e8-b06f-4b45-b962-fab2e6c03b68@intel.com>
On Tue, Mar 31, 2026 at 07:58:02AM -0700, Dave Hansen wrote:
>On 3/31/26 05:41, Chao Gao wrote:
>> Expose them as tdx-host device attributes. Make seamldr attributes
>> visible only when the update feature is supported, as that's their sole
>> purpose. Unconditional exposure is also problematic because reading them
>> triggers P-SEAMLDR calls that break KVM on CPUs with a specific erratum
>> (to be enumerated and handled in a later patch).
>
>The erratum is irrelevant, IMNHO. Or it *should* be irrelevant.
OK. I'll drop the erratum mention from the commit message.
^ permalink raw reply
* Re: [PATCH v2 05/31] x86/virt/tdx: Extend tdx_page_array to support IOMMU_MT
From: Edgecombe, Rick P @ 2026-04-01 0:17 UTC (permalink / raw)
To: yilun.xu@linux.intel.com
Cc: Gao, Chao, Xu, Yilun, x86@kernel.org, kas@kernel.org,
baolu.lu@linux.intel.com, dave.hansen@linux.intel.com,
Li, Xiaoyao, Williams, Dan J, Jiang, Dave,
linux-pci@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Duan, Zhenzhong, Verma, Vishal L,
kvm@vger.kernel.org
In-Reply-To: <acvX1x5nDdGtZWyI@yilunxu-OptiPlex-7050>
On Tue, 2026-03-31 at 22:19 +0800, Xu Yilun wrote:
> > Consider the amount of tricks that are needed to coax the tdx_page_array to
> > populate the handoff page as needed. It adds 2 pages here, then subtracts
> > them
> > later in the callback. Then tweaks the pa in tdx_page_array_populate() to
> > add
> > the length...
>
> mm.. The tricky part is the specific memory requirement/allocation, the
> common part is the pa list contained in a root page. Maybe we only model
> the later, let the specific user does the memory allocation. Is that
> closer to your "break concepts apart" idea?
I haven't wrapped my head around this enough to suggest anything is definitely
the right approach.
But yes, the idea would be that the allocation of the list of pages to give to
the TDX module would be a separate allocation and set of management functions.
And the the allocation of the pages that are used to communicate the list of
pages (and in this case other args) with the module would be another set. So
each type of TDX module arg page format (IOMMU_MT, etc) would be separable, but
share the page list allocation part only. It looks like Nikolay was probing
along the same path. Not sure if he had the same solution in mind.
So for this:
1. Allocate a list or array of pages using a generic method.
2. Allocate these two IOMMU special pages.
3. Allocate memory needed for the seamcall (root pages)
Hand all three to the wrapper and have it shove them all through in the special
way it prefers.
Maybe... Can you write something about the similarities and differences with the
three types of lists in that series? Like in a compact form?
Also, how much of the earlier code duplication you wanted to avoid was the
leaking and special error handling stuff?
^ permalink raw reply
* Re: [PATCH v2 2/5] x86/virt/tdx: Pull kexec cache flush logic into arch/x86
From: Edgecombe, Rick P @ 2026-03-31 23:29 UTC (permalink / raw)
To: seanjc@google.com
Cc: pbonzini@redhat.com, bp@alien8.de, x86@kernel.org, kas@kernel.org,
hpa@zytor.com, linux-kernel@vger.kernel.org, Verma, Vishal L,
Huang, Kai, mingo@redhat.com, tglx@kernel.org,
linux-coco@lists.linux.dev, dave.hansen@linux.intel.com,
kvm@vger.kernel.org
In-Reply-To: <acxTBwaw6_xYSShf@google.com>
On Tue, 2026-03-31 at 16:04 -0700, Sean Christopherson wrote:
> > Oh, you acked it actually. But I was under the impression that after this
> > patch here, the splat wouldn't be triggered. So it inadvertently fixes it.
>
> Ah, that's why I was a bit confused. I was assuming tdx_shutdown_cpu() was a
> cpuhp callback, but it's actually an IPI callback.
>
> Hmm, isn't this patch wrong then? Ah, no, the changelog says:
>
> However, WBINVD is already generally done at CPU offline as matter of
> course. So don't bother adding TDX specific logic for this, and rely on the
> normal WBINVD to handle it.
>
> What's the "normal" WBINVD? At the very least, tdx_offline_cpu() should have
> a comment that explicitly calls out where that WBVIND is.
I guess we could add one in tdx_offline_cpu(). Seems reasonable.
> I assume you're
> referring to the wbinvd() calls in things like hlt_play_dead()?
Yea.
>
> But unless the WBINVD is actually costly, why bother getting fancy?
What is the suggestion to make it less fancy? Just put the wbinvd in
tdx_offline_cpu()? Yea that works too. Probably will get a comment either way.
^ permalink raw reply
* Re: [PATCH v2 2/5] x86/virt/tdx: Pull kexec cache flush logic into arch/x86
From: Sean Christopherson @ 2026-03-31 23:04 UTC (permalink / raw)
To: Rick P Edgecombe
Cc: Vishal L Verma, Kai Huang, bp@alien8.de, x86@kernel.org,
kas@kernel.org, hpa@zytor.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, dave.hansen@linux.intel.com,
tglx@kernel.org, pbonzini@redhat.com, linux-coco@lists.linux.dev,
kvm@vger.kernel.org
In-Reply-To: <6ad3ff51bbab85147b716de0b1f4e8b994b1998b.camel@intel.com>
On Tue, Mar 31, 2026, Rick P Edgecombe wrote:
> On Tue, 2026-03-31 at 12:22 -0700, Sean Christopherson wrote:
> > > -
> > > -#ifdef CONFIG_KEXEC_CORE
> > > -void tdx_cpu_flush_cache_for_kexec(void)
> > > -{
> > > - lockdep_assert_preemption_disabled();
> >
> > Is there a pre-existing bug here that gets propagate to tdx_shutdown_cpu()?
> > When called from kvm_offline_cpu(), preemption won't be fully disabled, but
> > per-CPU access are fine because the task is pinned to the target CPU.
> >
> > See https://lore.kernel.org/all/aUVx20ZRjOzKgKqy@google.com
>
> Yes. And actually it got hit during development of this series. This patch will
> conflict with the fix:
> https://lore.kernel.org/lkml/20260312100009.924136-1-kai.huang@intel.com/
>
> Oh, you acked it actually. But I was under the impression that after this patch
> here, the splat wouldn't be triggered. So it inadvertently fixes it.
Ah, that's why I was a bit confused. I was assuming tdx_shutdown_cpu() was a
cpuhp callback, but it's actually an IPI callback.
Hmm, isn't this patch wrong then? Ah, no, the changelog says:
However, WBINVD is already generally done at CPU offline as matter of course.
So don't bother adding TDX specific logic for this, and rely on the normal
WBINVD to handle it.
What's the "normal" WBINVD? At the very least, tdx_offline_cpu() should have a
comment that explicitly calls out where that WBVIND is. I assume you're referring
to the wbinvd() calls in things like hlt_play_dead()?
But unless the WBINVD is actually costly, why bother getting fancy?
> But that other patch is much more backport friendly.
^ permalink raw reply
* Re: [PATCH v2 00/16] fs,x86/resctrl: Add kernel-mode (e.g., PLZA) support to the resctrl subsystem
From: Reinette Chatre @ 2026-03-31 22:24 UTC (permalink / raw)
To: Babu Moger, corbet, tony.luck, Dave.Martin, james.morse, tglx,
mingo, bp, dave.hansen
Cc: skhan, x86, hpa, peterz, juri.lelli, vincent.guittot,
dietmar.eggemann, rostedt, bsegall, mgorman, vschneid, kas,
rick.p.edgecombe, akpm, pmladek, rdunlap, dapeng1.mi, kees, elver,
paulmck, lirongqing, safinaskar, fvdl, seanjc, pawan.kumar.gupta,
xin, tiala, Neeraj.Upadhyay, chang.seok.bae, thomas.lendacky,
elena.reshetova, linux-doc, linux-kernel, linux-coco, kvm,
eranian, peternewman
In-Reply-To: <30deeb5b-d2ec-4f85-aa4f-c21400df3486@amd.com>
Hi Babu,
On 3/30/26 11:46 AM, Babu Moger wrote:
> On 3/27/26 17:11, Reinette Chatre wrote:
>> On 3/26/26 10:12 AM, Babu Moger wrote:
>>> On 3/24/26 17:51, Reinette Chatre wrote:
>>>> On 3/12/26 1:36 PM, Babu Moger wrote:
>>>>> Tony suggested using global variables to store the kernel mode
>>>>> CLOSID and RMID. However, the kernel mode CLOSID and RMID are
>>>>> coming from rdtgroup structure with the new interface. Accessing
>>>>> them requires holding the associated lock, which would make the
>>>>> context switch path unnecessarily expensive. So, dropped the idea.
>>>>> https://lore.kernel.org/lkml/aXuxVSbk1GR2ttzF@agluck-desk3/
>>>>> Let me know if there are other ways to optimize this.
>>>> I do not see why the context switch path needs to be touched at all with this
>>>> implementation. Since PLZA only supports global assignment does it not mean that resctrl
>>>> only needs to update PQR_PLZA_ASSOC when user writes to info/kernel_mode and
>>>> info/kernel_mode_assignment?
>>> Each thread has an MSR to configure whether to associate privilege level zero execution with a separate COS and/or RMID, and the value of the COS and/or RMID. PLZA may be enabled or disabled on a per-thread basis. However, the COS and RMID association and configuration must be the same for all threads in the QOS Domain.
>> Based on previous comment in https://lore.kernel.org/lkml/abb049fa-3a3d-4601-9ae3-61eeb7fd8fcf@amd.com/
>> and this implementation all fields of PQR_PLZA_ASSOC except PQR_PLZA_ASSOC.plza_en must be the
>> same for all CPUs on the system, not just per QoS domain. Could you please confirm?
>
> Sorry for the confusion. It is "per QoS domain".
>
> All the fields of PQR_PLZA_ASSOC except PQR_PLZA_ASSOC.plza_enmust be set to the same value for all HW threads in the QOS domain for consistent operation (Per-QosDomain).
Thank you for clarifying. To build on this, what would be best way for resctrl to interpret this?
As I see it all values in PQR_PLZA_ASSOC apply to *all* resources yet (theoretically?) every resource
can have domains that span different CPUs. There thus seem to be a built in assumption of what a "domain"
means for PQR_PLZA_ASSOC so it sounds to me as though, instead of saying that "PQR_PLZA_ASSOC needs
to be the same in QoS domain" it may be more accurate to, for example, say that "PQR_PLZA_ASSOC has L3 scope"?
This seems to be what this implementation does since it hardcodes PQR_PLZA_ASSOC scope to the L3
resource but that creates dependency to the L3 resource that would make PLZA unusable if, for example,
the user boots with "rdt=!l3cat" while wanting to use PLZA to manage MBA allocations when in kernel?
...
> Yes, I agree with your concerns. The goal here is to make the interface less disruptive while still addressing the different use cases.
I consider changing resctrl behavior when values are written to existing resctrl files
to be disruptive. This is something we explicitly discussed during v1 as something to
be avoided so this implementation that overloads the tasks file again is unexpected.
> Background: Customers have identified an issue with the QoS
> Bandwidth Control feature: when a CLOS is aggressively throttled
> and execution transitions into kernel mode, kernel operations are
> also subject to the same aggressive throttling.
>
> > Privilege-Level Zero Association (PLZA) allows a user to specify a
> COS and/or RMID to be used during execution at Privilege Level Zero.
> When PLZA is enabled on a hardware thread, any execution that enters
> Privilege Level Zero will have its transactions associated with the
> PLZA COS and/or RMID. Otherwise, the thread continues to use the COS
> and RMID specified by |PQR_ASSOC|. In other words, the hardware
> provides a dedicated COS and/or RMID specifically for kernel-mode
> execution.
ack.
>
> There are multiple ways this feature can be applied. For simplicity, the discussion below focuses only on CLOSID.
>
>
> 1. Global PLZA enablement
>
> PLZA can be configured as a global feature by setting |PQR_PLZA_ASSOC.closid = CLOSID| and |PQR_PLZA_ASSOC.plza_en = 1| on all threads in the system. A dedicated CLOSID is reserved for this purpose,
Also discussed during v1 is that there is no need to dedicate a CLOSID for this purpose.
There could be an "unthrottled" CLOSID to which all high priority user space tasks as
well as all kernel work of all tasks are assigned.
If user space chooses to dedicate a CLOSID for kernel work then that should supported and
interface can allow that, but there is no need for resctrl to enforce this.
> and all CPU threads use its allocations whenever they enter Privilege Level Zero. This CLOSID does not need to be associated with any resctrl group.
The CLOSID has to be associated with a resource group to be able to manage its
resource allocations, no?
> The user can explicitly enable or disable this feature.
ack.
> There is no context switch overhead but there is no flexibility with this approach.
Flexibility is subjective. As I understand this supports the only use case we learned about so far:
https://lore.kernel.org/lkml/CABPqkBSq=cgn-am4qorA_VN0vsbpbfDePSi7gubicpROB1=djw@mail.gmail.com/
> 2. Group based PLZA allocation : PLZA is managed via dedicated
> restctrl group. A separate resctrl group can be created
> specifically for PLZA, with a dedicated CLOSID used exclusively
> for kernel mode execution. This approach can be further divided
> into two association models:
So far this sounds like global allocation since both need a dedicated resource group.
Whether this group is dedicated to kernel work or shared between kernel and user space work
is up to the user. There is no motivation why CLOSID should ever be enforced to be
exclusive for kernel mode execution.
>
> i) CPU based association
> CPUs are assigned to the PLZA group, and PLZA is enabled only on
> those CPUs. This effectively creates a dedicated PLZA group. MSRs (|
> PQR_PLZA_ASSOC)| are programmed only when the user changes CPU
> assignments. This approach requires no changes to the context switch
> code and introduces no additional context switch overhead.
>
> ii) Task based association
> Tasks are explicitly assigned by the user to the PLZA group. Tasks
> need to be updated when user adds a new task. Also, this requires
> updates during task scheduling so that the MSRs (|PQR_PLZA_ASSOC)|
> are programmed on each context switch, which introduces additional
> context switch overhead.
As discussed during v1 any changes needed to support per task assignment would
need to be done with new files dedicated to this purpose. Do not overload the
existing resctrl tasks/cpus/cpus_list files.
> I tried to fit these requirements into the interface files in /sys/
> fs/resctrl/info/. I may have missed few things while trying to
> achieve it. As usual, I am open for the discussion and
> recommendations.
Many of these items were already discussed as part of v1 so I think we may be
talking past each other here. I tried to highlight the relevant points raised
during v1 discussion that I thought there already was agreement on.
The one new aspect is that I assumed this implementation will only be for
global configuration and assignment. It looks like you want to support both
global configuration and per-task assignment. In the original I did not consider
configuration and assignment to occur at different scope so we may need to come up
with new modes to distinguish. Consider the addition of two modes as below:
# cat info/kernel_mode
[inherit_ctrl_and_mon]
global_assign_ctrl_inherit_mon_set_all
global_assign_ctrl_assign_mon_set_all
global_assign_ctrl_inherit_mon_set_individual
global_assign_ctrl_assign_mon_set_individual
Above introduces a "set_all" and "set_individual" suffix to the original two
modes.
global_assign_ctrl_inherit_mon_set_all
global_assign_ctrl_assign_mon_set_all:
Above are the original two modes but makes it clear that when this mode is
activated _all_ tasks run with the assignment.
global_assign_ctrl_inherit_mon_set_individual
global_assign_ctrl_assign_mon_set_individual:
Above are two new modes. In this mode user space also assigns a resource
group globally but then needs to follow that up by activating every task
separately to run with this assignment.
One way in which this can be accomplished could be to have "kernel_mode_tasks",
"kernel_mode_cpus", and "kernel_mode_cpus_list" files become visible (or be
created) in the resource group found in info/kernel_mode_assignment. User
space interacts with the new files to set which tasks and/or CPUs run with
PLZA enabled.
Even so, as I understand global_assign_ctrl_inherit_mon_set_all and
global_assign_ctrl_assign_mon_set_all addresses the only known use case. Do you know
if there are use cases for global_assign_ctrl_inherit_mon_set_individual and
global_assign_ctrl_assign_mon_set_individual? The latter two adds significant
complexity to resctrl while I have not heard about any use case for it.
Reinette
^ permalink raw reply
* Re: [PATCH v2 2/5] x86/virt/tdx: Pull kexec cache flush logic into arch/x86
From: Edgecombe, Rick P @ 2026-03-31 22:21 UTC (permalink / raw)
To: Verma, Vishal L, seanjc@google.com
Cc: Huang, Kai, bp@alien8.de, x86@kernel.org, kas@kernel.org,
hpa@zytor.com, mingo@redhat.com, linux-kernel@vger.kernel.org,
dave.hansen@linux.intel.com, tglx@kernel.org, pbonzini@redhat.com,
linux-coco@lists.linux.dev, kvm@vger.kernel.org
In-Reply-To: <acwe7kKyjfEQ0sIf@google.com>
On Tue, 2026-03-31 at 12:22 -0700, Sean Christopherson wrote:
> > -
> > -#ifdef CONFIG_KEXEC_CORE
> > -void tdx_cpu_flush_cache_for_kexec(void)
> > -{
> > - lockdep_assert_preemption_disabled();
>
> Is there a pre-existing bug here that gets propagate to tdx_shutdown_cpu()?
> When called from kvm_offline_cpu(), preemption won't be fully disabled, but
> per-CPU access are fine because the task is pinned to the target CPU.
>
> See https://lore.kernel.org/all/aUVx20ZRjOzKgKqy@google.com
Yes. And actually it got hit during development of this series. This patch will
conflict with the fix:
https://lore.kernel.org/lkml/20260312100009.924136-1-kai.huang@intel.com/
Oh, you acked it actually. But I was under the impression that after this patch
here, the splat wouldn't be triggered. So it inadvertently fixes it. But that
other patch is much more backport friendly.
^ permalink raw reply
* Re: [PATCH 2/2] x86/tdx: Fix zero-extension for 32-bit port I/O
From: Huang, Kai @ 2026-03-31 22:13 UTC (permalink / raw)
To: x86@kernel.org, mingo@redhat.com, kas@kernel.org, tglx@kernel.org,
bp@alien8.de, dave.hansen@linux.intel.com
Cc: Edgecombe, Rick P, hpa@zytor.com,
sathyanarayanan.kuppuswamy@linux.intel.com,
linux-kernel@vger.kernel.org, tsyrulnikov.borys@gmail.com,
kvm@vger.kernel.org, stable@vger.kernel.org,
linux-coco@lists.linux.dev
In-Reply-To: <20260331112430.71425-3-kas@kernel.org>
On Tue, 2026-03-31 at 12:24 +0100, Kiryl Shutsemau (Meta) wrote:
> According to x86 architecture rules, 32-bit operations zero-extend the
> result to 64 bits.
>
FWIW, the relevant part in the SDM seems to be:
Chapter 3.4.1.1 General-Purpose Registers in 64-Bit Mode
...
* 32-bit operands generate a 32-bit result, zero-extended to a 64 bit
result in the destination general-purpose register.
> The current implementation of handle_in() only masks
> the lower 32 bits, which preserves the upper 32 bits of RAX when a
> 32-bit port IN instruction is emulated.
>
> Update handle_in() to zero out the entire RAX register when the I/O size
> is 4 bytes to ensure correct zero-extension. For smaller sizes (1 or 2
> bytes), continue to preserve the unaffected upper bits.
>
> Fixes: 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")
> Reported-by: Borys Tsyrulnikov <tsyrulnikov.borys@gmail.com>
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> Cc: stable@vger.kernel.org
Reviewed-by: Kai Huang <kai.huang@intel.com>
> ---
> arch/x86/coco/tdx/tdx.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 4d7f71d50122..b9b9a2d75119 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -703,8 +703,17 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
> */
> success = !__tdx_hypercall(&args);
>
> - /* Update part of the register affected by the emulated instruction */
> - regs->ax &= ~mask;
> + /*
> + * Update part of the register affected by the emulated instruction.
> + *
> + * 32-bit operands generate a 32-bit result, zero-extended to a 64-bit
> + * result.
> + */
> + if (size < 4)
> + regs->ax &= ~mask;
> + else
> + regs->ax = 0;
> +
> if (success)
> regs->ax |= args.r11 & mask;
>
^ permalink raw reply
* Re: [PATCH 2/2] x86/tdx: Fix zero-extension for 32-bit port I/O
From: Kuppuswamy Sathyanarayanan @ 2026-03-31 22:10 UTC (permalink / raw)
To: Kiryl Shutsemau (Meta), Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86
Cc: H . Peter Anvin, Rick Edgecombe, Borys Tsyrulnikov, linux-kernel,
linux-coco, kvm, stable
In-Reply-To: <20260331112430.71425-3-kas@kernel.org>
Hi Kiril,
On 3/31/2026 4:24 AM, Kiryl Shutsemau (Meta) wrote:
> According to x86 architecture rules, 32-bit operations zero-extend the
> result to 64 bits. The current implementation of handle_in() only masks
> the lower 32 bits, which preserves the upper 32 bits of RAX when a
> 32-bit port IN instruction is emulated.
>
> Update handle_in() to zero out the entire RAX register when the I/O size
> is 4 bytes to ensure correct zero-extension. For smaller sizes (1 or 2
> bytes), continue to preserve the unaffected upper bits.
>
> Fixes: 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")
> Reported-by: Borys Tsyrulnikov <tsyrulnikov.borys@gmail.com>
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> Cc: stable@vger.kernel.org
> ---
If you have bug or discussion link, please include it.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> arch/x86/coco/tdx/tdx.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 4d7f71d50122..b9b9a2d75119 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -703,8 +703,17 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
> */
> success = !__tdx_hypercall(&args);
>
> - /* Update part of the register affected by the emulated instruction */
> - regs->ax &= ~mask;
> + /*
> + * Update part of the register affected by the emulated instruction.
> + *
> + * 32-bit operands generate a 32-bit result, zero-extended to a 64-bit
> + * result.
> + */
> + if (size < 4)
> + regs->ax &= ~mask;
> + else
> + regs->ax = 0;
The logic would be more readable as:
if (size == 4)
regs->ax = 0;
else
regs->ax &= ~mask;
> +
> if (success)
> regs->ax |= args.r11 & mask;
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply
* Re: [PATCH 1/2] x86/tdx: Fix off-by-one in port I/O handling
From: Huang, Kai @ 2026-03-31 22:06 UTC (permalink / raw)
To: x86@kernel.org, mingo@redhat.com, kas@kernel.org, tglx@kernel.org,
bp@alien8.de, dave.hansen@linux.intel.com
Cc: Edgecombe, Rick P, hpa@zytor.com,
sathyanarayanan.kuppuswamy@linux.intel.com,
linux-kernel@vger.kernel.org, tsyrulnikov.borys@gmail.com,
kvm@vger.kernel.org, stable@vger.kernel.org,
linux-coco@lists.linux.dev
In-Reply-To: <20260331112430.71425-2-kas@kernel.org>
On Tue, 2026-03-31 at 12:24 +0100, Kiryl Shutsemau (Meta) wrote:
> handle_in() and handle_out() in arch/x86/coco/tdx/tdx.c use:
>
> u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
>
> GENMASK(h, l) includes bit h. For size=1 (INB), this produces
> GENMASK(8, 0) = 0x1FF (9 bits) instead of GENMASK(7, 0) = 0xFF (8
> bits). The mask is one bit too wide for all I/O sizes.
>
> Fix the mask calculation.
>
> Fixes: 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")
> Reported-by: Borys Tsyrulnikov <tsyrulnikov.borys@gmail.com>
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> Cc: stable@vger.kernel.org
Reviewed-by: Kai Huang <kai.huang@intel.com>
^ permalink raw reply
* Re: [PATCH 1/2] x86/tdx: Fix off-by-one in port I/O handling
From: Kuppuswamy Sathyanarayanan @ 2026-03-31 21:57 UTC (permalink / raw)
To: Kiryl Shutsemau (Meta), Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86
Cc: H . Peter Anvin, Rick Edgecombe, Borys Tsyrulnikov, linux-kernel,
linux-coco, kvm, stable
In-Reply-To: <20260331112430.71425-2-kas@kernel.org>
Hi Kirill,
On 3/31/2026 4:24 AM, Kiryl Shutsemau (Meta) wrote:
> handle_in() and handle_out() in arch/x86/coco/tdx/tdx.c use:
>
> u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
>
> GENMASK(h, l) includes bit h. For size=1 (INB), this produces
> GENMASK(8, 0) = 0x1FF (9 bits) instead of GENMASK(7, 0) = 0xFF (8
> bits). The mask is one bit too wide for all I/O sizes.
>
> Fix the mask calculation.
>
> Fixes: 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")
> Reported-by: Borys Tsyrulnikov <tsyrulnikov.borys@gmail.com>
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> Cc: stable@vger.kernel.org
> ---
LGTM. Can you include a link to the bug report or related discussion in
the commit log? It will help understand the impact of this issue.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> arch/x86/coco/tdx/tdx.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 7b2833705d47..4d7f71d50122 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -693,7 +693,7 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
> .r13 = PORT_READ,
> .r14 = port,
> };
> - u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
> + u64 mask = GENMASK(BITS_PER_BYTE * size - 1, 0);
> bool success;
>
> /*
> @@ -713,7 +713,7 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
>
> static bool handle_out(struct pt_regs *regs, int size, int port)
> {
> - u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
> + u64 mask = GENMASK(BITS_PER_BYTE * size - 1, 0);
>
> /*
> * Emulate the I/O write via hypercall. More info about ABI can be found
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply
* Re: [PATCH v2 1/5] x86/tdx: Move all TDX error defines into <asm/shared/tdx_errno.h>
From: Edgecombe, Rick P @ 2026-03-31 21:46 UTC (permalink / raw)
To: Verma, Vishal L, seanjc@google.com
Cc: bp@alien8.de, x86@kernel.org, kas@kernel.org, hpa@zytor.com,
mingo@redhat.com, linux-kernel@vger.kernel.org,
dave.hansen@linux.intel.com, tglx@kernel.org, pbonzini@redhat.com,
linux-coco@lists.linux.dev, kvm@vger.kernel.org
In-Reply-To: <acwgvBzK01IrAAeU@google.com>
On Tue, 2026-03-31 at 12:30 -0700, Sean Christopherson wrote:
> +#define TDX_SW_ERROR (TDX_ERROR | GENMASK_ULL(47, 40))
> > +#define TDX_SEAMCALL_VMFAILINVALID (TDX_SW_ERROR | _ULL(0xFFFF0000))
> > +
> > +#define TDX_SEAMCALL_GP (TDX_SW_ERROR | X86_TRAP_GP)
> > +#define TDX_SEAMCALL_UD (TDX_SW_ERROR | X86_TRAP_UD)
>
> I don't think the host's SW-defined error codes should be used by the guest. The
> guest can't even make SEAMCALLs. So unless I'm misunderstanding the purpose, I
> don't think it makes sense to move these into tdx_errno.h.
Seems reasonable.
>
> Regardless, please split this up into two patches:
>
> 1. Move tdx_errno.h
> 2. Land more #defines in tdx_errno.h
>
> Because IIUC, tdx_errno.h holds *only* architecturally defined values, which makes
> (1) super duper trivial to review and ack.
Thanks!
^ permalink raw reply
* Re: [PATCH v2 3/5] x86/virt/tdx: Add SEAMCALL wrapper for TDH.SYS.DISABLE
From: Edgecombe, Rick P @ 2026-03-31 21:36 UTC (permalink / raw)
To: Verma, Vishal L, kas@kernel.org
Cc: seanjc@google.com, bp@alien8.de, x86@kernel.org, hpa@zytor.com,
mingo@redhat.com, linux-kernel@vger.kernel.org,
dave.hansen@linux.intel.com, tglx@kernel.org, pbonzini@redhat.com,
linux-coco@lists.linux.dev, kvm@vger.kernel.org
In-Reply-To: <b9aa84d76c131132b5268712a44200e804c31aeb.camel@intel.com>
On Tue, 2026-03-31 at 18:22 +0000, Verma, Vishal L wrote:
> >
> > I guess the actual behaviour is dependant on the return code. It is
> > obviously going to be the case for TDX_SUCCESS. And from the discussion,
> > I guess that's true for TDX_SYS_BUSY and TDX_INTERRUPTED_RESUMABLE.
> >
> > What about other cases? The spec draft also lists TDX_SYS_NOT_READY and
> > TDX_SYS_SHUTDOWN.
>
> I think these are safe too - TDX_SYS_SHUTDOWN means the module has
> already been shutdown, which this seamcall would've done, so things
> should be in the same state either way.
>
> TDX_SYS_NOT_READY means the module hasn't been initialized yet. This
> seamcall should just exit, and the module is already blocking any
> seamcall that need the module to be initialized. The seamcalls to
> initialize the module will be allowed, as they are after a sys_disable
> call anyway.
Should the seamcall return success in the case where it would return
TDX_SYS_NOT_READY? It is in basically a reset state right? The errors we care
about are actual errors (TDX_SW_ERROR), so it makes no difference to the code in
the patch. But it might be a nicer API for the seamcall?
>
> >
> > I wounder if it can affect the kernel. Consider the case when kexec
> > (crash kernel start) happens due to crash on TDX module.
> >
> > Will we be able to shutdown TDX module cleanly and make kexec safe?
>
> Hm -are the semantics for what happens if there is a crash in the
> module defined? I think Linux should expect that sys_disable should
> either start doing its shutdown work, or exit with one of the other
> defined exit statuses. Anything else would be considered a module bug.
We often have the question come up about how much we should to guard against
bugs in the TDX module. I tend to also think we should not do defensive
programming, same as we do for the kernel. If it's easy to handle something or
emit a warning it's nice, but otherwise the solution for such cases should be to
fix the TDX module bug.
But for the kdump case, we don't actually need sys disable to succeed. The kdump
kernel will not load the TDX module. And as for the errata, this already needs a
special situation to be a problem. But even if it happens, I'd think better to
try to the kdump. Not sure what the fix would be for that scenario, even if we
allowed for a large complexity budget. So best effort seems good.
Does it seem reasonable?
^ permalink raw reply
* Re: [PATCH v2 1/5] x86/tdx: Move all TDX error defines into <asm/shared/tdx_errno.h>
From: Sean Christopherson @ 2026-03-31 19:30 UTC (permalink / raw)
To: Vishal Verma
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Kiryl Shutsemau, Rick Edgecombe, Paolo Bonzini,
linux-kernel, linux-coco, kvm
In-Reply-To: <20260323-fuller_tdx_kexec_support-v2-1-87a36409e051@intel.com>
On Mon, Mar 23, 2026, Vishal Verma wrote:
> diff --git a/arch/x86/kvm/vmx/tdx_errno.h b/arch/x86/include/asm/shared/tdx_errno.h
> similarity index 64%
> rename from arch/x86/kvm/vmx/tdx_errno.h
> rename to arch/x86/include/asm/shared/tdx_errno.h
> index 6ff4672c4181..8bf6765cf082 100644
> --- a/arch/x86/kvm/vmx/tdx_errno.h
> +++ b/arch/x86/include/asm/shared/tdx_errno.h
> @@ -1,14 +1,15 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> -/* architectural status code for SEAMCALL */
> -
> -#ifndef __KVM_X86_TDX_ERRNO_H
> -#define __KVM_X86_TDX_ERRNO_H
> +#ifndef _ASM_X86_SHARED_TDX_ERRNO_H
> +#define _ASM_X86_SHARED_TDX_ERRNO_H
> +#include <asm/trapnr.h>
>
> +/* Upper 32 bit of the TDX error code encodes the status */
> #define TDX_SEAMCALL_STATUS_MASK 0xFFFFFFFF00000000ULL
>
> /*
> - * TDX SEAMCALL Status Codes (returned in RAX)
> + * TDX Status Codes (returned in RAX)
> */
> +#define TDX_SUCCESS 0ULL
> #define TDX_NON_RECOVERABLE_VCPU 0x4000000100000000ULL
> #define TDX_NON_RECOVERABLE_TD 0x4000000200000000ULL
> #define TDX_NON_RECOVERABLE_TD_NON_ACCESSIBLE 0x6000000500000000ULL
> @@ -17,6 +18,7 @@
> #define TDX_OPERAND_INVALID 0xC000010000000000ULL
> #define TDX_OPERAND_BUSY 0x8000020000000000ULL
> #define TDX_PREVIOUS_TLB_EPOCH_BUSY 0x8000020100000000ULL
> +#define TDX_RND_NO_ENTROPY 0x8000020300000000ULL
> #define TDX_PAGE_METADATA_INCORRECT 0xC000030000000000ULL
> #define TDX_VCPU_NOT_ASSOCIATED 0x8000070200000000ULL
> #define TDX_KEY_GENERATION_FAILED 0x8000080000000000ULL
> @@ -28,6 +30,20 @@
> #define TDX_EPT_ENTRY_STATE_INCORRECT 0xC0000B0D00000000ULL
> #define TDX_METADATA_FIELD_NOT_READABLE 0xC0000C0200000000ULL
>
> +/*
> + * SW-defined error codes.
> + *
> + * Bits 47:40 == 0xFF indicate Reserved status code class that never used by
> + * TDX module.
> + */
> +#define TDX_ERROR _BITULL(63)
> +#define TDX_NON_RECOVERABLE _BITULL(62)
> +#define TDX_SW_ERROR (TDX_ERROR | GENMASK_ULL(47, 40))
> +#define TDX_SEAMCALL_VMFAILINVALID (TDX_SW_ERROR | _ULL(0xFFFF0000))
> +
> +#define TDX_SEAMCALL_GP (TDX_SW_ERROR | X86_TRAP_GP)
> +#define TDX_SEAMCALL_UD (TDX_SW_ERROR | X86_TRAP_UD)
I don't think the host's SW-defined error codes should be used by the guest. The
guest can't even make SEAMCALLs. So unless I'm misunderstanding the purpose, I
don't think it makes sense to move these into tdx_errno.h.
Regardless, please split this up into two patches:
1. Move tdx_errno.h
2. Land more #defines in tdx_errno.h
Because IIUC, tdx_errno.h holds *only* architecturally defined values, which makes
(1) super duper trivial to review and ack.
^ permalink raw reply
* Re: [PATCH v2 2/5] x86/virt/tdx: Pull kexec cache flush logic into arch/x86
From: Sean Christopherson @ 2026-03-31 19:22 UTC (permalink / raw)
To: Vishal Verma
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Kiryl Shutsemau, Rick Edgecombe, Paolo Bonzini,
linux-kernel, linux-coco, kvm, Kai Huang
In-Reply-To: <20260323-fuller_tdx_kexec_support-v2-2-87a36409e051@intel.com>
On Mon, Mar 23, 2026, Vishal Verma wrote:
> From: Rick Edgecombe <rick.p.edgecombe@intel.com>
>
> KVM tries to take care of some required cache flushing earlier in the
> kexec path in order to be kind to some long standing races that can occur
> later in the operation. Until recently, VMXOFF was handled within KVM.
> Since VMX being enabled is required to make a SEAMCALL, it had the best
> per-cpu scoped operation to plug the flushing into. So it is kicked off
> from there.
>
> This early kexec cache flushing in KVM happens via a syscore shutdown
> callback. Now that VMX enablement control has moved to arch/x86, which has
> grown its own syscore shutdown callback, it no longer make sense for it to
> live in KVM. It fits better with the TDX enablement managing code.
>
> In addition, future changes will add a SEAMCALL that happens immediately
> before VMXOFF, which means the cache flush in KVM will be too late to
> flush the cache before the last SEAMCALL. So move it to the newly added TDX
> arch/x86 syscore shutdown handler.
>
> Since tdx_cpu_flush_cache_for_kexec() is no longer needed by KVM, make it
> static and remove the export. Since it is also not part of an operation
> spread across disparate components, remove the redundant comments and
> verbose naming.
>
> In the existing KVM based code, CPU offline also funnels through
> tdx_cpu_flush_cache_for_kexec(). So the centralization to the arch/x86
> syscore shutdown callback elides this CPU offline time behavior. However,
> WBINVD is already generally done at CPU offline as matter of course. So
> don't bother adding TDX specific logic for this, and rely on the normal
> WBINVD to handle it.
>
> Acked-by: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
Ingoring the potential fixup needed for the existing bug...
Acked-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/include/asm/tdx.h | 6 ------
> arch/x86/kvm/vmx/tdx.c | 10 ----------
> arch/x86/virt/vmx/tdx/tdx.c | 39 ++++++++++++++++++++-------------------
> 3 files changed, 20 insertions(+), 35 deletions(-)
>
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 2917b3451491..7674fc530090 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -205,11 +205,5 @@ static inline const char *tdx_dump_mce_info(struct mce *m) { return NULL; }
> static inline const struct tdx_sys_info *tdx_get_sysinfo(void) { return NULL; }
> #endif /* CONFIG_INTEL_TDX_HOST */
>
> -#ifdef CONFIG_KEXEC_CORE
> -void tdx_cpu_flush_cache_for_kexec(void);
> -#else
> -static inline void tdx_cpu_flush_cache_for_kexec(void) { }
> -#endif
> -
> #endif /* !__ASSEMBLER__ */
> #endif /* _ASM_X86_TDX_H */
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index b7264b533feb..50a5cfdbd33e 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -440,16 +440,6 @@ void tdx_disable_virtualization_cpu(void)
> tdx_flush_vp(&arg);
> }
> local_irq_restore(flags);
> -
> - /*
> - * Flush cache now if kexec is possible: this is necessary to avoid
> - * having dirty private memory cachelines when the new kernel boots,
> - * but WBINVD is a relatively expensive operation and doing it during
> - * kexec can exacerbate races in native_stop_other_cpus(). Do it
> - * now, since this is a safe moment and there is going to be no more
> - * TDX activity on this CPU from this point on.
> - */
> - tdx_cpu_flush_cache_for_kexec();
> }
>
> #define TDX_SEAMCALL_RETRIES 10000
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index cb9b3210ab71..0802d0fd18a4 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -224,8 +224,28 @@ static int tdx_offline_cpu(unsigned int cpu)
> return 0;
> }
>
> +static void tdx_cpu_flush_cache(void)
> +{
> + lockdep_assert_preemption_disabled();
> +
> + if (!this_cpu_read(cache_state_incoherent))
> + return;
> +
> + wbinvd();
> + this_cpu_write(cache_state_incoherent, false);
> +}
> +
> static void tdx_shutdown_cpu(void *ign)
> {
> + /*
> + * Flush cache now if kexec is possible: this is necessary to avoid
> + * having dirty private memory cachelines when the new kernel boots,
> + * but WBINVD is a relatively expensive operation and doing it during
> + * kexec can exacerbate races in native_stop_other_cpus(). Do it
> + * now, since this is a safe moment and there is going to be no more
> + * TDX activity on this CPU from this point on.
> + */
> + tdx_cpu_flush_cache();
> x86_virt_put_ref(X86_FEATURE_VMX);
> }
>
> @@ -1920,22 +1940,3 @@ u64 tdh_phymem_page_wbinvd_hkid(u64 hkid, struct page *page)
> return seamcall(TDH_PHYMEM_PAGE_WBINVD, &args);
> }
> EXPORT_SYMBOL_FOR_KVM(tdh_phymem_page_wbinvd_hkid);
> -
> -#ifdef CONFIG_KEXEC_CORE
> -void tdx_cpu_flush_cache_for_kexec(void)
> -{
> - lockdep_assert_preemption_disabled();
Is there a pre-existing bug here that gets propagate to tdx_shutdown_cpu()? When
called from kvm_offline_cpu(), preemption won't be fully disabled, but per-CPU
access are fine because the task is pinned to the target CPU.
See https://lore.kernel.org/all/aUVx20ZRjOzKgKqy@google.com
> -
> - if (!this_cpu_read(cache_state_incoherent))
> - return;
> -
> - /*
> - * Private memory cachelines need to be clean at the time of
> - * kexec. Write them back now, as the caller promises that
> - * there should be no more SEAMCALLs on this CPU.
> - */
> - wbinvd();
> - this_cpu_write(cache_state_incoherent, false);
> -}
> -EXPORT_SYMBOL_FOR_KVM(tdx_cpu_flush_cache_for_kexec);
> -#endif
>
> --
> 2.53.0
>
^ permalink raw reply
* Re: [PATCH 1/2] x86/virt/tdx: Use PFN directly for mapping guest private memory
From: Sean Christopherson @ 2026-03-31 19:13 UTC (permalink / raw)
To: Yan Zhao
Cc: Rick P Edgecombe, Dave Hansen, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Kai Huang, Xiaoyao Li,
dave.hansen@linux.intel.com, kas@kernel.org, mingo@redhat.com,
pbonzini@redhat.com, binbin.wu@linux.intel.com,
ackerleytng@google.com, linux-kernel@vger.kernel.org,
Isaku Yamahata, sagis@google.com, Vishal Annapurve, bp@alien8.de,
tglx@kernel.org, yilun.xu@linux.intel.com, x86@kernel.org
In-Reply-To: <acYrxIxoENyZhKCV@yzhao56-desk.sh.intel.com>
On Fri, Mar 27, 2026, Yan Zhao wrote:
> On Thu, Mar 26, 2026 at 12:57:26AM +0800, Edgecombe, Rick P wrote:
> > On Wed, 2026-03-25 at 17:10 +0800, Yan Zhao wrote:
> > > > I don't really understand what this is saying.
> > > >
> > > > Is the concern that KVM might want to set up page tables for memory
> > > > that differ from how it was allocated? I'm a bit worried that this
> > > > assumes something about folios that doesn't always hold.
> > > >
> > > > I think the hugetlbfs gigantic support uses folios in at least a
> > > > few spots today.
> > > Below is the background of this problem. I'll try to include a short
> > > summary in the next version's patch logs.
> >
> > While this patchset is kind of pre-work for TDX huge pages, the reason
> > to separate it out and push it earlier is because it has some value on
> > it's own. So I'd think to focus mostly on the impact of the change
> > today.
> >
> > How about this justification:
> > 1. Because KVM handles guest memory as PFNs, and the SEAMCALLs under
> > discussion are only used there, PFN is more natural.
> >
> > 2. The struct page was partly making sure we didn't pass a wrong arg
> > (typical type safety) and partly ensuring that KVM doesn't pass non-
> > convertible memory, however the SEAMCALLs themselves can check this for
> > the kernel. So the case is already covered by warnings.
> >
> > In conclusion, the PFN is more natural and the original purpose of
> > struct page is already covered.
Most importantly, having core TDX make assumptions based on the struct page and/or
folio will create subtle dependencies that are easily avoided.
> > Sean said somewhere IIRC that he would have NAKed the struct page thing
> > if he had seen it, for even the base support.
Yes.
> > And the two points above don't actually require discussion of even huge
> > pages. So does it actually add any value to dive into the issues you list
> > below?
> I wanted to mention the issues listed below because I'm not sure if anyone has
> the same question as me: why do we have to convert struct page to PFN if they
> can both achieve the same purpose, given that currently all private memory
> allocated by gmem has struct page backing?
From https://lore.kernel.org/all/aWgyhmTJphGQqO0Y@google.com:
: I'm not at all opposed to backing guest_memfd with "struct page", quite the
: opposite. What I don't want is to bake assumptions into KVM code that doesn't
: _require_ struct page, because that has cause KVM immense pain in the past.
:
: And I'm strongly opposed to KVM special-casing TDX or anything else, precisely
: because we struggled through all that pain so that KVM would work better with
: memory that isn't backed by "struct page", or more specifically, memory that has
: an associated "struct page", but isn't managed by core MM, e.g. isn't refcounted.
^ permalink raw reply
* Re: [PATCH v2 3/5] x86/virt/tdx: Add SEAMCALL wrapper for TDH.SYS.DISABLE
From: Verma, Vishal L @ 2026-03-31 18:22 UTC (permalink / raw)
To: kas@kernel.org, Edgecombe, Rick P
Cc: seanjc@google.com, bp@alien8.de, dave.hansen@linux.intel.com,
hpa@zytor.com, mingo@redhat.com, linux-kernel@vger.kernel.org,
x86@kernel.org, tglx@kernel.org, pbonzini@redhat.com,
linux-coco@lists.linux.dev, kvm@vger.kernel.org
In-Reply-To: <acu4raBTjdFcfVlS@thinkstation>
On Tue, 2026-03-31 at 13:18 +0100, Kiryl Shutsemau wrote:
> On Mon, Mar 30, 2026 at 07:25:22PM +0000, Edgecombe, Rick P wrote:
> > > I assumed that if the SEAMCALL fails other SEAMCALLs suppose to be
> > > functional. Hm?
> >
> > The behavior should be that once you make this seamcall (assuming it's
> > supported) that no other seamcalls can be made. They will return an
> > error. Do you think something else would be better? If it's an old TDX
> > module, nothing happens of course.
>
> I guess the actual behaviour is dependant on the return code. It is
> obviously going to be the case for TDX_SUCCESS. And from the discussion,
> I guess that's true for TDX_SYS_BUSY and TDX_INTERRUPTED_RESUMABLE.
>
> What about other cases? The spec draft also lists TDX_SYS_NOT_READY and
> TDX_SYS_SHUTDOWN.
I think these are safe too - TDX_SYS_SHUTDOWN means the module has
already been shutdown, which this seamcall would've done, so things
should be in the same state either way.
TDX_SYS_NOT_READY means the module hasn't been initialized yet. This
seamcall should just exit, and the module is already blocking any
seamcall that need the module to be initialized. The seamcalls to
initialize the module will be allowed, as they are after a sys_disable
call anyway.
>
> I wounder if it can affect the kernel. Consider the case when kexec
> (crash kernel start) happens due to crash on TDX module.
>
> Will we be able to shutdown TDX module cleanly and make kexec safe?
Hm -are the semantics for what happens if there is a crash in the
module defined? I think Linux should expect that sys_disable should
either start doing its shutdown work, or exit with one of the other
defined exit statuses. Anything else would be considered a module bug.
^ permalink raw reply
* Re: [PATCH v13 10/48] arm64: RMI: Ensure that the RMM has GPT entries for memory
From: Mathieu Poirier @ 2026-03-31 17:43 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: Steven Price, kvm, kvmarm, Catalin Marinas, Marc Zyngier,
Will Deacon, James Morse, Oliver Upton, Zenghui Yu,
linux-arm-kernel, linux-kernel, Joey Gouly, Alexandru Elisei,
Christoffer Dall, Fuad Tabba, linux-coco, Ganapatrao Kulkarni,
Gavin Shan, Shanker Donthineni, Alper Gun, Aneesh Kumar K . V,
Emi Kisanuki, Vishal Annapurve
In-Reply-To: <152f5070-fda6-4381-bc40-4a70908c27c1@arm.com>
On Tue, Mar 31, 2026 at 12:05:47PM +0100, Suzuki K Poulose wrote:
> Hi Mathieu,
>
> On 30/03/2026 21:58, Mathieu Poirier wrote:
> > Hi,
> >
> > On Wed, Mar 18, 2026 at 03:53:34PM +0000, Steven Price wrote:
> > > The RMM may not be tracking all the memory of the system at boot. Create
> > > the necessary tracking state and GPTs within the RMM so that all boot
> > > memory can be delegated to the RMM as needed during runtime.
> > >
> > > Note: support is currently missing for SROs which means that if the RMM
> > > needs memory donating this will fail (and render CCA unusable in Linux).
> > >
> > > Signed-off-by: Steven Price <steven.price@arm.com>
> > > ---
> > > New patch for v13
> > > ---
> > > arch/arm64/kvm/rmi.c | 89 ++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 89 insertions(+)
> > >
> > > diff --git a/arch/arm64/kvm/rmi.c b/arch/arm64/kvm/rmi.c
> > > index 9590dff9a2c1..80aedc85e94a 100644
> > > --- a/arch/arm64/kvm/rmi.c
> > > +++ b/arch/arm64/kvm/rmi.c
> > > @@ -4,6 +4,7 @@
> > > */
> > > #include <linux/kvm_host.h>
> > > +#include <linux/memblock.h>
> > > #include <asm/kvm_pgtable.h>
> > > #include <asm/rmi_cmds.h>
> > > @@ -56,6 +57,18 @@ static int rmi_check_version(void)
> > > return 0;
> > > }
> > > +/*
> > > + * These are the 'default' sizes when passing 0 as the tracking_region_size.
> > > + * TODO: Support other granule sizes
> > > + */
> > > +#ifdef CONFIG_PAGE_SIZE_4KB
> > > +#define RMM_GRANULE_TRACKING_SIZE SZ_1G
> > > +#elif defined(CONFIG_PAGE_SIZE_16KB)
> > > +#define RMM_GRANULE_TRACKING_SIZE SZ_32M
> > > +#elif defined(CONFIG_PAGE_SIZE_64KB)
> > > +#define RMM_GRANULE_TRACKING_SIZE SZ_512M
> > > +#endif
> > > +
> > > static int rmi_configure(void)
> > > {
> > > struct rmm_config *config __free(free_page) = NULL;
> > > @@ -95,6 +108,80 @@ static int rmi_configure(void)
> > > return 0;
> > > }
> > > +static int rmi_verify_memory_tracking(phys_addr_t start, phys_addr_t end)
> > > +{
> > > + start = ALIGN_DOWN(start, RMM_GRANULE_TRACKING_SIZE);
> >
> > This will produce an error on systems where the start of system memory is not
> > aligned to RMM_GRANULE_TRACKING_SIZE. For instance, on QEMU-SBSA the system
> > memory starts at 0x100_4300_0000. With the above and RMM_GRANULE_TRACKING_SIZE
> > set to SZ_1G, @start becomes 0x100_4000_0000, which falls outside the memory map
> > known to the TF-A. I fixed it with these modifications:
>
> Thanks for raising this. This would need to be addressed in the RMM
> spec, I have raised it with the team and will be addressed soon.
>
> >
> > LINUX:
> >
> > diff --git a/arch/arm64/kvm/rmi.c b/arch/arm64/kvm/rmi.c
> > index 10ff1c3bddaf..21bfbbe2f047 100644
> > --- a/arch/arm64/kvm/rmi.c
> > +++ b/arch/arm64/kvm/rmi.c
> > @@ -424,7 +424,9 @@ static int rmi_configure(void)
> > static int rmi_verify_memory_tracking(phys_addr_t start, phys_addr_t end)
> > {
> > - start = ALIGN_DOWN(start, RMM_GRANULE_TRACKING_SIZE);
> > + phys_addr_t offset;
> > +
> > + offset = start - ALIGN_DOWN(start, RMM_GRANULE_TRACKING_SIZE);
> > end = ALIGN(end, RMM_GRANULE_TRACKING_SIZE);
> > while (start < end) {
> > @@ -439,7 +441,13 @@ static int rmi_verify_memory_tracking(phys_addr_t start, phys_addr_t end)
> > start);
> > return -ENODEV;
> > }
> > - start += RMM_GRANULE_TRACKING_SIZE;
> > +
> > + if (offset) {
> > + start += (RMM_GRANULE_TRACKING_SIZE - offset);
> > + offset = 0;
> > + } else {
> > + start += RMM_GRANULE_TRACKING_SIZE;
> > + }
> > }
> > return 0;
> >
> > RMM:
> >
> > diff --git a/runtime/rmi/granule.c b/runtime/rmi/granule.c
> > index cef521fc0869..60358d9ee81e 100644
> > --- a/runtime/rmi/granule.c
> > +++ b/runtime/rmi/granule.c
> > @@ -209,9 +209,11 @@ void smc_granule_tracking_get(unsigned long addr,
> > return;
> > }
> > +#if 0
> > if (!ALIGNED(addr, RMM_INTERNAL_TRACKING_REGION_SIZE)) {
> > return;
> > }
> > +#endif
> > g = find_granule(addr);
> > if (g != NULL) {
> >
> > This is likely not the right fix but hopefully provides some guidance. Send me
> > your patches when you have an idea and I'll test them.
>
> We will send you the update once it is fixed in the RMM spec. The rough idea
> is to remove the ALIGNMENT restrictions and return a Range that
> the host can iterate over to find "regions" with the same type of
> memory.
>
Ok, thanks for looking into this.
>
> Cheers
> Suzuki
>
>
> >
> > Thanks,
> > Mathieu
> >
> >
> > > + end = ALIGN(end, RMM_GRANULE_TRACKING_SIZE);
> > > +
> > > + while (start < end) {
> > > + unsigned long ret, category, state;
> > > +
> > > + ret = rmi_granule_tracking_get(start, &category, &state);
> > > + if (ret != RMI_SUCCESS ||
> > > + state != RMI_TRACKING_FINE ||
> > > + category != RMI_MEM_CATEGORY_CONVENTIONAL) {
> > > + /* TODO: Set granule tracking in this case */
> > > + kvm_err("Granule tracking for region isn't fine/conventional: %llx",
> > > + start);
> > > + return -ENODEV;
> > > + }
> > > + start += RMM_GRANULE_TRACKING_SIZE;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static unsigned long rmi_l0gpt_size(void)
> > > +{
> > > + return 1UL << (30 + FIELD_GET(RMI_FEATURE_REGISTER_1_L0GPTSZ,
> > > + rmm_feat_reg1));
> > > +}
> > > +
> > > +static int rmi_create_gpts(phys_addr_t start, phys_addr_t end)
> > > +{
> > > + unsigned long l0gpt_sz = rmi_l0gpt_size();
> > > +
> > > + start = ALIGN_DOWN(start, l0gpt_sz);
> > > + end = ALIGN(end, l0gpt_sz);
> > > +
> > > + while (start < end) {
> > > + int ret = rmi_gpt_l1_create(start);
> > > +
> > > + if (ret && ret != RMI_ERROR_GPT) {
> > > + /*
> > > + * FIXME: Handle SRO so that memory can be donated for
> > > + * the tables.
> > > + */
> > > + kvm_err("GPT Level1 table missing for %llx\n", start);
> > > + return -ENOMEM;
> > > + }
> > > + start += l0gpt_sz;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int rmi_init_metadata(void)
> > > +{
> > > + phys_addr_t start, end;
> > > + const struct memblock_region *r;
> > > +
> > > + for_each_mem_region(r) {
> > > + int ret;
> > > +
> > > + start = memblock_region_memory_base_pfn(r) << PAGE_SHIFT;
> > > + end = memblock_region_memory_end_pfn(r) << PAGE_SHIFT;
> > > + ret = rmi_verify_memory_tracking(start, end);
> > > + if (ret)
> > > + return ret;
> > > + ret = rmi_create_gpts(start, end);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int rmm_check_features(void)
> > > {
> > > if (kvm_lpa2_is_enabled() && !rmi_has_feature(RMI_FEATURE_REGISTER_0_LPA2)) {
> > > @@ -120,6 +207,8 @@ void kvm_init_rmi(void)
> > > return;
> > > if (rmi_configure())
> > > return;
> > > + if (rmi_init_metadata())
> > > + return;
> > > /* Future patch will enable static branch kvm_rmi_is_available */
> > > }
> > > --
> > > 2.43.0
> > >
> > >
>
^ permalink raw reply
* SVSM Development Call April 1st, 2026
From: Jörg Rödel @ 2026-03-31 16:22 UTC (permalink / raw)
To: coconut-svsm, linux-coco
Hi,
Here is the call for agenda items for this weeks SVSM development call. Please
send any agenda items you have in mind as a reply to this email or raise them
in the meeting.
We will use the LF Zoom instance. Details of the meeting can be found in our
governance repository at:
https://github.com/coconut-svsm/governance
The link to the COCONUT-SVSM calendar is:
https://zoom-lfx.platform.linuxfoundation.org/meetings/coconut-svsm?view=week
The meeting will be recorded and the recording eventually published.
Regards,
Jörg
^ 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