RE: [PATCH] intel-iommu: Set status bit after operation completed
>-Original Message- >From: Michael S. Tsirkin >Sent: Wednesday, May 10, 2023 2:29 PM >To: Duan, Zhenzhong >Cc: qemu-devel@nongnu.org; pet...@redhat.com; jasow...@redhat.com; >pbonz...@redhat.com; richard.hender...@linaro.org; edua...@habkost.net; >marcel.apfelb...@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 > > >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
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
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
>-Original Message- >From: Peter Xu >Sent: Friday, March 10, 2023 10:29 PM >To: Duan, Zhenzhong >Cc: qemu-devel@nongnu.org; m...@redhat.com; jasow...@redhat.com; >pbonz...@redhat.com; richard.hender...@linaro.org; edua...@habkost.net; >marcel.apfelb...@gmail.com >Subject: Re: [PATCH] intel-iommu: Set status bit after operation completed > >On Fri, Mar 10, 2023 at 02:32:13AM +, 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
On Fri, Mar 10, 2023 at 02:32:13AM +, 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
Hi Peter >-Original Message- >From: Peter Xu >Sent: Thursday, March 9, 2023 10:56 PM >To: Duan, Zhenzhong >Cc: qemu-devel@nongnu.org; m...@redhat.com; jasow...@redhat.com; >pbonz...@redhat.com; richard.hender...@linaro.org; edua...@habkost.net; >marcel.apfelb...@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
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
