[PATCH] intel-iommu: Set status bit after operation completed

[PATCH] intel-iommu: Set status bit after operation completed

[Zhenzhong Duan]

According to SDM 11.4.4.2 Global Status Register:
"This field is cleared by hardware when software sets the SRTP field in the
Global Command register. This field is set by hardware when hardware
completes the ‘Set Root Table Pointer’ operation using the value provided
in the Root Table Address register"

Follow above spec to clear then set RTPS after finish all works, this way
helps avoiding potential race with guest kernel. Though linux kernel is
single threaded in writing GCMD_REG and checking GSTS_REG.

Same reasion for GSTS_REG.TES

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/i386/intel_iommu.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index faade7def8..7cba1945a3 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2312,11 +2312,12 @@ static void vtd_handle_gcmd_qie(IntelIOMMUState *s, bool en)
 /* Set Root Table Pointer */
 static void vtd_handle_gcmd_srtp(IntelIOMMUState *s)
 {
+    vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_RTPS, 0);
     vtd_root_table_setup(s);
-    /* Ok - report back to driver */
-    vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_RTPS);
     vtd_reset_caches(s);
     vtd_address_space_refresh_all(s);
+    /* Ok - report back to driver */
+    vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_RTPS);
 }
 
 /* Set Interrupt Remap Table Pointer */
@@ -2338,19 +2339,22 @@ static void vtd_handle_gcmd_te(IntelIOMMUState *s, bool en)
 
     if (en) {
         s->dmar_enabled = true;
-        /* Ok - report back to driver */
-        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_TES);
     } else {
         s->dmar_enabled = false;
 
         /* Clear the index of Fault Recording Register */
         s->next_frcd_reg = 0;
-        /* Ok - report back to driver */
-        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_TES, 0);
     }
 
     vtd_reset_caches(s);
     vtd_address_space_refresh_all(s);
+
+    /* Ok - report back to driver */
+    if (en) {
+        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_TES);
+    } else {
+        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_TES, 0);
+    }
 }
 
 /* Handle Interrupt Remap Enable/Disable */
-- 
2.25.1

Re: [PATCH] intel-iommu: Set status bit after operation completed

[Peter Xu]

Hi, Zhenzhong,

On Thu, Mar 09, 2023 at 05:23:19PM +0800, Zhenzhong Duan wrote:
> According to SDM 11.4.4.2 Global Status Register:
> "This field is cleared by hardware when software sets the SRTP field in the
> Global Command register. This field is set by hardware when hardware
> completes the ‘Set Root Table Pointer’ operation using the value provided
> in the Root Table Address register"
> 
> Follow above spec to clear then set RTPS after finish all works, this way
> helps avoiding potential race with guest kernel. Though linux kernel is
> single threaded in writing GCMD_REG and checking GSTS_REG.
> 
> Same reasion for GSTS_REG.TES

Is this a real bug?  Or, when it'll make a difference to the guest?

Thanks,

-- 
Peter Xu

RE: [PATCH] intel-iommu: Set status bit after operation completed

[Duan, Zhenzhong]

Hi Peter

>-----Original Message-----
>From: Peter Xu <peterx@redhat.com>
>Sent: Thursday, March 9, 2023 10:56 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>Cc: qemu-devel@nongnu.org; mst@redhat.com; jasowang@redhat.com;
>pbonzini@redhat.com; richard.henderson@linaro.org; eduardo@habkost.net;
>marcel.apfelbaum@gmail.com
>Subject: Re: [PATCH] intel-iommu: Set status bit after operation completed
>
>Hi, Zhenzhong,
>
>On Thu, Mar 09, 2023 at 05:23:19PM +0800, Zhenzhong Duan wrote:
>> According to SDM 11.4.4.2 Global Status Register:
>> "This field is cleared by hardware when software sets the SRTP field
>> in the Global Command register. This field is set by hardware when
>> hardware completes the ‘Set Root Table Pointer’ operation using the
>> value provided in the Root Table Address register"
>>
>> Follow above spec to clear then set RTPS after finish all works, this
>> way helps avoiding potential race with guest kernel. Though linux
>> kernel is single threaded in writing GCMD_REG and checking GSTS_REG.
>>
>> Same reasion for GSTS_REG.TES
>
>Is this a real bug?  Or, when it'll make a difference to the guest?
Not a real bug, just for robustness. Sorry, I should have made it clear in comments.

I think it may break with special designed guest OS,
E.x: Imagine a guest write GCMD_REG and start a new thread to do further work.
New thread find status bit in GTS_REG set and go ahead, but the address space switch
may not finish yet if guest memory is big, which may trigger a potential race.

Original code lives for a long history so it looks all the practiced os aren't designed as above.

Thanks
Zhenzhong

Re: [PATCH] intel-iommu: Set status bit after operation completed

[Peter Xu]

On Fri, Mar 10, 2023 at 02:32:13AM +0000, Duan, Zhenzhong wrote:
> I think it may break with special designed guest OS,
> E.x: Imagine a guest write GCMD_REG and start a new thread to do further work.
> New thread find status bit in GTS_REG set and go ahead, but the address space switch
> may not finish yet if guest memory is big, which may trigger a potential race.

