| Message ID | 436ac2021bb3d75114ca0e45f25a6a8257489d3b.1737754129.git.nicolinc@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommufd: Add vIOMMU infrastructure (Part-3: vEVENTQ) | expand |
On Fri, Jan 24, 2025 at 04:30:43PM -0800, Nicolin Chen wrote: > There is a DoS concern on the shared hardware event queue among devices > passed through to VMs, that too many translation failures that belong to > VMs could overflow the shared hardware event queue if those VMs or their > VMMs don't handle/recover the devices properly. > > The MEV bit in the STE allows to configure the SMMU HW to merge similar > event records, though there is no guarantee. Set it in a nested STE for > DoS mitigations. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 2 ++ > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 ++-- > 3 files changed, 5 insertions(+), 2 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -1051,7 +1051,7 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits) > cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR | > STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH | > STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW | > - STRTAB_STE_1_EATS); > + STRTAB_STE_1_EATS | STRTAB_STE_1_MEV); > used_bits[2] |= cpu_to_le64(STRTAB_STE_2_S2VMID); You also ran the test suite? Jason
On Tue, Feb 18, 2025 at 01:21:20PM -0400, Jason Gunthorpe wrote: > On Fri, Jan 24, 2025 at 04:30:43PM -0800, Nicolin Chen wrote: > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > @@ -1051,7 +1051,7 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits) > > cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR | > > STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH | > > STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW | > > - STRTAB_STE_1_EATS); > > + STRTAB_STE_1_EATS | STRTAB_STE_1_MEV); > > used_bits[2] |= cpu_to_le64(STRTAB_STE_2_S2VMID); > > You also ran the test suite? Yes, I enabled that in my config, and didn't see anything wrong: [ 10.832738] # Subtest: arm-smmu-v3-kunit-test [ 10.837549] # module: arm_smmu_v3_test [ 10.844208] ok 1 arm_smmu_v3_write_ste_test_bypass_to_abort [ 10.844339] ok 2 arm_smmu_v3_write_ste_test_abort_to_bypass [ 10.850507] ok 3 arm_smmu_v3_write_ste_test_cdtable_to_abort [ 10.856669] ok 4 arm_smmu_v3_write_ste_test_abort_to_cdtable [ 10.862934] ok 5 arm_smmu_v3_write_ste_test_cdtable_to_bypass [ 10.869200] ok 6 arm_smmu_v3_write_ste_test_bypass_to_cdtable [ 10.875550] ok 7 arm_smmu_v3_write_ste_test_cdtable_s1dss_change [ 10.881899] ok 8 arm_smmu_v3_write_ste_test_s1dssbypass_to_stebypass [ 10.888512] ok 9 arm_smmu_v3_write_ste_test_stebypass_to_s1dssbypass [ 10.895482] ok 10 arm_smmu_v3_write_ste_test_s2_to_abort [ 10.902457] ok 11 arm_smmu_v3_write_ste_test_abort_to_s2 [ 10.908355] ok 12 arm_smmu_v3_write_ste_test_s2_to_bypass [ 10.914263] ok 13 arm_smmu_v3_write_ste_test_bypass_to_s2 [ 10.920269] ok 14 arm_smmu_v3_write_ste_test_s1_to_s2 [ 10.926267] ok 15 arm_smmu_v3_write_ste_test_s2_to_s1 [ 10.931900] ok 16 arm_smmu_v3_write_ste_test_non_hitless [ 10.937536] ok 17 arm_smmu_v3_write_cd_test_s1_clear [ 10.943435] ok 18 arm_smmu_v3_write_cd_test_s1_change_asid [ 10.948995] ok 19 arm_smmu_v3_write_ste_test_s1_to_s2_stall [ 10.955074] ok 20 arm_smmu_v3_write_ste_test_s2_to_s1_stall [ 10.961244] ok 21 arm_smmu_v3_write_cd_test_sva_clear [ 10.967419] ok 22 arm_smmu_v3_write_cd_test_sva_release [ 10.972941] # arm-smmu-v3-kunit-test: pass:22 fail:0 skip:0 total:22 [ 10.985141] ok 1 arm-smmu-v3-kunit-test Thanks Nicolin
On Tue, Feb 18, 2025 at 05:24:08AM +0000, Tian, Kevin wrote: > > From: Nicolin Chen <nicolinc@nvidia.com> > > Sent: Saturday, January 25, 2025 8:31 AM > > > > There is a DoS concern on the shared hardware event queue among devices > > passed through to VMs, that too many translation failures that belong to > > VMs could overflow the shared hardware event queue if those VMs or their > > VMMs don't handle/recover the devices properly. > > This statement is not specific to the nested configuration. > > > > > The MEV bit in the STE allows to configure the SMMU HW to merge similar > > event records, though there is no guarantee. Set it in a nested STE for > > DoS mitigations. > > Is MEV available only in nested mode? Otherwise it perhaps makes > sense to turn it on in all configurations in IOMMUFD paths... MEV is available at all times (if an implemented by the HW) and doesn't depend on the nested mode. As per the Arm SMMUv3 spec (section 3.5.5): Events can be merged where all of the following conditions are upheld: - The event types and all fields are identical, except fields explicitly indicated in section 7.3 Event records. - If present, the Stall field is 0. Stall fault records are not merged. I'm not sure to what extent, but I think *trying* to merge similar event should reduce some chances of overflowing the hw eventq. > Is MEV available only in nested mode? Otherwise it perhaps makes > sense to turn it on in all configurations in IOMMUFD paths... I think the arm-smmu-v3's iommufd implementation only supports nested which could be the reason. Thanks, Praan
On Tue, Feb 18, 2025 at 06:17:15PM +0000, Pranjal Shrivastava wrote: > > Is MEV available only in nested mode? Otherwise it perhaps makes > > sense to turn it on in all configurations in IOMMUFD paths... > > I think the arm-smmu-v3's iommufd implementation only supports nested > which could be the reason. I think starting with MEV in this limited case is reasonable. I agree it makes sense to always turn it on from a production perspective.. Jason
On Tue, Feb 18, 2025 at 06:17:15PM +0000, Pranjal Shrivastava wrote: > On Tue, Feb 18, 2025 at 05:24:08AM +0000, Tian, Kevin wrote: > > > From: Nicolin Chen <nicolinc@nvidia.com> > > > Sent: Saturday, January 25, 2025 8:31 AM > > > > > > There is a DoS concern on the shared hardware event queue among devices > > > passed through to VMs, that too many translation failures that belong to > > > VMs could overflow the shared hardware event queue if those VMs or their > > > VMMs don't handle/recover the devices properly. > > > > This statement is not specific to the nested configuration. > > > > > > > > The MEV bit in the STE allows to configure the SMMU HW to merge similar > > > event records, though there is no guarantee. Set it in a nested STE for > > > DoS mitigations. > > > > Is MEV available only in nested mode? Otherwise it perhaps makes > > sense to turn it on in all configurations in IOMMUFD paths... > > MEV is available at all times (if an implemented by the HW) and doesn't > depend on the nested mode. As per the Arm SMMUv3 spec (section 3.5.5): > > Events can be merged where all of the following conditions are upheld: > - The event types and all fields are identical, except fields explicitly > indicated in section 7.3 Event records. > > - If present, the Stall field is 0. Stall fault records are not merged. > > I'm not sure to what extent, but I think *trying* to merge similar event > should reduce some chances of overflowing the hw eventq. > > > Is MEV available only in nested mode? Otherwise it perhaps makes > > sense to turn it on in all configurations in IOMMUFD paths... > > I think the arm-smmu-v3's iommufd implementation only supports nested > which could be the reason. I guess what Kevin says is that non-nested STE should set the MEV as well, e.g. BYPASS and ABORT, and perhaps stage-1-only case too where the attaching domain = UNMANAGED. Thanks Nicolin
On Tue, Feb 18, 2025 at 02:52:29PM -0400, Jason Gunthorpe wrote: > On Tue, Feb 18, 2025 at 06:17:15PM +0000, Pranjal Shrivastava wrote: > > > > Is MEV available only in nested mode? Otherwise it perhaps makes > > > sense to turn it on in all configurations in IOMMUFD paths... > > > > I think the arm-smmu-v3's iommufd implementation only supports nested > > which could be the reason. > > I think starting with MEV in this limited case is reasonable. > > I agree it makes sense to always turn it on from a production > perspective.. Then, I will just add a line to the commit log: "In the future, we might want to enable the MEV for non-nested cases too such as domain->type == IOMMU_DOMAIN_UNMANAGED or even IOMMU_DOMAIN_DMA." Thanks Nicolin
On Fri, Jan 24, 2025 at 04:30:43PM -0800, Nicolin Chen wrote: > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c > index ceeed907a714..20a0e39d7caa 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c > @@ -43,6 +43,8 @@ static void arm_smmu_make_nested_cd_table_ste( > target->data[0] |= nested_domain->ste[0] & > ~cpu_to_le64(STRTAB_STE_0_CFG); > target->data[1] |= nested_domain->ste[1]; > + /* Merge events for DoS mitigations on eventq */ > + target->data[1] |= STRTAB_STE_1_MEV; This should have cpu_to_le64(). Fixed accordingly. Thanks Nicolin
On Tue, Feb 18, 2025 at 10:53:55AM -0800, Nicolin Chen wrote: > > > Is MEV available only in nested mode? Otherwise it perhaps makes > > > sense to turn it on in all configurations in IOMMUFD paths... > > > > I think the arm-smmu-v3's iommufd implementation only supports nested > > which could be the reason. > > I guess what Kevin says is that non-nested STE should set the MEV > as well, e.g. BYPASS and ABORT, and perhaps stage-1-only case too > where the attaching domain = UNMANAGED. > Ohh okay, got it. Thanks! Praan
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index d24c3d8ee397..7181001fc5d7 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -266,6 +266,7 @@ static inline u32 arm_smmu_strtab_l2_idx(u32 sid) #define STRTAB_STE_1_S1COR GENMASK_ULL(5, 4) #define STRTAB_STE_1_S1CSH GENMASK_ULL(7, 6) +#define STRTAB_STE_1_MEV (1UL << 19) #define STRTAB_STE_1_S2FWB (1UL << 25) #define STRTAB_STE_1_S1STALLD (1UL << 27) diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c index ceeed907a714..20a0e39d7caa 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c @@ -43,6 +43,8 @@ static void arm_smmu_make_nested_cd_table_ste( target->data[0] |= nested_domain->ste[0] & ~cpu_to_le64(STRTAB_STE_0_CFG); target->data[1] |= nested_domain->ste[1]; + /* Merge events for DoS mitigations on eventq */ + target->data[1] |= STRTAB_STE_1_MEV; } /* diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 59fbc342a095..14e079cfb8b6 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1051,7 +1051,7 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits) cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR | STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH | STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW | - STRTAB_STE_1_EATS); + STRTAB_STE_1_EATS | STRTAB_STE_1_MEV); used_bits[2] |= cpu_to_le64(STRTAB_STE_2_S2VMID); /* @@ -1067,7 +1067,7 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits) if (cfg & BIT(1)) { used_bits[1] |= cpu_to_le64(STRTAB_STE_1_S2FWB | STRTAB_STE_1_EATS | - STRTAB_STE_1_SHCFG); + STRTAB_STE_1_SHCFG | STRTAB_STE_1_MEV); used_bits[2] |= cpu_to_le64(STRTAB_STE_2_S2VMID | STRTAB_STE_2_VTCR | STRTAB_STE_2_S2AA64 | STRTAB_STE_2_S2ENDI |
There is a DoS concern on the shared hardware event queue among devices passed through to VMs, that too many translation failures that belong to VMs could overflow the shared hardware event queue if those VMs or their VMMs don't handle/recover the devices properly. The MEV bit in the STE allows to configure the SMMU HW to merge similar event records, though there is no guarantee. Set it in a nested STE for DoS mitigations. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 2 ++ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 ++-- 3 files changed, 5 insertions(+), 2 deletions(-)