| Message ID | 20250501210456.89071-19-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/i386/pc: Remove deprecated 2.8 and 2.9 PC machines | expand |
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) > >
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)
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(-)