IMHO it's fine.  For MMIO QEMU takes the BQL so if another thread reads the
status reg it should be serialized until the current vcpu finishes.

See prepare_mmio_access().  Thanks,

-- 
Peter Xu

RE: [PATCH] intel-iommu: Set status bit after operation completed

[Duan, Zhenzhong]

>-----Original Message-----
>From: Peter Xu <peterx@redhat.com>
>Sent: Friday, March 10, 2023 10:29 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>Cc: qemu-devel@nongnu.org; mst@redhat.com; jasowang@redhat.com;
>pbonzini@redhat.com; richard.henderson@linaro.org; eduardo@habkost.net;
>marcel.apfelbaum@gmail.com
>Subject: Re: [PATCH] intel-iommu: Set status bit after operation completed
>
>On Fri, Mar 10, 2023 at 02:32:13AM +0000, Duan, Zhenzhong wrote:
>> I think it may break with special designed guest OS,
>> E.x: Imagine a guest write GCMD_REG and start a new thread to do further
>work.
>> New thread find status bit in GTS_REG set and go ahead, but the
>> address space switch may not finish yet if guest memory is big, which may
>trigger a potential race.
>
>IMHO it's fine.  For MMIO QEMU takes the BQL so if another thread reads the
>status reg it should be serialized until the current vcpu finishes.
>
>See prepare_mmio_access().  Thanks,
You are right, just know this, thanks Peter.

Regards
Zhenzhong

Re: [PATCH] intel-iommu: Set status bit after operation completed

[Michael S. Tsirkin]

On Thu, Mar 09, 2023 at 05:23:19PM +0800, Zhenzhong Duan wrote:
> According to SDM 11.4.4.2 Global Status Register:
> "This field is cleared by hardware when software sets the SRTP field in the
> Global Command register. This field is set by hardware when hardware
> completes the ‘Set Root Table Pointer’ operation using the value provided
> in the Root Table Address register"
> 
> Follow above spec to clear then set RTPS after finish all works, this way
> helps avoiding potential race with guest kernel. Though linux kernel is
> single threaded in writing GCMD_REG and checking GSTS_REG.
> 
> Same reasion for GSTS_REG.TES
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>


So I am dropping this?

> ---
>  hw/i386/intel_iommu.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index faade7def8..7cba1945a3 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2312,11 +2312,12 @@ static void vtd_handle_gcmd_qie(IntelIOMMUState *s, bool en)
>  /* Set Root Table Pointer */
>  static void vtd_handle_gcmd_srtp(IntelIOMMUState *s)
>  {
> +    vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_RTPS, 0);
>      vtd_root_table_setup(s);
> -    /* Ok - report back to driver */
> -    vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_RTPS);
>      vtd_reset_caches(s);
>      vtd_address_space_refresh_all(s);
> +    /* Ok - report back to driver */
> +    vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_RTPS);
>  }
>  
>  /* Set Interrupt Remap Table Pointer */
> @@ -2338,19 +2339,22 @@ static void vtd_handle_gcmd_te(IntelIOMMUState *s, bool en)
>  
>      if (en) {
>          s->dmar_enabled = true;
> -        /* Ok - report back to driver */
> -        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_TES);
>      } else {
>          s->dmar_enabled = false;
>  
>          /* Clear the index of Fault Recording Register */
>          s->next_frcd_reg = 0;
> -        /* Ok - report back to driver */
> -        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_TES, 0);
>      }
>  
>      vtd_reset_caches(s);
>      vtd_address_space_refresh_all(s);
> +
> +    /* Ok - report back to driver */
> +    if (en) {
> +        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_TES);
> +    } else {
> +        vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_TES, 0);
> +    }
>  }
>  
>  /* Handle Interrupt Remap Enable/Disable */
> -- 
> 2.25.1

RE: [PATCH] intel-iommu: Set status bit after operation completed

[Duan, Zhenzhong]

>-----Original Message-----
>From: Michael S. Tsirkin <mst@redhat.com>
>Sent: Wednesday, May 10, 2023 2:29 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>Cc: qemu-devel@nongnu.org; peterx@redhat.com; jasowang@redhat.com;
>pbonzini@redhat.com; richard.henderson@linaro.org; eduardo@habkost.net;
>marcel.apfelbaum@gmail.com
>Subject: Re: [PATCH] intel-iommu: Set status bit after operation completed
>
>On Thu, Mar 09, 2023 at 05:23:19PM +0800, Zhenzhong Duan wrote:
>> According to SDM 11.4.4.2 Global Status Register:
>> "This field is cleared by hardware when software sets the SRTP field
>> in the Global Command register. This field is set by hardware when
>> hardware completes the ‘Set Root Table Pointer’ operation using the
>> value provided in the Root Table Address register"
>>
>> Follow above spec to clear then set RTPS after finish all works, this
>> way helps avoiding potential race with guest kernel. Though linux
>> kernel is single threaded in writing GCMD_REG and checking GSTS_REG.
>>
>> Same reasion for GSTS_REG.TES
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
>
>So I am dropping this?

Yes, please. As Peter point out, there is no such race as BQL serialize that.

Thanks
Zhenzhong