Hi Eric,
On 8/31/2026 4:22 PM, Eric Auger wrote:
Hi Tao,
On 8/13/26 6:26 PM, Tao Tang wrote:
Parse each PCI device's sec-sid property during SMMU device initialization
and cache it in SMMUDevice::sec_sid. Support "non-secure" and "secure",
default to non-secure when unspecified, and reject invalid values with an
explicit error. Use sdev->sec_sid in smmuv3_translate() to select the
register bank instead of hardcoding the non-secure context.
Keep sec-sid parsing in smmu-common, and add a SMMUv3-specific validation
hook to enforce architectural constraints: fail fast when sec-sid=secure
while SMMU_S_IDR1.SECURE_IMPL is 0 or secure AS is not available.
Typically, SEC_SID is a system-defined attribute (e.g. sideband or tied-off)
rather than something a PCIe endpoint can freely toggle in pre-RME scenario.
So this PCI sec-sid property is used as a static platform/testing knob to
drive the SMMU bank selection.
For future RME-DA and TDISP support, this static property will need to be
replaced by runtime platform plumbing that derives the effective SEC_SID
from the device security assignment.
Signed-off-by: Tao Tang <[email protected]>
Reviewed-by: Pierrick Bouvier <[email protected]>
---
hw/arm/smmu-common.c | 37 ++++++++++++++++++++++
hw/arm/smmuv3.c | 61 +++++++++++++++++++++++++++++++++++-
include/hw/arm/smmu-common.h | 2 ++
3 files changed, 99 insertions(+), 1 deletion(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index e8a1ed65c19..4a94799fb0d 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -21,6 +21,7 @@
#include "exec/target_page.h"
#include "hw/core/cpu.h"
#include "hw/pci/pci_bridge.h"
+#include "hw/pci/pci_device.h"
#include "hw/core/qdev-properties.h"
#include "qapi/error.h"
#include "qemu/jhash.h"
@@ -1100,14 +1101,50 @@ SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t
bus_num)
return NULL;
}
+static SMMUSecSID smmu_parse_pci_sec_sid(PCIDevice *pdev, int bus_num,
+ int devfn)
+{
+ const char *sec_sid;
+
+ if (!pdev || !pdev->sec_sid) {
+ return SMMU_SEC_SID_NS;
+ }
+
+ sec_sid = pdev->sec_sid;
+ if (!strcmp(sec_sid, "non-secure")) {
+ return SMMU_SEC_SID_NS;
+ }
else if?
+ if (!strcmp(sec_sid, "secure")) {
+ return SMMU_SEC_SID_S;
+ }
+
+ error_report("Invalid sec-sid value '%s' for PCI device %02x:%02x.%x; "
+ "allowed values: non-secure or secure (case-sensitive)",
To me this is not the place where pci property validation should happen.
This should happen in the pcie device instead, on property setting.
I've moved sec-sid value validation to the PCI property setter and
simplified the SMMU-side conversion.
+ sec_sid, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
+ exit(EXIT_FAILURE);
+}
+
void smmu_init_sdev(SMMUState *s, SMMUDevice *sdev, PCIBus *bus, int devfn)
{
static unsigned int index;
g_autofree char *name = g_strdup_printf("%s-%d-%d", s->mrtypename, devfn,
index++);
+ SMMUBaseClass *sbc = ARM_SMMU_GET_CLASS(s);
+ PCIDevice *pdev;
+ int bus_num;
+
sdev->smmu = s;
sdev->bus = bus;
sdev->devfn = devfn;
+ sdev->sec_sid = SMMU_SEC_SID_NS;
do we need this init? smmu_parse_pci_sec_sid() already implements the
default.
Yes this should be removed.
+
+ bus_num = pci_bus_num(bus);
+ pdev = pci_find_device(bus, bus_num, devfn);
+ sdev->sec_sid = smmu_parse_pci_sec_sid(pdev, bus_num, devfn);
+ if (sbc->validate_sec_sid &&
+ !sbc->validate_sec_sid(s, sdev, bus_num)) {
+ exit(EXIT_FAILURE);
The problem is any attempt to hotplu a secure device will exit QEMU.
I wonder if we shouldn't implement a PCIIOMMUOps that detects the
incompatibility.
Maybe supports_address_space() could do that. At the moment it is used
to check accel mode only but another implementation could check secure
compatibility I think. The advantage is this callback passes an errp.
Thanks for the advice. I'll add a new smmu_supports_address_space()
callback to smmu_ops to check Secure compatibility and propagate errors
through errp, so incompatible hotplug requests won't terminate QEMU.
static const PCIIOMMUOps smmu_ops = {
.supports_address_space = smmu_supports_address_space,
.get_address_space = smmu_find_add_as,
};
+ }
memory_region_init_iommu(&sdev->iommu, sizeof(sdev->iommu),
s->mrtypename, OBJECT(s), name, UINT64_MAX);
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index e5f0bc18415..a755f1ebd69 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -43,6 +43,12 @@
((ptw_info).stage == SMMU_STAGE_2 && \
(cfg)->s2cfg.record_faults))
+enum {
+ SMMU_IOMMU_IDX_NS,
+ SMMU_IOMMU_IDX_S,
+ SMMU_IOMMU_IDX_NUM,
+};
why do we need another enum, can't we reuse SMMUSecSID?
......
------------------------------<snip>------------------------------
------------------------------<snip>------------------------------
object_class_property_set_description(klass, "accel",
"Enable SMMUv3 accelerator support. Allows host SMMUv3 to be "
@@ -2905,6 +2962,8 @@ static void
smmuv3_iommu_memory_region_class_init(ObjectClass *klass,
imrc->translate = smmuv3_translate;
imrc->notify_flag_changed = smmuv3_notify_flag_changed;
+ imrc->attrs_to_index = smmuv3_attrs_to_index;
+ imrc->num_indexes = smmuv3_num_indexes;
Why do we need the above ops? Isn't it generic enough to stay in smmu
base code?
In v6, I'll drop SMMU_IOMMU_IDX_* and reuse the SMMU_SEC_SID_*
constants. I'll also move the attrs_to_index() and num_indexes()
implementations to smmu-common.c/.h with function renamed to smmu_*.
Best regards,
Tao
