From: Peter Xu <peterx@redhat.com>

No bug report for this, but logically tearing down of existing address
space should happen before reset of IOMMU state / registers, because the
current address spaces may still rely on those information.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 1a07faddb4..8b467cbbd2 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -4090,8 +4090,8 @@ static void vtd_reset(DeviceState *dev)
 {
     IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
 
-    vtd_init(s);
     vtd_address_space_refresh_all(s);
+    vtd_init(s);
 }
 
 static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
-- 
2.43.0

Hi Peter,

On 17/1/24 10:15, peterx@redhat.com wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> No bug report for this, but logically tearing down of existing address
> space should happen before reset of IOMMU state / registers, because the
> current address spaces may still rely on those information.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/i386/intel_iommu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 1a07faddb4..8b467cbbd2 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -4090,8 +4090,8 @@ static void vtd_reset(DeviceState *dev)
>   {
>       IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>   
> -    vtd_init(s);
>       vtd_address_space_refresh_all(s);
> +    vtd_init(s);
>   }

You might want to convert to 3-phases reset API here, calling
vtd_address_space_refresh_all() in a ResettableEnterPhase handler
and vtd_init() in ResettableHoldPhase (or ResettableExitPhase?).

Hi Peter,

On 1/17/24 10:15, peterx@redhat.com wrote:
> From: Peter Xu <peterx@redhat.com>
>
> No bug report for this, but logically tearing down of existing address
> space should happen before reset of IOMMU state / registers, because the
> current address spaces may still rely on those information.
do you mean that vtd_address_space_refresh_all() implementation my rely
on data reset by vtd_init()?

By the way the comment before the function is a bit confusing because it
says that we should not reset as when reset but isn't it was it done here?

Thanks

Eric
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 1a07faddb4..8b467cbbd2 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -4090,8 +4090,8 @@ static void vtd_reset(DeviceState *dev)
>  {
>      IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>  
> -    vtd_init(s);
>      vtd_address_space_refresh_all(s);
> +    vtd_init(s);
>  }
>  
>  static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)