diff mbox series

[v2,01/17] intel_iommu: Use the latest fault reasons defined by spec

Message ID 20240805062727.2307552-2-zhenzhong.duan@intel.com (mailing list archive)
State New, archived
Headers show
Series intel_iommu: Enable stage-1 translation for emulated device | expand

Commit Message

Duan, Zhenzhong Aug. 5, 2024, 6:27 a.m. UTC 
From: Yu Zhang <yu.c.zhang@linux.intel.com>

Spec revision 3.0 or above defines more detailed fault reasons for
scalable mode. So introduce them into emulation code, see spec
section 7.1.2 for details.

Note spec revision has no relation with VERSION register, Guest
kernel should not use that register to judge what features are
supported. Instead cap/ecap bits should be checked.

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>
---
 hw/i386/intel_iommu_internal.h |  9 ++++++++-
 hw/i386/intel_iommu.c          | 25 ++++++++++++++++---------
 2 files changed, 24 insertions(+), 10 deletions(-)

Comments

Yi Liu Aug. 13, 2024, 10:57 a.m. UTC | #1 
On 2024/8/5 14:27, Zhenzhong Duan wrote:
> From: Yu Zhang <yu.c.zhang@linux.intel.com>
> 
> Spec revision 3.0 or above defines more detailed fault reasons for
> scalable mode. So introduce them into emulation code, see spec
> section 7.1.2 for details.
> 
> Note spec revision has no relation with VERSION register, Guest
> kernel should not use that register to judge what features are
> supported. Instead cap/ecap bits should be checked.
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>
> ---
>   hw/i386/intel_iommu_internal.h |  9 ++++++++-
>   hw/i386/intel_iommu.c          | 25 ++++++++++++++++---------
>   2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 5f32c36943..8fa27c7f3b 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -311,7 +311,14 @@ typedef enum VTDFaultReason {
>                                     * request while disabled */
>       VTD_FR_IR_SID_ERR = 0x26,   /* Invalid Source-ID */
>   
> -    VTD_FR_PASID_TABLE_INV = 0x58,  /*Invalid PASID table entry */
> +    /* PASID directory entry access failure */
> +    VTD_FR_PASID_DIR_ACCESS_ERR = 0x50,
> +    /* The Present(P) field of pasid directory entry is 0 */
> +    VTD_FR_PASID_DIR_ENTRY_P = 0x51,
> +    VTD_FR_PASID_TABLE_ACCESS_ERR = 0x58, /* PASID table entry access failure */
> +    /* The Present(P) field of pasid table entry is 0 */
> +    VTD_FR_PASID_ENTRY_P = 0x59,
> +    VTD_FR_PASID_TABLE_ENTRY_INV = 0x5b,  /*Invalid PASID table entry */

how about making the comment line aligned? Either one line or two lines.
Besides this, lgtm.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

>   
>       /* Output address in the interrupt address range for scalable mode */
>       VTD_FR_SM_INTERRUPT_ADDR = 0x87,
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 16d2885fcc..c52912f593 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -796,7 +796,7 @@ static int vtd_get_pdire_from_pdir_table(dma_addr_t pasid_dir_base,
>       addr = pasid_dir_base + index * entry_size;
>       if (dma_memory_read(&address_space_memory, addr,
>                           pdire, entry_size, MEMTXATTRS_UNSPECIFIED)) {
> -        return -VTD_FR_PASID_TABLE_INV;
> +        return -VTD_FR_PASID_DIR_ACCESS_ERR;
>       }
>   
>       pdire->val = le64_to_cpu(pdire->val);
> @@ -814,6 +814,7 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>                                             dma_addr_t addr,
>                                             VTDPASIDEntry *pe)
>   {
> +    uint8_t pgtt;
>       uint32_t index;
>       dma_addr_t entry_size;
>       X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
> @@ -823,7 +824,7 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>       addr = addr + index * entry_size;
>       if (dma_memory_read(&address_space_memory, addr,
>                           pe, entry_size, MEMTXATTRS_UNSPECIFIED)) {
> -        return -VTD_FR_PASID_TABLE_INV;
> +        return -VTD_FR_PASID_TABLE_ACCESS_ERR;
>       }
>       for (size_t i = 0; i < ARRAY_SIZE(pe->val); i++) {
>           pe->val[i] = le64_to_cpu(pe->val[i]);
> @@ -831,11 +832,13 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>   
>       /* Do translation type check */
>       if (!vtd_pe_type_check(x86_iommu, pe)) {
> -        return -VTD_FR_PASID_TABLE_INV;
> +        return -VTD_FR_PASID_TABLE_ENTRY_INV;
>       }
>   
> -    if (!vtd_is_level_supported(s, VTD_PE_GET_LEVEL(pe))) {
> -        return -VTD_FR_PASID_TABLE_INV;
> +    pgtt = VTD_PE_GET_TYPE(pe);
> +    if (pgtt == VTD_SM_PASID_ENTRY_SLT &&
> +        !vtd_is_level_supported(s, VTD_PE_GET_LEVEL(pe))) {
> +            return -VTD_FR_PASID_TABLE_ENTRY_INV;
>       }
>   
>       return 0;
> @@ -876,7 +879,7 @@ static int vtd_get_pe_from_pasid_table(IntelIOMMUState *s,
>       }
>   
>       if (!vtd_pdire_present(&pdire)) {
> -        return -VTD_FR_PASID_TABLE_INV;
> +        return -VTD_FR_PASID_DIR_ENTRY_P;
>       }
>   
>       ret = vtd_get_pe_from_pdire(s, pasid, &pdire, pe);
> @@ -885,7 +888,7 @@ static int vtd_get_pe_from_pasid_table(IntelIOMMUState *s,
>       }
>   
>       if (!vtd_pe_present(pe)) {
> -        return -VTD_FR_PASID_TABLE_INV;
> +        return -VTD_FR_PASID_ENTRY_P;
>       }
>   
>       return 0;
> @@ -938,7 +941,7 @@ static int vtd_ce_get_pasid_fpd(IntelIOMMUState *s,
>       }
>   
>       if (!vtd_pdire_present(&pdire)) {
> -        return -VTD_FR_PASID_TABLE_INV;
> +        return -VTD_FR_PASID_DIR_ENTRY_P;
>       }
>   
>       /*
> @@ -1795,7 +1798,11 @@ static const bool vtd_qualified_faults[] = {
>       [VTD_FR_ROOT_ENTRY_RSVD] = false,
>       [VTD_FR_PAGING_ENTRY_RSVD] = true,
>       [VTD_FR_CONTEXT_ENTRY_TT] = true,
> -    [VTD_FR_PASID_TABLE_INV] = false,
> +    [VTD_FR_PASID_DIR_ACCESS_ERR] = false,
> +    [VTD_FR_PASID_DIR_ENTRY_P] = true,
> +    [VTD_FR_PASID_TABLE_ACCESS_ERR] = false,
> +    [VTD_FR_PASID_ENTRY_P] = true,
> +    [VTD_FR_PASID_TABLE_ENTRY_INV] = true,
>       [VTD_FR_SM_INTERRUPT_ADDR] = true,
>       [VTD_FR_MAX] = false,
>   };
Duan, Zhenzhong Aug. 14, 2024, 2:30 a.m. UTC | #2 
>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Subject: Re: [PATCH v2 01/17] intel_iommu: Use the latest fault reasons
>defined by spec
>
>On 2024/8/5 14:27, Zhenzhong Duan wrote:
>> From: Yu Zhang <yu.c.zhang@linux.intel.com>
>>
>> Spec revision 3.0 or above defines more detailed fault reasons for
>> scalable mode. So introduce them into emulation code, see spec
>> section 7.1.2 for details.
>>
>> Note spec revision has no relation with VERSION register, Guest
>> kernel should not use that register to judge what features are
>> supported. Instead cap/ecap bits should be checked.
>>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> Reviewed-by: Clément Mathieu--Drif<clement.mathieu--drif@eviden.com>
>> ---
>>   hw/i386/intel_iommu_internal.h |  9 ++++++++-
>>   hw/i386/intel_iommu.c          | 25 ++++++++++++++++---------
>>   2 files changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu_internal.h
>b/hw/i386/intel_iommu_internal.h
>> index 5f32c36943..8fa27c7f3b 100644
>> --- a/hw/i386/intel_iommu_internal.h
>> +++ b/hw/i386/intel_iommu_internal.h
>> @@ -311,7 +311,14 @@ typedef enum VTDFaultReason {
>>                                     * request while disabled */
>>       VTD_FR_IR_SID_ERR = 0x26,   /* Invalid Source-ID */
>>
>> -    VTD_FR_PASID_TABLE_INV = 0x58,  /*Invalid PASID table entry */
>> +    /* PASID directory entry access failure */
>> +    VTD_FR_PASID_DIR_ACCESS_ERR = 0x50,
>> +    /* The Present(P) field of pasid directory entry is 0 */
>> +    VTD_FR_PASID_DIR_ENTRY_P = 0x51,
>> +    VTD_FR_PASID_TABLE_ACCESS_ERR = 0x58, /* PASID table entry
>access failure */
>> +    /* The Present(P) field of pasid table entry is 0 */
>> +    VTD_FR_PASID_ENTRY_P = 0x59,
>> +    VTD_FR_PASID_TABLE_ENTRY_INV = 0x5b,  /*Invalid PASID table entry
>*/
>
>how about making the comment line aligned? Either one line or two lines.

It looks the original rule is:
If one line exceeds 80 chars, split definition and comments to two lines.
If not, just use one line.

I'm following that rule.

Thanks
Zhenzhong

>Besides this, lgtm.
>
>Reviewed-by: Yi Liu <yi.l.liu@intel.com>
>
>>
>>       /* Output address in the interrupt address range for scalable mode */
>>       VTD_FR_SM_INTERRUPT_ADDR = 0x87,
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 16d2885fcc..c52912f593 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -796,7 +796,7 @@ static int
>vtd_get_pdire_from_pdir_table(dma_addr_t pasid_dir_base,
>>       addr = pasid_dir_base + index * entry_size;
>>       if (dma_memory_read(&address_space_memory, addr,
>>                           pdire, entry_size, MEMTXATTRS_UNSPECIFIED)) {
>> -        return -VTD_FR_PASID_TABLE_INV;
>> +        return -VTD_FR_PASID_DIR_ACCESS_ERR;
>>       }
>>
>>       pdire->val = le64_to_cpu(pdire->val);
>> @@ -814,6 +814,7 @@ static int
>vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>>                                             dma_addr_t addr,
>>                                             VTDPASIDEntry *pe)
>>   {
>> +    uint8_t pgtt;
>>       uint32_t index;
>>       dma_addr_t entry_size;
>>       X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>> @@ -823,7 +824,7 @@ static int
>vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>>       addr = addr + index * entry_size;
>>       if (dma_memory_read(&address_space_memory, addr,
>>                           pe, entry_size, MEMTXATTRS_UNSPECIFIED)) {
>> -        return -VTD_FR_PASID_TABLE_INV;
>> +        return -VTD_FR_PASID_TABLE_ACCESS_ERR;
>>       }
>>       for (size_t i = 0; i < ARRAY_SIZE(pe->val); i++) {
>>           pe->val[i] = le64_to_cpu(pe->val[i]);
>> @@ -831,11 +832,13 @@ static int
>vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>>
>>       /* Do translation type check */
>>       if (!vtd_pe_type_check(x86_iommu, pe)) {
>> -        return -VTD_FR_PASID_TABLE_INV;
>> +        return -VTD_FR_PASID_TABLE_ENTRY_INV;
>>       }
>>
>> -    if (!vtd_is_level_supported(s, VTD_PE_GET_LEVEL(pe))) {
>> -        return -VTD_FR_PASID_TABLE_INV;
>> +    pgtt = VTD_PE_GET_TYPE(pe);
>> +    if (pgtt == VTD_SM_PASID_ENTRY_SLT &&
>> +        !vtd_is_level_supported(s, VTD_PE_GET_LEVEL(pe))) {
>> +            return -VTD_FR_PASID_TABLE_ENTRY_INV;
>>       }
>>
>>       return 0;
>> @@ -876,7 +879,7 @@ static int
>vtd_get_pe_from_pasid_table(IntelIOMMUState *s,
>>       }
>>
>>       if (!vtd_pdire_present(&pdire)) {
>> -        return -VTD_FR_PASID_TABLE_INV;
>> +        return -VTD_FR_PASID_DIR_ENTRY_P;
>>       }
>>
>>       ret = vtd_get_pe_from_pdire(s, pasid, &pdire, pe);
>> @@ -885,7 +888,7 @@ static int
>vtd_get_pe_from_pasid_table(IntelIOMMUState *s,
>>       }
>>
>>       if (!vtd_pe_present(pe)) {
>> -        return -VTD_FR_PASID_TABLE_INV;
>> +        return -VTD_FR_PASID_ENTRY_P;
>>       }
>>
>>       return 0;
>> @@ -938,7 +941,7 @@ static int vtd_ce_get_pasid_fpd(IntelIOMMUState
>*s,
>>       }
>>
>>       if (!vtd_pdire_present(&pdire)) {
>> -        return -VTD_FR_PASID_TABLE_INV;
>> +        return -VTD_FR_PASID_DIR_ENTRY_P;
>>       }
>>
>>       /*
>> @@ -1795,7 +1798,11 @@ static const bool vtd_qualified_faults[] = {
>>       [VTD_FR_ROOT_ENTRY_RSVD] = false,
>>       [VTD_FR_PAGING_ENTRY_RSVD] = true,
>>       [VTD_FR_CONTEXT_ENTRY_TT] = true,
>> -    [VTD_FR_PASID_TABLE_INV] = false,
>> +    [VTD_FR_PASID_DIR_ACCESS_ERR] = false,
>> +    [VTD_FR_PASID_DIR_ENTRY_P] = true,
>> +    [VTD_FR_PASID_TABLE_ACCESS_ERR] = false,
>> +    [VTD_FR_PASID_ENTRY_P] = true,
>> +    [VTD_FR_PASID_TABLE_ENTRY_INV] = true,
>>       [VTD_FR_SM_INTERRUPT_ADDR] = true,
>>       [VTD_FR_MAX] = false,
>>   };
>
>--
>Regards,
>Yi Liu
diff mbox series

Patch

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index 5f32c36943..8fa27c7f3b 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -311,7 +311,14 @@  typedef enum VTDFaultReason {
                                   * request while disabled */
     VTD_FR_IR_SID_ERR = 0x26,   /* Invalid Source-ID */
 
-    VTD_FR_PASID_TABLE_INV = 0x58,  /*Invalid PASID table entry */
+    /* PASID directory entry access failure */
+    VTD_FR_PASID_DIR_ACCESS_ERR = 0x50,
+    /* The Present(P) field of pasid directory entry is 0 */
+    VTD_FR_PASID_DIR_ENTRY_P = 0x51,
+    VTD_FR_PASID_TABLE_ACCESS_ERR = 0x58, /* PASID table entry access failure */
+    /* The Present(P) field of pasid table entry is 0 */
+    VTD_FR_PASID_ENTRY_P = 0x59,
+    VTD_FR_PASID_TABLE_ENTRY_INV = 0x5b,  /*Invalid PASID table entry */
 
     /* Output address in the interrupt address range for scalable mode */
     VTD_FR_SM_INTERRUPT_ADDR = 0x87,
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 16d2885fcc..c52912f593 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -796,7 +796,7 @@  static int vtd_get_pdire_from_pdir_table(dma_addr_t pasid_dir_base,
     addr = pasid_dir_base + index * entry_size;
     if (dma_memory_read(&address_space_memory, addr,
                         pdire, entry_size, MEMTXATTRS_UNSPECIFIED)) {
-        return -VTD_FR_PASID_TABLE_INV;
+        return -VTD_FR_PASID_DIR_ACCESS_ERR;
     }
 
     pdire->val = le64_to_cpu(pdire->val);
@@ -814,6 +814,7 @@  static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
                                           dma_addr_t addr,
                                           VTDPASIDEntry *pe)
 {
+    uint8_t pgtt;
     uint32_t index;
     dma_addr_t entry_size;
     X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
@@ -823,7 +824,7 @@  static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
     addr = addr + index * entry_size;
     if (dma_memory_read(&address_space_memory, addr,
                         pe, entry_size, MEMTXATTRS_UNSPECIFIED)) {
-        return -VTD_FR_PASID_TABLE_INV;
+        return -VTD_FR_PASID_TABLE_ACCESS_ERR;
     }
     for (size_t i = 0; i < ARRAY_SIZE(pe->val); i++) {
         pe->val[i] = le64_to_cpu(pe->val[i]);
@@ -831,11 +832,13 @@  static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
 
     /* Do translation type check */
     if (!vtd_pe_type_check(x86_iommu, pe)) {
-        return -VTD_FR_PASID_TABLE_INV;
+        return -VTD_FR_PASID_TABLE_ENTRY_INV;
     }
 
-    if (!vtd_is_level_supported(s, VTD_PE_GET_LEVEL(pe))) {
-        return -VTD_FR_PASID_TABLE_INV;
+    pgtt = VTD_PE_GET_TYPE(pe);
+    if (pgtt == VTD_SM_PASID_ENTRY_SLT &&
+        !vtd_is_level_supported(s, VTD_PE_GET_LEVEL(pe))) {
+            return -VTD_FR_PASID_TABLE_ENTRY_INV;
     }
 
     return 0;
@@ -876,7 +879,7 @@  static int vtd_get_pe_from_pasid_table(IntelIOMMUState *s,
     }
 
     if (!vtd_pdire_present(&pdire)) {
-        return -VTD_FR_PASID_TABLE_INV;
+        return -VTD_FR_PASID_DIR_ENTRY_P;
     }
 
     ret = vtd_get_pe_from_pdire(s, pasid, &pdire, pe);
@@ -885,7 +888,7 @@  static int vtd_get_pe_from_pasid_table(IntelIOMMUState *s,
     }
 
     if (!vtd_pe_present(pe)) {
-        return -VTD_FR_PASID_TABLE_INV;
+        return -VTD_FR_PASID_ENTRY_P;
     }
 
     return 0;
@@ -938,7 +941,7 @@  static int vtd_ce_get_pasid_fpd(IntelIOMMUState *s,
     }
 
     if (!vtd_pdire_present(&pdire)) {
-        return -VTD_FR_PASID_TABLE_INV;
+        return -VTD_FR_PASID_DIR_ENTRY_P;
     }
 
     /*
@@ -1795,7 +1798,11 @@  static const bool vtd_qualified_faults[] = {
     [VTD_FR_ROOT_ENTRY_RSVD] = false,
     [VTD_FR_PAGING_ENTRY_RSVD] = true,
     [VTD_FR_CONTEXT_ENTRY_TT] = true,
-    [VTD_FR_PASID_TABLE_INV] = false,
+    [VTD_FR_PASID_DIR_ACCESS_ERR] = false,
+    [VTD_FR_PASID_DIR_ENTRY_P] = true,
+    [VTD_FR_PASID_TABLE_ACCESS_ERR] = false,
+    [VTD_FR_PASID_ENTRY_P] = true,
+    [VTD_FR_PASID_TABLE_ENTRY_INV] = true,
     [VTD_FR_SM_INTERRUPT_ADDR] = true,
     [VTD_FR_MAX] = false,
 };