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

[Duan, Zhenzhong]

>-----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