diff mbox series

intel-iommu: fix Read DMAR IQA REG DW

Message ID 20240704035302.306244-1-seven.yi.lee@gmail.com (mailing list archive)
State New, archived
Headers show
Series intel-iommu: fix Read DMAR IQA REG DW | expand

Commit Message

YeeLi July 4, 2024, 3:53 a.m. UTC 
From: yeeli <seven.yi.lee@gmail.com>

When dmar_readq or devmem2 read the DW of IQA always 0UL because
"& VTD_IQA_QS". So, try to fix it.

case:
after vtd_mem_write
IQA val: 0x100206801

after vtd_mem_read
IQA val: 0x100206001

Signed-off-by: yeeli <seven.yi.lee@gmail.com>
---
 hw/i386/intel_iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

YeeLi July 4, 2024, 6:54 a.m. UTC | #1 
Sorry, the patch missing "((  ))"
Replace "& VTD_IQA_QS" with "& (VTD_IQA_QS | VTD_IQA_DW_MASK))" is correct.
Revised patch as follows,

Signed-off-by: yeeli <seven.yi.lee@gmail.com>
---
 hw/i386/intel_iommu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 37c21a0aec..23562ba26b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2938,7 +2938,8 @@ static uint64_t vtd_mem_read(void *opaque,
hwaddr addr, unsigned size)

     /* Invalidation Queue Address Register, 64-bit */
     case DMAR_IQA_REG:
-        val = s->iq | (vtd_get_quad(s, DMAR_IQA_REG) & VTD_IQA_QS);
+        val = s->iq | (vtd_get_quad(s, DMAR_IQA_REG) & (VTD_IQA_QS
+                    | VTD_IQA_DW_MASK));
         if (size == 4) {
             val = val & ((1ULL << 32) - 1);
         }
Michael S. Tsirkin July 20, 2024, 6:28 p.m. UTC | #2 
On Thu, Jul 04, 2024 at 02:54:00PM +0800, Yee Li wrote:
> Sorry, the patch missing "((  ))"
> Replace "& VTD_IQA_QS" with "& (VTD_IQA_QS | VTD_IQA_DW_MASK))" is correct.
> Revised patch as follows,
> 

So submit it properly.
Also, how did you test the patch?

> Signed-off-by: yeeli <seven.yi.lee@gmail.com>




> ---
>  hw/i386/intel_iommu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 37c21a0aec..23562ba26b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2938,7 +2938,8 @@ static uint64_t vtd_mem_read(void *opaque,
> hwaddr addr, unsigned size)
> 
>      /* Invalidation Queue Address Register, 64-bit */
>      case DMAR_IQA_REG:
> -        val = s->iq | (vtd_get_quad(s, DMAR_IQA_REG) & VTD_IQA_QS);
> +        val = s->iq | (vtd_get_quad(s, DMAR_IQA_REG) & (VTD_IQA_QS
> +                    | VTD_IQA_DW_MASK));


This is a very messy way to write this.
Align things properly pls.

>          if (size == 4) {
>              val = val & ((1ULL << 32) - 1);
>          }
> -- 
> 2.34.1
> 
> YeeLi <seven.yi.lee@gmail.com> 于2024年7月4日周四 11:53写道：
> >
> > From: yeeli <seven.yi.lee@gmail.com>
> >
> > When dmar_readq or devmem2 read the DW of IQA always 0UL because
> > "& VTD_IQA_QS". So, try to fix it.
> >
> > case:
> > after vtd_mem_write
> > IQA val: 0x100206801
> >
> > after vtd_mem_read
> > IQA val: 0x100206001
> >
> > Signed-off-by: yeeli <seven.yi.lee@gmail.com>
> > ---
> >  hw/i386/intel_iommu.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 37c21a0aec..e230a45940 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -2938,7 +2938,8 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
> >
> >      /* Invalidation Queue Address Register, 64-bit */
> >      case DMAR_IQA_REG:
> > -        val = s->iq | (vtd_get_quad(s, DMAR_IQA_REG) & VTD_IQA_QS);
> > +        val = s->iq | (vtd_get_quad(s, DMAR_IQA_REG) & VTD_IQA_QS
> > +                | VTD_IQA_DW_MASK);
> >          if (size == 4) {
> >              val = val & ((1ULL << 32) - 1);
> >          }
> > --
> > 2.34.1
> >
Michael S. Tsirkin July 20, 2024, 6:29 p.m. UTC | #3 
On Thu, Jul 04, 2024 at 11:53:02AM +0800, YeeLi wrote:
> From: yeeli <seven.yi.lee@gmail.com>
> 
> When dmar_readq or devmem2 read the DW of IQA always 0UL because
> "& VTD_IQA_QS". So, try to fix it.
> 
> case:
> after vtd_mem_write
> IQA val: 0x100206801
> 
> after vtd_mem_read
> IQA val: 0x100206001
> 
> Signed-off-by: yeeli <seven.yi.lee@gmail.com>


how was this tested?

