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

Zhenzhong Duan posted 1 patch 1 year ago
hw/i386/intel_iommu.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
[PATCH] intel-iommu: Set status bit after operation completed
Posted by Zhenzhong Duan 1 year ago
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
Posted by Michael S. Tsirkin 10 months, 3 weeks ago
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
Posted by Duan, Zhenzhong 10 months, 3 weeks ago

>-----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
Re: [PATCH] intel-iommu: Set status bit after operation completed
Posted by Peter Xu 1 year ago
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
Posted by Duan, Zhenzhong 1 year ago
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
Posted by Peter Xu 1 year ago
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
Posted by Duan, Zhenzhong 1 year ago
>-----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