The X86IOMMUState::pt_supported boolean was only set in
the hw_compat_2_9[] array, via the 'pt=off' property. We
removed all machines using that array, lets remove that
property and all the code around it, always setting the
VTD_ECAP_PT capability.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/hw/i386/x86-iommu.h | 1 -
hw/i386/amd_iommu.c | 12 ++----------
hw/i386/intel_iommu.c | 13 ++-----------
hw/i386/x86-iommu.c | 1 -
4 files changed, 4 insertions(+), 23 deletions(-)
diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h
index bfd21649d08..d6e52b1eb6b 100644
--- a/include/hw/i386/x86-iommu.h
+++ b/include/hw/i386/x86-iommu.h
@@ -63,7 +63,6 @@ struct X86IOMMUState {
SysBusDevice busdev;
OnOffAuto intr_supported; /* Whether vIOMMU supports IR */
bool dt_supported; /* Whether vIOMMU supports DT */
- bool pt_supported; /* Whether vIOMMU supports pass-through */
QLIST_HEAD(, IEC_Notifier) iec_notifiers; /* IEC notify list */
};
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 2cf7e24a21d..516e231bf13 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1426,7 +1426,6 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
AMDVIState *s = opaque;
AMDVIAddressSpace **iommu_as, *amdvi_dev_as;
int bus_num = pci_bus_num(bus);
- X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
iommu_as = s->address_spaces[bus_num];
@@ -1486,15 +1485,8 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
AMDVI_INT_ADDR_FIRST,
&amdvi_dev_as->iommu_ir, 1);
- if (!x86_iommu->pt_supported) {
- memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, false);
- memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
- true);
- } else {
- memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
- false);
- memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, true);
- }
+ memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, false);
+ memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu), true);
}
return &iommu_as[devfn]->as;
}
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c980cecb4ee..cc08dc41441 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1066,6 +1066,7 @@ static inline bool vtd_ce_type_check(X86IOMMUState *x86_iommu,
{
switch (vtd_ce_get_type(ce)) {
case VTD_CONTEXT_TT_MULTI_LEVEL:
+ case VTD_CONTEXT_TT_PASS_THROUGH:
/* Always supported */
break;
case VTD_CONTEXT_TT_DEV_IOTLB:
@@ -1074,12 +1075,6 @@ static inline bool vtd_ce_type_check(X86IOMMUState *x86_iommu,
return false;
}
break;
- case VTD_CONTEXT_TT_PASS_THROUGH:
- if (!x86_iommu->pt_supported) {
- error_report_once("%s: PT specified but not supported", __func__);
- return false;
- }
- break;
default:
/* Unknown type */
error_report_once("%s: unknown ce type: %"PRIu32, __func__,
@@ -4520,7 +4515,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
{
X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
- s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
+ s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | VTD_ECAP_PT |
VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS |
VTD_CAP_MGAW(s->aw_bits);
if (s->dma_drain) {
@@ -4548,10 +4543,6 @@ static void vtd_cap_init(IntelIOMMUState *s)
s->ecap |= VTD_ECAP_DT;
}
- if (x86_iommu->pt_supported) {
- s->ecap |= VTD_ECAP_PT;
- }
-
if (s->caching_mode) {
s->cap |= VTD_CAP_CM;
}
diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
index d34a6849f4a..ca7cd953e98 100644
--- a/hw/i386/x86-iommu.c
+++ b/hw/i386/x86-iommu.c
@@ -129,7 +129,6 @@ static const Property x86_iommu_properties[] = {
DEFINE_PROP_ON_OFF_AUTO("intremap", X86IOMMUState,
intr_supported, ON_OFF_AUTO_AUTO),
DEFINE_PROP_BOOL("device-iotlb", X86IOMMUState, dt_supported, false),
- DEFINE_PROP_BOOL("pt", X86IOMMUState, pt_supported, true),
};
static void x86_iommu_class_init(ObjectClass *klass, const void *data)
--
2.47.1
On Thu, 1 May 2025 23:04:56 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> The X86IOMMUState::pt_supported boolean was only set in
> the hw_compat_2_9[] array, via the 'pt=off' property. We
> removed all machines using that array, lets remove that
> property and all the code around it, always setting the
> VTD_ECAP_PT capability.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> include/hw/i386/x86-iommu.h | 1 -
> hw/i386/amd_iommu.c | 12 ++----------
> hw/i386/intel_iommu.c | 13 ++-----------
> hw/i386/x86-iommu.c | 1 -
> 4 files changed, 4 insertions(+), 23 deletions(-)
>
> diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h
> index bfd21649d08..d6e52b1eb6b 100644
> --- a/include/hw/i386/x86-iommu.h
> +++ b/include/hw/i386/x86-iommu.h
> @@ -63,7 +63,6 @@ struct X86IOMMUState {
> SysBusDevice busdev;
> OnOffAuto intr_supported; /* Whether vIOMMU supports IR */
> bool dt_supported; /* Whether vIOMMU supports DT */
> - bool pt_supported; /* Whether vIOMMU supports pass-through */
> QLIST_HEAD(, IEC_Notifier) iec_notifiers; /* IEC notify list */
> };
>
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index 2cf7e24a21d..516e231bf13 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -1426,7 +1426,6 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> AMDVIState *s = opaque;
> AMDVIAddressSpace **iommu_as, *amdvi_dev_as;
> int bus_num = pci_bus_num(bus);
> - X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>
> iommu_as = s->address_spaces[bus_num];
>
> @@ -1486,15 +1485,8 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
> AMDVI_INT_ADDR_FIRST,
> &amdvi_dev_as->iommu_ir, 1);
>
> - if (!x86_iommu->pt_supported) {
> - memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, false);
> - memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
> - true);
> - } else {
> - memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
> - false);
> - memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, true);
> - }
> + memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, false);
> + memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu), true);
given default is 'true', this hunk seems to be backwards,
shouldn't it be 'else' branch code
> }
> return &iommu_as[devfn]->as;
> }
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index c980cecb4ee..cc08dc41441 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1066,6 +1066,7 @@ static inline bool vtd_ce_type_check(X86IOMMUState *x86_iommu,
> {
> switch (vtd_ce_get_type(ce)) {
> case VTD_CONTEXT_TT_MULTI_LEVEL:
> + case VTD_CONTEXT_TT_PASS_THROUGH:
> /* Always supported */
> break;
> case VTD_CONTEXT_TT_DEV_IOTLB:
> @@ -1074,12 +1075,6 @@ static inline bool vtd_ce_type_check(X86IOMMUState *x86_iommu,
> return false;
> }
> break;
> - case VTD_CONTEXT_TT_PASS_THROUGH:
> - if (!x86_iommu->pt_supported) {
> - error_report_once("%s: PT specified but not supported", __func__);
> - return false;
> - }
> - break;
> default:
> /* Unknown type */
> error_report_once("%s: unknown ce type: %"PRIu32, __func__,
> @@ -4520,7 +4515,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
> {
> X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>
> - s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
> + s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | VTD_ECAP_PT |
> VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS |
> VTD_CAP_MGAW(s->aw_bits);
> if (s->dma_drain) {
> @@ -4548,10 +4543,6 @@ static void vtd_cap_init(IntelIOMMUState *s)
> s->ecap |= VTD_ECAP_DT;
> }
>
> - if (x86_iommu->pt_supported) {
> - s->ecap |= VTD_ECAP_PT;
> - }
> -
> if (s->caching_mode) {
> s->cap |= VTD_CAP_CM;
> }
> diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
> index d34a6849f4a..ca7cd953e98 100644
> --- a/hw/i386/x86-iommu.c
> +++ b/hw/i386/x86-iommu.c
> @@ -129,7 +129,6 @@ static const Property x86_iommu_properties[] = {
> DEFINE_PROP_ON_OFF_AUTO("intremap", X86IOMMUState,
> intr_supported, ON_OFF_AUTO_AUTO),
> DEFINE_PROP_BOOL("device-iotlb", X86IOMMUState, dt_supported, false),
> - DEFINE_PROP_BOOL("pt", X86IOMMUState, pt_supported, true),
> };
>
> static void x86_iommu_class_init(ObjectClass *klass, const void *data)
On 6/6/25 9:42 AM, Igor Mammedov wrote:
> On Thu, 1 May 2025 23:04:56 +0200
> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
>> @@ -1486,15 +1485,8 @@ static AddressSpace *amdvi_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
>> AMDVI_INT_ADDR_FIRST,
>> &amdvi_dev_as->iommu_ir, 1);
>>
>> - if (!x86_iommu->pt_supported) {
>> - memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, false);
>> - memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
>> - true);
>> - } else {
>> - memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu),
>> - false);
>> - memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, true);
>> - }
>> + memory_region_set_enabled(&amdvi_dev_as->iommu_nodma, false);
>> + memory_region_set_enabled(MEMORY_REGION(&amdvi_dev_as->iommu), true);
>
> given default is 'true', this hunk seems to be backwards,
> shouldn't it be 'else' branch code
>
I believe this is trying to preserve the change recently introduced in:
31753d5a336f ("hw/i386/amd_iommu: Fix device setup failure when PT is on.")
However, that doesn't explain the logic of enabling those specific
memory regions, which I think is also prone to cause confusion. I tried
to explain the "trick" and argued against it here[0].
I am ultimately rewriting that code in a series to add DMA remap
support[1], which hopefully makes it easier to follow which memory
regions are active under specific configurations.
Thank you,
Alejandro
[0]
https://lore.kernel.org/qemu-devel/914314b3-611d-4da3-9050-3c8c1b881e40@oracle.com/
[1]
https://lore.kernel.org/qemu-devel/20250502021605.1795985-1-alejandro.j.jimenez@oracle.com/
>
>> }
>> return &iommu_as[devfn]->as;
>> }
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index c980cecb4ee..cc08dc41441 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -1066,6 +1066,7 @@ static inline bool vtd_ce_type_check(X86IOMMUState *x86_iommu,
>> {
>> switch (vtd_ce_get_type(ce)) {
>> case VTD_CONTEXT_TT_MULTI_LEVEL:
>> + case VTD_CONTEXT_TT_PASS_THROUGH:
>> /* Always supported */
>> break;
>> case VTD_CONTEXT_TT_DEV_IOTLB:
>> @@ -1074,12 +1075,6 @@ static inline bool vtd_ce_type_check(X86IOMMUState *x86_iommu,
>> return false;
>> }
>> break;
>> - case VTD_CONTEXT_TT_PASS_THROUGH:
>> - if (!x86_iommu->pt_supported) {
>> - error_report_once("%s: PT specified but not supported", __func__);
>> - return false;
>> - }
>> - break;
>> default:
>> /* Unknown type */
>> error_report_once("%s: unknown ce type: %"PRIu32, __func__,
>> @@ -4520,7 +4515,7 @@ static void vtd_cap_init(IntelIOMMUState *s)
>> {
>> X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>>
>> - s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND |
>> + s->cap = VTD_CAP_FRO | VTD_CAP_NFR | VTD_CAP_ND | VTD_ECAP_PT |
>> VTD_CAP_MAMV | VTD_CAP_PSI | VTD_CAP_SLLPS |
>> VTD_CAP_MGAW(s->aw_bits);
>> if (s->dma_drain) {
>> @@ -4548,10 +4543,6 @@ static void vtd_cap_init(IntelIOMMUState *s)
>> s->ecap |= VTD_ECAP_DT;
>> }
>>
>> - if (x86_iommu->pt_supported) {
>> - s->ecap |= VTD_ECAP_PT;
>> - }
>> -
>> if (s->caching_mode) {
>> s->cap |= VTD_CAP_CM;
>> }
>> diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
>> index d34a6849f4a..ca7cd953e98 100644
>> --- a/hw/i386/x86-iommu.c
>> +++ b/hw/i386/x86-iommu.c
>> @@ -129,7 +129,6 @@ static const Property x86_iommu_properties[] = {
>> DEFINE_PROP_ON_OFF_AUTO("intremap", X86IOMMUState,
>> intr_supported, ON_OFF_AUTO_AUTO),
>> DEFINE_PROP_BOOL("device-iotlb", X86IOMMUState, dt_supported, false),
>> - DEFINE_PROP_BOOL("pt", X86IOMMUState, pt_supported, true),
>> };
>>
>> static void x86_iommu_class_init(ObjectClass *klass, const void *data)
>
>
© 2016 - 2025 Red Hat, Inc.