> ---
>  hw/i386/intel_iommu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 37c21a0aec..e230a45940 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2938,7 +2938,8 @@ static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
>  
>      /* Invalidation Queue Address Register, 64-bit */
>      case DMAR_IQA_REG:
> -        val = s->iq | (vtd_get_quad(s, DMAR_IQA_REG) & VTD_IQA_QS);
> +        val = s->iq | (vtd_get_quad(s, DMAR_IQA_REG) & VTD_IQA_QS
> +                | VTD_IQA_DW_MASK);
>          if (size == 4) {
>              val = val & ((1ULL << 32) - 1);
>          }
> -- 
> 2.34.1
YeeLi July 23, 2024, 2:40 a.m. UTC | #4 
> > When dmar_readq or devmem2 read the DW of IQA always 0UL because
> > "& VTD_IQA_QS". So, try to fix it.
> >
> > case:
> > after vtd_mem_write
> > IQA val: 0x100206801
> >
> > after vtd_mem_read
> > IQA val: 0x100206001
> >
> > Signed-off-by: yeeli <seven.yi.lee@gmail.com>
>
>
> how was this tested?

If VT-D hardware supports scalable mode, Linux will set the IQA DW (bit 11).
In qemu, the vtd_mem_write and vtd_update_iq_dw set DW well, however,
vtd_mem_read the DW wrong because "& VTD_IQA_QS" dropped the value
of DW (bit 11).

So testing the patch is easy,

config the "x-scalable-mode" option as below:

"-device intel-iommu,caching-mode=on,x-scalable-mode=on,aw-bits=48"

after Linux OS boot, check the IQA_REG DW (bit 11) as below:

IOMMU_DEBUGFS:

"cat /sys/kernel/debug/iommu/intel |grep IQA"
"IQA        0x90        0x00000001001da801"

or devmem2 read the reg:

"devmem2 0xfed90090"
"/dev/mem opened."
"Memory mapped at address 0x7f983014f000."
"Value at address 0xFED90090 (0x7f983014f000): 0x0"
YeeLi July 23, 2024, 3:02 a.m. UTC | #5 
> or devmem2 read the reg:
>
> "devmem2 0xfed90090"
> "/dev/mem opened."
> "Memory mapped at address 0x7f983014f000."
> "Value at address 0xFED90090 (0x7f983014f000): 0x0"

Sorry, correct the devmem2 read value.
"Value at address 0xFED90090 (0x7f983014f000): 0x1DA801"
Yi Liu July 23, 2024, 8:05 a.m. UTC | #6 
On 2024/7/23 10:40, Yee Li wrote:
>>> When dmar_readq or devmem2 read the DW of IQA always 0UL because
>>> "& VTD_IQA_QS". So, try to fix it.
>>>
>>> case:
>>> after vtd_mem_write
>>> IQA val: 0x100206801
>>>
>>> after vtd_mem_read
>>> IQA val: 0x100206001
>>>
>>> Signed-off-by: yeeli <seven.yi.lee@gmail.com>

I think you may need to capitalize the first character of your first and
last name.

>>
>> how was this tested?
> 
> If VT-D hardware supports scalable mode, Linux will set the IQA DW (bit 11).
> In qemu, the vtd_mem_write and vtd_update_iq_dw set DW well, however,
> vtd_mem_read the DW wrong because "& VTD_IQA_QS" dropped the value
> of DW (bit 11).
> 
> So testing the patch is easy,
> 
> config the "x-scalable-mode" option as below:
> 
> "-device intel-iommu,caching-mode=on,x-scalable-mode=on,aw-bits=48"
> 
> after Linux OS boot, check the IQA_REG DW (bit 11) as below:
> 
> IOMMU_DEBUGFS:
> 
> "cat /sys/kernel/debug/iommu/intel |grep IQA"
> "IQA        0x90        0x00000001001da801"
> 
> or devmem2 read the reg:
> 
> "devmem2 0xfed90090"
> "/dev/mem opened."
> "Memory mapped at address 0x7f983014f000."
> "Value at address 0xFED90090 (0x7f983014f000): 0x0"
> 

So you found it by checking the debugfs output, and it looks to miss
the DW bit. is it? Put a clearer commit message would be helpful.

Please address Michael's comment, add a "Fixes: xxx" tag and resend.
YeeLi July 24, 2024, 6:34 a.m. UTC | #7 
> So you found it by checking the debugfs output, and it looks to miss
> the DW bit. is it? Put a clearer commit message would be helpful.

Yes, it is. So, "dropped the value of DW field" is indeed a bug?

> Please address Michael's comment, add a "Fixes: xxx" tag and resend.
OK, I will.
diff mbox series

Patch

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 37c21a0aec..e230a45940 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2938,7 +2938,8 @@  static uint64_t vtd_mem_read(void *opaque, hwaddr addr, unsigned size)
 
     /* Invalidation Queue Address Register, 64-bit */
     case DMAR_IQA_REG:
-        val = s->iq | (vtd_get_quad(s, DMAR_IQA_REG) & VTD_IQA_QS);
+        val = s->iq | (vtd_get_quad(s, DMAR_IQA_REG) & VTD_IQA_QS
+                | VTD_IQA_DW_MASK);
         if (size == 4) {
             val = val & ((1ULL << 32) - 1);
         }