RE: [PATCH] intel_iommu: Use the latest fault reasons defined by spec

[Liu, Yi L]

> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
> Sent: Friday, May 17, 2024 9:13 PM
> 
> Hi Zhenzhong
> 
> On 17/05/2024 12:23, 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.
> >
> >
> > From: Yu Zhang <yu.c.zhang@linux.intel.com>
> >
> > Currently we use only VTD_FR_PASID_TABLE_INV as fault reason.
> > Update with more detailed fault reasons listed in VT-d spec 7.2.3.
> >
> > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> > ---
> >   hw/i386/intel_iommu_internal.h |  8 +++++++-
> >   hw/i386/intel_iommu.c          | 25 ++++++++++++++++---------
> >   2 files changed, 23 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> > index f8cf99bddf..666e2cf2ce 100644
> > --- a/hw/i386/intel_iommu_internal.h
> > +++ b/hw/i386/intel_iommu_internal.h
> > @@ -311,7 +311,13 @@ 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 */
> > +    VTD_FR_PASID_ENTRY_P = 0x59, /* The Present(P) field of pasidt-entry 
> > is 0 */
> s/pasidt/pasid

Per spec, it is pasid table entry. So Zhenzhong may need to use the same word
With the line below. E.g. PASID Table entry.

Regards,
Yi Liu

> > +    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 cc8e59674e..0951ebb71d 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -771,7 +771,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);
> > @@ -789,6 +789,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);
> > @@ -798,7 +799,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]);
> > @@ -806,11 +807,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;
> > @@ -851,7 +854,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);
> > @@ -860,7 +863,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;
> > @@ -913,7 +916,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;
> >       }
> >
> >       /*
> > @@ -1770,7 +1773,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,
> >   };
> > --
> > 2.34.1
> >
> >
> lgtm