diff mbox series

[v5,08/20] intel_iommu: Check stage-1 translation result with interrupt range

Message ID 20241111083457.2090664-9-zhenzhong.duan@intel.com
State New
Headers show
Series intel_iommu: Enable stage-1 translation for emulated device | expand

Commit Message

Duan, Zhenzhong Nov. 11, 2024, 8:34 a.m. UTC
Per VT-d spec 4.1 section 3.15, "Untranslated requests and translation
requests that result in an address in the interrupt range will be
blocked with condition code LGN.4 or SGN.8."

This applies to both stage-1 and stage-2 IOMMU page table, move the
check from vtd_iova_to_slpte() to vtd_do_iommu_translate() so stage-1
page table could also be checked.

By this chance, update the comment with correct section number.

Suggested-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/i386/intel_iommu.c | 48 ++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

Comments

CLEMENT MATHIEU--DRIF Nov. 13, 2024, 6:55 a.m. UTC | #1
On 11/11/2024 09:34, Zhenzhong Duan wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> Per VT-d spec 4.1 section 3.15, "Untranslated requests and translation
> requests that result in an address in the interrupt range will be
> blocked with condition code LGN.4 or SGN.8."
>
> This applies to both stage-1 and stage-2 IOMMU page table, move the
> check from vtd_iova_to_slpte() to vtd_do_iommu_translate() so stage-1
> page table could also be checked.
>
> By this chance, update the comment with correct section number.
>
> Suggested-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu.c | 48 ++++++++++++++++++++++---------------------
>   1 file changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 4cc4d668fc..e651401db1 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1138,7 +1138,6 @@ static int vtd_iova_to_slpte(IntelIOMMUState *s, VTDContextEntry *ce,
>       uint32_t offset;
>       uint64_t slpte;
>       uint64_t access_right_check;
> -    uint64_t xlat, size;
>
>       if (!vtd_iova_sl_range_check(s, iova, ce, aw_bits, pasid)) {
>           error_report_once("%s: detected IOVA overflow (iova=0x%" PRIx64 ","
> @@ -1191,28 +1190,7 @@ static int vtd_iova_to_slpte(IntelIOMMUState *s, VTDContextEntry *ce,
>           level--;
>       }
>
> -    xlat = vtd_get_pte_addr(*slptep, aw_bits);
> -    size = ~vtd_pt_level_page_mask(level) + 1;
> -
> -    /*
> -     * From VT-d spec 3.14: Untranslated requests and translation
> -     * requests that result in an address in the interrupt range will be
> -     * blocked with condition code LGN.4 or SGN.8.
> -     */
> -    if ((xlat > VTD_INTERRUPT_ADDR_LAST ||
> -         xlat + size - 1 < VTD_INTERRUPT_ADDR_FIRST)) {
> -        return 0;
> -    } else {
> -        error_report_once("%s: xlat address is in interrupt range "
> -                          "(iova=0x%" PRIx64 ", level=0x%" PRIx32 ", "
> -                          "slpte=0x%" PRIx64 ", write=%d, "
> -                          "xlat=0x%" PRIx64 ", size=0x%" PRIx64 ", "
> -                          "pasid=0x%" PRIx32 ")",
> -                          __func__, iova, level, slpte, is_write,
> -                          xlat, size, pasid);
> -        return s->scalable_mode ? -VTD_FR_SM_INTERRUPT_ADDR :
> -                                  -VTD_FR_INTERRUPT_ADDR;
> -    }
> +    return 0;
>   }
>
>   typedef int (*vtd_page_walk_hook)(const IOMMUTLBEvent *event, void *private);
> @@ -2064,6 +2042,7 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
>       uint8_t access_flags;
>       bool rid2pasid = (pasid == PCI_NO_PASID) && s->root_scalable;
>       VTDIOTLBEntry *iotlb_entry;
> +    uint64_t xlat, size;
>
>       /*
>        * We have standalone memory region for interrupt addresses, we
> @@ -2173,6 +2152,29 @@ static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
>           ret_fr = vtd_iova_to_slpte(s, &ce, addr, is_write, &pte, &level,
>                                      &reads, &writes, s->aw_bits, pasid);
>       }
> +    if (!ret_fr) {
> +        xlat = vtd_get_pte_addr(pte, s->aw_bits);
> +        size = ~vtd_pt_level_page_mask(level) + 1;
> +
> +        /*
> +         * Per VT-d spec 4.1 section 3.15: Untranslated requests and translation
> +         * requests that result in an address in the interrupt range will be
> +         * blocked with condition code LGN.4 or SGN.8.
> +         */
> +        if ((xlat <= VTD_INTERRUPT_ADDR_LAST &&
> +             xlat + size - 1 >= VTD_INTERRUPT_ADDR_FIRST)) {
> +            error_report_once("%s: xlat address is in interrupt range "
> +                              "(iova=0x%" PRIx64 ", level=0x%" PRIx32 ", "
> +                              "pte=0x%" PRIx64 ", write=%d, "
> +                              "xlat=0x%" PRIx64 ", size=0x%" PRIx64 ", "
> +                              "pasid=0x%" PRIx32 ")",
> +                              __func__, addr, level, pte, is_write,
> +                              xlat, size, pasid);

Hi Zhenzhong,

Shouldn't we add the pgtt value to this trace as it can now be generated 
by both FL and SL?

Thanks
cmd
> +            ret_fr = s->scalable_mode ? -VTD_FR_SM_INTERRUPT_ADDR :
> +                                        -VTD_FR_INTERRUPT_ADDR;
> +        }
> +    }
> +
>       if (ret_fr) {
>           vtd_report_fault(s, -ret_fr, is_fpd_set, source_id,
>                            addr, is_write, pasid != PCI_NO_PASID, pasid);
> --
> 2.34.1
>
Duan, Zhenzhong Nov. 13, 2024, 8:49 a.m. UTC | #2
>-----Original Message-----
>From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>Sent: Wednesday, November 13, 2024 2:56 PM
>Subject: Re: [PATCH v5 08/20] intel_iommu: Check stage-1 translation result with
>interrupt range
>
>
>On 11/11/2024 09:34, Zhenzhong Duan wrote:
>> Caution: External email. Do not open attachments or click links, unless this
>email comes from a known sender and you know the content is safe.
>>
>>
>> Per VT-d spec 4.1 section 3.15, "Untranslated requests and translation
>> requests that result in an address in the interrupt range will be
>> blocked with condition code LGN.4 or SGN.8."
>>
>> This applies to both stage-1 and stage-2 IOMMU page table, move the
>> check from vtd_iova_to_slpte() to vtd_do_iommu_translate() so stage-1
>> page table could also be checked.
>>
>> By this chance, update the comment with correct section number.
>>
>> Suggested-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/i386/intel_iommu.c | 48 ++++++++++++++++++++++---------------------
>>   1 file changed, 25 insertions(+), 23 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 4cc4d668fc..e651401db1 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -1138,7 +1138,6 @@ static int vtd_iova_to_slpte(IntelIOMMUState *s,
>VTDContextEntry *ce,
>>       uint32_t offset;
>>       uint64_t slpte;
>>       uint64_t access_right_check;
>> -    uint64_t xlat, size;
>>
>>       if (!vtd_iova_sl_range_check(s, iova, ce, aw_bits, pasid)) {
>>           error_report_once("%s: detected IOVA overflow (iova=0x%" PRIx64 ","
>> @@ -1191,28 +1190,7 @@ static int vtd_iova_to_slpte(IntelIOMMUState *s,
>VTDContextEntry *ce,
>>           level--;
>>       }
>>
>> -    xlat = vtd_get_pte_addr(*slptep, aw_bits);
>> -    size = ~vtd_pt_level_page_mask(level) + 1;
>> -
>> -    /*
>> -     * From VT-d spec 3.14: Untranslated requests and translation
>> -     * requests that result in an address in the interrupt range will be
>> -     * blocked with condition code LGN.4 or SGN.8.
>> -     */
>> -    if ((xlat > VTD_INTERRUPT_ADDR_LAST ||
>> -         xlat + size - 1 < VTD_INTERRUPT_ADDR_FIRST)) {
>> -        return 0;
>> -    } else {
>> -        error_report_once("%s: xlat address is in interrupt range "
>> -                          "(iova=0x%" PRIx64 ", level=0x%" PRIx32 ", "
>> -                          "slpte=0x%" PRIx64 ", write=%d, "
>> -                          "xlat=0x%" PRIx64 ", size=0x%" PRIx64 ", "
>> -                          "pasid=0x%" PRIx32 ")",
>> -                          __func__, iova, level, slpte, is_write,
>> -                          xlat, size, pasid);
>> -        return s->scalable_mode ? -VTD_FR_SM_INTERRUPT_ADDR :
>> -                                  -VTD_FR_INTERRUPT_ADDR;
>> -    }
>> +    return 0;
>>   }
>>
>>   typedef int (*vtd_page_walk_hook)(const IOMMUTLBEvent *event, void
>*private);
>> @@ -2064,6 +2042,7 @@ static bool
>vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
>>       uint8_t access_flags;
>>       bool rid2pasid = (pasid == PCI_NO_PASID) && s->root_scalable;
>>       VTDIOTLBEntry *iotlb_entry;
>> +    uint64_t xlat, size;
>>
>>       /*
>>        * We have standalone memory region for interrupt addresses, we
>> @@ -2173,6 +2152,29 @@ static bool
>vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
>>           ret_fr = vtd_iova_to_slpte(s, &ce, addr, is_write, &pte, &level,
>>                                      &reads, &writes, s->aw_bits, pasid);
>>       }
>> +    if (!ret_fr) {
>> +        xlat = vtd_get_pte_addr(pte, s->aw_bits);
>> +        size = ~vtd_pt_level_page_mask(level) + 1;
>> +
>> +        /*
>> +         * Per VT-d spec 4.1 section 3.15: Untranslated requests and translation
>> +         * requests that result in an address in the interrupt range will be
>> +         * blocked with condition code LGN.4 or SGN.8.
>> +         */
>> +        if ((xlat <= VTD_INTERRUPT_ADDR_LAST &&
>> +             xlat + size - 1 >= VTD_INTERRUPT_ADDR_FIRST)) {
>> +            error_report_once("%s: xlat address is in interrupt range "
>> +                              "(iova=0x%" PRIx64 ", level=0x%" PRIx32 ", "
>> +                              "pte=0x%" PRIx64 ", write=%d, "
>> +                              "xlat=0x%" PRIx64 ", size=0x%" PRIx64 ", "
>> +                              "pasid=0x%" PRIx32 ")",
>> +                              __func__, addr, level, pte, is_write,
>> +                              xlat, size, pasid);
>
>Hi Zhenzhong,
>
>Shouldn't we add the pgtt value to this trace as it can now be generated
>by both FL and SL?

Hi Clement,

We don't always have a pgtt value to dump, e.g., when vIOMMU is in legacy mode.
Meanwhile we have other way to get pgtt if there is, e.g., from qemu cmdline.
Pgtt is also unrelated to the error itself, so I'd like to skip pgtt dump to be a bit simple.

Thanks
Zhenzhong
CLEMENT MATHIEU--DRIF Nov. 14, 2024, 6:04 a.m. UTC | #3
On 13/11/2024 09:49, Duan, Zhenzhong wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
>> -----Original Message-----
>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>> Sent: Wednesday, November 13, 2024 2:56 PM
>> Subject: Re: [PATCH v5 08/20] intel_iommu: Check stage-1 translation result with
>> interrupt range
>>
>>
>> On 11/11/2024 09:34, Zhenzhong Duan wrote:
>>> Caution: External email. Do not open attachments or click links, unless this
>> email comes from a known sender and you know the content is safe.
>>>
>>> Per VT-d spec 4.1 section 3.15, "Untranslated requests and translation
>>> requests that result in an address in the interrupt range will be
>>> blocked with condition code LGN.4 or SGN.8."
>>>
>>> This applies to both stage-1 and stage-2 IOMMU page table, move the
>>> check from vtd_iova_to_slpte() to vtd_do_iommu_translate() so stage-1
>>> page table could also be checked.
>>>
>>> By this chance, update the comment with correct section number.
>>>
>>> Suggested-by: Yi Liu <yi.l.liu@intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    hw/i386/intel_iommu.c | 48 ++++++++++++++++++++++---------------------
>>>    1 file changed, 25 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index 4cc4d668fc..e651401db1 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -1138,7 +1138,6 @@ static int vtd_iova_to_slpte(IntelIOMMUState *s,
>> VTDContextEntry *ce,
>>>        uint32_t offset;
>>>        uint64_t slpte;
>>>        uint64_t access_right_check;
>>> -    uint64_t xlat, size;
>>>
>>>        if (!vtd_iova_sl_range_check(s, iova, ce, aw_bits, pasid)) {
>>>            error_report_once("%s: detected IOVA overflow (iova=0x%" PRIx64 ","
>>> @@ -1191,28 +1190,7 @@ static int vtd_iova_to_slpte(IntelIOMMUState *s,
>> VTDContextEntry *ce,
>>>            level--;
>>>        }
>>>
>>> -    xlat = vtd_get_pte_addr(*slptep, aw_bits);
>>> -    size = ~vtd_pt_level_page_mask(level) + 1;
>>> -
>>> -    /*
>>> -     * From VT-d spec 3.14: Untranslated requests and translation
>>> -     * requests that result in an address in the interrupt range will be
>>> -     * blocked with condition code LGN.4 or SGN.8.
>>> -     */
>>> -    if ((xlat > VTD_INTERRUPT_ADDR_LAST ||
>>> -         xlat + size - 1 < VTD_INTERRUPT_ADDR_FIRST)) {
>>> -        return 0;
>>> -    } else {
>>> -        error_report_once("%s: xlat address is in interrupt range "
>>> -                          "(iova=0x%" PRIx64 ", level=0x%" PRIx32 ", "
>>> -                          "slpte=0x%" PRIx64 ", write=%d, "
>>> -                          "xlat=0x%" PRIx64 ", size=0x%" PRIx64 ", "
>>> -                          "pasid=0x%" PRIx32 ")",
>>> -                          __func__, iova, level, slpte, is_write,
>>> -                          xlat, size, pasid);
>>> -        return s->scalable_mode ? -VTD_FR_SM_INTERRUPT_ADDR :
>>> -                                  -VTD_FR_INTERRUPT_ADDR;
>>> -    }
>>> +    return 0;
>>>    }
>>>
>>>    typedef int (*vtd_page_walk_hook)(const IOMMUTLBEvent *event, void
>> *private);
>>> @@ -2064,6 +2042,7 @@ static bool
>> vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
>>>        uint8_t access_flags;
>>>        bool rid2pasid = (pasid == PCI_NO_PASID) && s->root_scalable;
>>>        VTDIOTLBEntry *iotlb_entry;
>>> +    uint64_t xlat, size;
>>>
>>>        /*
>>>         * We have standalone memory region for interrupt addresses, we
>>> @@ -2173,6 +2152,29 @@ static bool
>> vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
>>>            ret_fr = vtd_iova_to_slpte(s, &ce, addr, is_write, &pte, &level,
>>>                                       &reads, &writes, s->aw_bits, pasid);
>>>        }
>>> +    if (!ret_fr) {
>>> +        xlat = vtd_get_pte_addr(pte, s->aw_bits);
>>> +        size = ~vtd_pt_level_page_mask(level) + 1;
>>> +
>>> +        /*
>>> +         * Per VT-d spec 4.1 section 3.15: Untranslated requests and translation
>>> +         * requests that result in an address in the interrupt range will be
>>> +         * blocked with condition code LGN.4 or SGN.8.
>>> +         */
>>> +        if ((xlat <= VTD_INTERRUPT_ADDR_LAST &&
>>> +             xlat + size - 1 >= VTD_INTERRUPT_ADDR_FIRST)) {
>>> +            error_report_once("%s: xlat address is in interrupt range "
>>> +                              "(iova=0x%" PRIx64 ", level=0x%" PRIx32 ", "
>>> +                              "pte=0x%" PRIx64 ", write=%d, "
>>> +                              "xlat=0x%" PRIx64 ", size=0x%" PRIx64 ", "
>>> +                              "pasid=0x%" PRIx32 ")",
>>> +                              __func__, addr, level, pte, is_write,
>>> +                              xlat, size, pasid);
>> Hi Zhenzhong,
>>
>> Shouldn't we add the pgtt value to this trace as it can now be generated
>> by both FL and SL?
> Hi Clement,
>
> We don't always have a pgtt value to dump, e.g., when vIOMMU is in legacy mode.
> Meanwhile we have other way to get pgtt if there is, e.g., from qemu cmdline.
> Pgtt is also unrelated to the error itself, so I'd like to skip pgtt dump to be a bit simple.

Hi,
pgtt is initialized just above and is set to SLT when the vIOMMU is in 
legacy mode.
But it's fine, we can keep the patch as is!
Thanks

Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>



>
> Thanks
> Zhenzhong
>
Jason Wang Dec. 4, 2024, 2:11 a.m. UTC | #4
On Mon, Nov 11, 2024 at 4:38 PM Zhenzhong Duan <zhenzhong.duan@intel.com> wrote:
>
> Per VT-d spec 4.1 section 3.15, "Untranslated requests and translation
> requests that result in an address in the interrupt range will be
> blocked with condition code LGN.4 or SGN.8."
>
> This applies to both stage-1 and stage-2 IOMMU page table, move the
> check from vtd_iova_to_slpte() to vtd_do_iommu_translate() so stage-1
> page table could also be checked.
>
> By this chance, update the comment with correct section number.
>
> Suggested-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks
diff mbox series

Patch

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4cc4d668fc..e651401db1 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1138,7 +1138,6 @@  static int vtd_iova_to_slpte(IntelIOMMUState *s, VTDContextEntry *ce,
     uint32_t offset;
     uint64_t slpte;
     uint64_t access_right_check;
-    uint64_t xlat, size;
 
     if (!vtd_iova_sl_range_check(s, iova, ce, aw_bits, pasid)) {
         error_report_once("%s: detected IOVA overflow (iova=0x%" PRIx64 ","
@@ -1191,28 +1190,7 @@  static int vtd_iova_to_slpte(IntelIOMMUState *s, VTDContextEntry *ce,
         level--;
     }
 
-    xlat = vtd_get_pte_addr(*slptep, aw_bits);
-    size = ~vtd_pt_level_page_mask(level) + 1;
-
-    /*
-     * From VT-d spec 3.14: Untranslated requests and translation
-     * requests that result in an address in the interrupt range will be
-     * blocked with condition code LGN.4 or SGN.8.
-     */
-    if ((xlat > VTD_INTERRUPT_ADDR_LAST ||
-         xlat + size - 1 < VTD_INTERRUPT_ADDR_FIRST)) {
-        return 0;
-    } else {
-        error_report_once("%s: xlat address is in interrupt range "
-                          "(iova=0x%" PRIx64 ", level=0x%" PRIx32 ", "
-                          "slpte=0x%" PRIx64 ", write=%d, "
-                          "xlat=0x%" PRIx64 ", size=0x%" PRIx64 ", "
-                          "pasid=0x%" PRIx32 ")",
-                          __func__, iova, level, slpte, is_write,
-                          xlat, size, pasid);
-        return s->scalable_mode ? -VTD_FR_SM_INTERRUPT_ADDR :
-                                  -VTD_FR_INTERRUPT_ADDR;
-    }
+    return 0;
 }
 
 typedef int (*vtd_page_walk_hook)(const IOMMUTLBEvent *event, void *private);
@@ -2064,6 +2042,7 @@  static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
     uint8_t access_flags;
     bool rid2pasid = (pasid == PCI_NO_PASID) && s->root_scalable;
     VTDIOTLBEntry *iotlb_entry;
+    uint64_t xlat, size;
 
     /*
      * We have standalone memory region for interrupt addresses, we
@@ -2173,6 +2152,29 @@  static bool vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus,
         ret_fr = vtd_iova_to_slpte(s, &ce, addr, is_write, &pte, &level,
                                    &reads, &writes, s->aw_bits, pasid);
     }
+    if (!ret_fr) {
+        xlat = vtd_get_pte_addr(pte, s->aw_bits);
+        size = ~vtd_pt_level_page_mask(level) + 1;
+
+        /*
+         * Per VT-d spec 4.1 section 3.15: Untranslated requests and translation
+         * requests that result in an address in the interrupt range will be
+         * blocked with condition code LGN.4 or SGN.8.
+         */
+        if ((xlat <= VTD_INTERRUPT_ADDR_LAST &&
+             xlat + size - 1 >= VTD_INTERRUPT_ADDR_FIRST)) {
+            error_report_once("%s: xlat address is in interrupt range "
+                              "(iova=0x%" PRIx64 ", level=0x%" PRIx32 ", "
+                              "pte=0x%" PRIx64 ", write=%d, "
+                              "xlat=0x%" PRIx64 ", size=0x%" PRIx64 ", "
+                              "pasid=0x%" PRIx32 ")",
+                              __func__, addr, level, pte, is_write,
+                              xlat, size, pasid);
+            ret_fr = s->scalable_mode ? -VTD_FR_SM_INTERRUPT_ADDR :
+                                        -VTD_FR_INTERRUPT_ADDR;
+        }
+    }
+
     if (ret_fr) {
         vtd_report_fault(s, -ret_fr, is_fpd_set, source_id,
                          addr, is_write, pasid != PCI_NO_PASID, pasid);