intel_iommu: Use the latest fault reasons defined by spec

Commit Message

Duan, Zhenzhong May 17, 2024, 10:23 a.m. UTC

From: Yu Zhang <[e-mail]>

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 <[e-mail]>
Signed-off-by: Zhenzhong Duan <[e-mail]>
---
 hw/i386/intel_iommu_internal.h |  8 +++++++-
 hw/i386/intel_iommu.c          | 25 ++++++++++++++++---------
 2 files changed, 23 insertions(+), 10 deletions(-)

Comments

CLEMENT MATHIEU--DRIF May 17, 2024, 1:13 p.m. UTC | #1

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 <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Zhenzhong Duan <[e-mail]>
> ---
>   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
> +    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

Yi Liu May 19, 2024, 5:08 a.m. UTC | #2

> From: CLEMENT MATHIEU--DRIF <[e-mail]>
> 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 <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Zhenzhong Duan <[e-mail]>
> > ---
> >   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

CLEMENT MATHIEU--DRIF May 19, 2024, 8:21 a.m. UTC | #3

> From: CLEMENT MATHIEU--DRIF <[e-mail]>
> 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 <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Zhenzhong Duan <[e-mail]>
> > ---
> >   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


Ok fine

regards,
>cmd


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

Jason Wang May 20, 2024, 12:43 a.m. UTC | #4

On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan <[e-mail]> wrote:
>
> From: Yu Zhang <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Zhenzhong Duan <[e-mail]>
> ---

I wonder if this could be noticed by the guest or not. If yes should
we consider starting to add thing like version to vtd emulation code?

Thanks

Duan, Zhenzhong May 20, 2024, 3:11 a.m. UTC | #5

>-----Original Message-----
>From: Liu, Yi L <[e-mail]>
>Subject: RE: [PATCH] intel_iommu: Use the latest fault reasons defined by
>spec
>
>> From: CLEMENT MATHIEU--DRIF <[e-mail]>
>> 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 <[e-mail]>
>> >
>> > 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 <[e-mail]>
>> > Signed-off-by: Zhenzhong Duan <[e-mail]>
>> > ---
>> >   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.

Yes, will fix.

Thanks
Zhenzhong

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

Duan, Zhenzhong May 20, 2024, 3:41 a.m. UTC | #6

>-----Original Message-----
>From: Jason Wang <[e-mail]>
>Sent: Monday, May 20, 2024 8:44 AM
>To: Duan, Zhenzhong <[e-mail]>
>Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng, Chao P
><[e-mail]>; Yu Zhang <[e-mail]>; Michael
>S. Tsirkin <[e-mail]>; Paolo Bonzini <[e-mail]>;
>Richard Henderson <[e-mail]>; Eduardo Habkost
><[e-mail]>; Marcel Apfelbaum <[e-mail]>
>Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
>spec
>
>On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
><[e-mail]> wrote:
>>
>> From: Yu Zhang <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Zhenzhong Duan <[e-mail]>
>> ---
>
>I wonder if this could be noticed by the guest or not. If yes should
>we consider starting to add thing like version to vtd emulation code?

Kernel only dumps the reason like below:

DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault addr 0x1234600000 [fault reason 0x71] SM: Present bit in first-level paging entry is clear

Maybe bump 1.0 -> 1.1?
My understanding version number is only informational and is far from
accurate to mark if a feature supported. Driver should check cap/ecap
bits instead.

Thanks
Zhenzhong

Yi Liu May 20, 2024, 4:15 a.m. UTC | #7

> From: Duan, Zhenzhong <[e-mail]>
> Sent: Monday, May 20, 2024 11:41 AM
> 
> 
> 
> >-----Original Message-----
> >From: Jason Wang <[e-mail]>
> >Sent: Monday, May 20, 2024 8:44 AM
> >To: Duan, Zhenzhong <[e-mail]>
> >Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng, Chao P
> ><[e-mail]>; Yu Zhang <[e-mail]>; Michael
> >S. Tsirkin <[e-mail]>; Paolo Bonzini <[e-mail]>;
> >Richard Henderson <[e-mail]>; Eduardo Habkost
> ><[e-mail]>; Marcel Apfelbaum <[e-mail]>
> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
> >spec
> >
> >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
> ><[e-mail]> wrote:
> >>
> >> From: Yu Zhang <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> Signed-off-by: Zhenzhong Duan <[e-mail]>
> >> ---
> >
> >I wonder if this could be noticed by the guest or not. If yes should
> >we consider starting to add thing like version to vtd emulation code?
>
> Kernel only dumps the reason like below:
> 
> DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault addr 0x1234600000
> [fault reason 0x71] SM: Present bit in first-level paging entry is clear

Yes, guest kernel would notice it as the fault would be injected to vm. 

> Maybe bump 1.0 -> 1.1?
> My understanding version number is only informational and is far from
> accurate to mark if a feature supported. Driver should check cap/ecap
> bits instead.

Should the version ID here be aligned with VT-d spec? If yes, it should
be 3.0 as the scalable mode was introduced in spec 3.0. And the fault
code was redefined together with the introduction of this translation
mode. Below is the a snippet from the change log of VT-d spec.

June 2018 3.0
• Removed all text related to Extended-Mode.
• Added support for scalable-mode translation for DMA Remapping, that enables PASIDgranular first-level, second-level, nested and pass-through translation functions.
• Widen invalidation queue descriptors and page request queue descriptors from 128 bits
to 256 bits and redefined page-request and page-response descriptors.
• Listed all fault conditions in a unified table and described DMA Remapping hardware
behavior under each condition. Assigned new code for each fault condition in scalablemode operation.
• Added support for Accessed/Dirty (A/D) bits in second-level translation.
• Added support for submitting commands and receiving response from virtual DMA
Remapping hardware.
• Added a table on snooping behavior and memory type of hardware access to various
remapping structures as appendix.
• Move Page Request Overflow (PRO) fault reporting from Fault Status register
(FSTS_REG) to Page Request Status register (PRS_REG).

Regards.
Yi Liu

Duan, Zhenzhong May 20, 2024, 7:55 a.m. UTC | #8

>-----Original Message-----
>From: Liu, Yi L <[e-mail]>
>Subject: RE: [PATCH] intel_iommu: Use the latest fault reasons defined by
>spec
>
>> From: Duan, Zhenzhong <[e-mail]>
>> Sent: Monday, May 20, 2024 11:41 AM
>>
>>
>>
>> >-----Original Message-----
>> >From: Jason Wang <[e-mail]>
>> >Sent: Monday, May 20, 2024 8:44 AM
>> >To: Duan, Zhenzhong <[e-mail]>
>> >Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng, Chao P
>> ><[e-mail]>; Yu Zhang <[e-mail]>;
>Michael
>> >S. Tsirkin <[e-mail]>; Paolo Bonzini <[e-mail]>;
>> >Richard Henderson <[e-mail]>; Eduardo Habkost
>> ><[e-mail]>; Marcel Apfelbaum
><[e-mail]>
>> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
>> >spec
>> >
>> >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
>> ><[e-mail]> wrote:
>> >>
>> >> From: Yu Zhang <[e-mail]>
>> >>
>> >> 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 <[e-mail]>
>> >> Signed-off-by: Zhenzhong Duan <[e-mail]>
>> >> ---
>> >
>> >I wonder if this could be noticed by the guest or not. If yes should
>> >we consider starting to add thing like version to vtd emulation code?
>>
>> Kernel only dumps the reason like below:
>>
>> DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault addr
>0x1234600000
>> [fault reason 0x71] SM: Present bit in first-level paging entry is clear
>
>Yes, guest kernel would notice it as the fault would be injected to vm.
>
>> Maybe bump 1.0 -> 1.1?
>> My understanding version number is only informational and is far from
>> accurate to mark if a feature supported. Driver should check cap/ecap
>> bits instead.
>
>Should the version ID here be aligned with VT-d spec? If yes, it should
>be 3.0 as the scalable mode was introduced in spec 3.0. And the fault
>code was redefined together with the introduction of this translation
>mode. Below is the a snippet from the change log of VT-d spec.

OK, then 3.0 is a better choice. Will update version.
For Jason's question, even though more fault reasons are added,
but the reason numbers are still backward compatible,
so no need to define reasons per version.

Thanks
Zhenzhong

>
>June 2018 3.0
>• Removed all text related to Extended-Mode.
>• Added support for scalable-mode translation for DMA Remapping, that
>enables PASIDgranular first-level, second-level, nested and pass-through
>translation functions.
>• Widen invalidation queue descriptors and page request queue descriptors
>from 128 bits
>to 256 bits and redefined page-request and page-response descriptors.
>• Listed all fault conditions in a unified table and described DMA Remapping
>hardware
>behavior under each condition. Assigned new code for each fault condition in
>scalablemode operation.
>• Added support for Accessed/Dirty (A/D) bits in second-level translation.
>• Added support for submitting commands and receiving response from
>virtual DMA
>Remapping hardware.
>• Added a table on snooping behavior and memory type of hardware access
>to various
>remapping structures as appendix.
>• Move Page Request Overflow (PRO) fault reporting from Fault Status
>register
>(FSTS_REG) to Page Request Status register (PRS_REG).
>
>Regards.
>Yi Liu

Jason Wang May 21, 2024, 2:48 a.m. UTC | #9

On Mon, May 20, 2024 at 12:15 PM Liu, Yi L <[e-mail]> wrote:
>
> > From: Duan, Zhenzhong <[e-mail]>
> > Sent: Monday, May 20, 2024 11:41 AM
> >
> >
> >
> > >-----Original Message-----
> > >From: Jason Wang <[e-mail]>
> > >Sent: Monday, May 20, 2024 8:44 AM
> > >To: Duan, Zhenzhong <[e-mail]>
> > >Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng, Chao P
> > ><[e-mail]>; Yu Zhang <[e-mail]>; Michael
> > >S. Tsirkin <[e-mail]>; Paolo Bonzini <[e-mail]>;
> > >Richard Henderson <[e-mail]>; Eduardo Habkost
> > ><[e-mail]>; Marcel Apfelbaum <[e-mail]>
> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
> > >spec
> > >
> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
> > ><[e-mail]> wrote:
> > >>
> > >> From: Yu Zhang <[e-mail]>
> > >>
> > >> 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 <[e-mail]>
> > >> Signed-off-by: Zhenzhong Duan <[e-mail]>
> > >> ---
> > >
> > >I wonder if this could be noticed by the guest or not. If yes should
> > >we consider starting to add thing like version to vtd emulation code?
> >
> > Kernel only dumps the reason like below:
> >
> > DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault addr 0x1234600000
> > [fault reason 0x71] SM: Present bit in first-level paging entry is clear
>
> Yes, guest kernel would notice it as the fault would be injected to vm.
>
> > Maybe bump 1.0 -> 1.1?
> > My understanding version number is only informational and is far from
> > accurate to mark if a feature supported. Driver should check cap/ecap
> > bits instead.
>
> Should the version ID here be aligned with VT-d spec?

Probably, this might be something that could be noticed by the
management to migration compatibility.

> If yes, it should
> be 3.0 as the scalable mode was introduced in spec 3.0. And the fault
> code was redefined together with the introduction of this translation
> mode. Below is the a snippet from the change log of VT-d spec.
>
> June 2018 3.0
> • Removed all text related to Extended-Mode.
> • Added support for scalable-mode translation for DMA Remapping, that enables PASIDgranular first-level, second-level, nested and pass-through translation functions.
> • Widen invalidation queue descriptors and page request queue descriptors from 128 bits
> to 256 bits and redefined page-request and page-response descriptors.
> • Listed all fault conditions in a unified table and described DMA Remapping hardware
> behavior under each condition. Assigned new code for each fault condition in scalablemode operation.
> • Added support for Accessed/Dirty (A/D) bits in second-level translation.
> • Added support for submitting commands and receiving response from virtual DMA
> Remapping hardware.
> • Added a table on snooping behavior and memory type of hardware access to various
> remapping structures as appendix.
> • Move Page Request Overflow (PRO) fault reporting from Fault Status register
> (FSTS_REG) to Page Request Status register (PRS_REG).
>
> Regards.
> Yi Liu

Thanks

Duan, Zhenzhong May 21, 2024, 10:25 a.m. UTC | #10

>-----Original Message-----
>From: Jason Wang <[e-mail]>
>Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
>spec
>
>On Mon, May 20, 2024 at 12:15 PM Liu, Yi L <[e-mail]> wrote:
>>
>> > From: Duan, Zhenzhong <[e-mail]>
>> > Sent: Monday, May 20, 2024 11:41 AM
>> >
>> >
>> >
>> > >-----Original Message-----
>> > >From: Jason Wang <[e-mail]>
>> > >Sent: Monday, May 20, 2024 8:44 AM
>> > >To: Duan, Zhenzhong <[e-mail]>
>> > >Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng, Chao
>P
>> > ><[e-mail]>; Yu Zhang <[e-mail]>;
>Michael
>> > >S. Tsirkin <[e-mail]>; Paolo Bonzini <[e-mail]>;
>> > >Richard Henderson <[e-mail]>; Eduardo Habkost
>> > ><[e-mail]>; Marcel Apfelbaum
><[e-mail]>
>> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined
>by
>> > >spec
>> > >
>> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
>> > ><[e-mail]> wrote:
>> > >>
>> > >> From: Yu Zhang <[e-mail]>
>> > >>
>> > >> 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 <[e-mail]>
>> > >> Signed-off-by: Zhenzhong Duan <[e-mail]>
>> > >> ---
>> > >
>> > >I wonder if this could be noticed by the guest or not. If yes should
>> > >we consider starting to add thing like version to vtd emulation code?
>> >
>> > Kernel only dumps the reason like below:
>> >
>> > DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault addr
>0x1234600000
>> > [fault reason 0x71] SM: Present bit in first-level paging entry is clear
>>
>> Yes, guest kernel would notice it as the fault would be injected to vm.
>>
>> > Maybe bump 1.0 -> 1.1?
>> > My understanding version number is only informational and is far from
>> > accurate to mark if a feature supported. Driver should check cap/ecap
>> > bits instead.
>>
>> Should the version ID here be aligned with VT-d spec?
>
>Probably, this might be something that could be noticed by the
>management to migration compatibility.

Could you elaborate what we need to do for migration compatibility?
I see version is already exported so libvirt can query it, see:

    DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),

Thanks
Zhenzhong

>
>> If yes, it should
>> be 3.0 as the scalable mode was introduced in spec 3.0. And the fault
>> code was redefined together with the introduction of this translation
>> mode. Below is the a snippet from the change log of VT-d spec.
>>
>> June 2018 3.0
>> • Removed all text related to Extended-Mode.
>> • Added support for scalable-mode translation for DMA Remapping, that
>enables PASIDgranular first-level, second-level, nested and pass-through
>translation functions.
>> • Widen invalidation queue descriptors and page request queue
>descriptors from 128 bits
>> to 256 bits and redefined page-request and page-response descriptors.
>> • Listed all fault conditions in a unified table and described DMA
>Remapping hardware
>> behavior under each condition. Assigned new code for each fault condition
>in scalablemode operation.
>> • Added support for Accessed/Dirty (A/D) bits in second-level translation.
>> • Added support for submitting commands and receiving response from
>virtual DMA
>> Remapping hardware.
>> • Added a table on snooping behavior and memory type of hardware
>access to various
>> remapping structures as appendix.
>> • Move Page Request Overflow (PRO) fault reporting from Fault Status
>register
>> (FSTS_REG) to Page Request Status register (PRS_REG).
>>
>> Regards.
>> Yi Liu
>
>Thanks

Jason Wang May 22, 2024, 8:12 a.m. UTC | #11

On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong
<[e-mail]> wrote:
>
>
>
> >-----Original Message-----
> >From: Jason Wang <[e-mail]>
> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
> >spec
> >
> >On Mon, May 20, 2024 at 12:15 PM Liu, Yi L <[e-mail]> wrote:
> >>
> >> > From: Duan, Zhenzhong <[e-mail]>
> >> > Sent: Monday, May 20, 2024 11:41 AM
> >> >
> >> >
> >> >
> >> > >-----Original Message-----
> >> > >From: Jason Wang <[e-mail]>
> >> > >Sent: Monday, May 20, 2024 8:44 AM
> >> > >To: Duan, Zhenzhong <[e-mail]>
> >> > >Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng, Chao
> >P
> >> > ><[e-mail]>; Yu Zhang <[e-mail]>;
> >Michael
> >> > >S. Tsirkin <[e-mail]>; Paolo Bonzini <[e-mail]>;
> >> > >Richard Henderson <[e-mail]>; Eduardo Habkost
> >> > ><[e-mail]>; Marcel Apfelbaum
> ><[e-mail]>
> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined
> >by
> >> > >spec
> >> > >
> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
> >> > ><[e-mail]> wrote:
> >> > >>
> >> > >> From: Yu Zhang <[e-mail]>
> >> > >>
> >> > >> 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 <[e-mail]>
> >> > >> Signed-off-by: Zhenzhong Duan <[e-mail]>
> >> > >> ---
> >> > >
> >> > >I wonder if this could be noticed by the guest or not. If yes should
> >> > >we consider starting to add thing like version to vtd emulation code?
> >> >
> >> > Kernel only dumps the reason like below:
> >> >
> >> > DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault addr
> >0x1234600000
> >> > [fault reason 0x71] SM: Present bit in first-level paging entry is clear
> >>
> >> Yes, guest kernel would notice it as the fault would be injected to vm.
> >>
> >> > Maybe bump 1.0 -> 1.1?
> >> > My understanding version number is only informational and is far from
> >> > accurate to mark if a feature supported. Driver should check cap/ecap
> >> > bits instead.
> >>
> >> Should the version ID here be aligned with VT-d spec?
> >
> >Probably, this might be something that could be noticed by the
> >management to migration compatibility.
>
> Could you elaborate what we need to do for migration compatibility?
> I see version is already exported so libvirt can query it, see:
>
>     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),

It is the Qemu command line parameters not the version of the vmstate.

For example -device intel-iommu,version=3.0

Qemu then knows it should behave as 3.0.

Thanks

>
> Thanks
> Zhenzhong
>
> >
> >> If yes, it should
> >> be 3.0 as the scalable mode was introduced in spec 3.0. And the fault
> >> code was redefined together with the introduction of this translation
> >> mode. Below is the a snippet from the change log of VT-d spec.
> >>
> >> June 2018 3.0
> >> • Removed all text related to Extended-Mode.
> >> • Added support for scalable-mode translation for DMA Remapping, that
> >enables PASIDgranular first-level, second-level, nested and pass-through
> >translation functions.
> >> • Widen invalidation queue descriptors and page request queue
> >descriptors from 128 bits
> >> to 256 bits and redefined page-request and page-response descriptors.
> >> • Listed all fault conditions in a unified table and described DMA
> >Remapping hardware
> >> behavior under each condition. Assigned new code for each fault condition
> >in scalablemode operation.
> >> • Added support for Accessed/Dirty (A/D) bits in second-level translation.
> >> • Added support for submitting commands and receiving response from
> >virtual DMA
> >> Remapping hardware.
> >> • Added a table on snooping behavior and memory type of hardware
> >access to various
> >> remapping structures as appendix.
> >> • Move Page Request Overflow (PRO) fault reporting from Fault Status
> >register
> >> (FSTS_REG) to Page Request Status register (PRS_REG).
> >>
> >> Regards.
> >> Yi Liu
> >
> >Thanks
>

Duan, Zhenzhong May 24, 2024, 8:40 a.m. UTC | #12

>-----Original Message-----
>From: Jason Wang <[e-mail]>
>Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
>spec
>
>On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong
><[e-mail]> wrote:
>>
>>
>>
>> >-----Original Message-----
>> >From: Jason Wang <[e-mail]>
>> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
>> >spec
>> >
>> >On Mon, May 20, 2024 at 12:15 PM Liu, Yi L <[e-mail]> wrote:
>> >>
>> >> > From: Duan, Zhenzhong <[e-mail]>
>> >> > Sent: Monday, May 20, 2024 11:41 AM
>> >> >
>> >> >
>> >> >
>> >> > >-----Original Message-----
>> >> > >From: Jason Wang <[e-mail]>
>> >> > >Sent: Monday, May 20, 2024 8:44 AM
>> >> > >To: Duan, Zhenzhong <[e-mail]>
>> >> > >Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng,
>Chao
>> >P
>> >> > ><[e-mail]>; Yu Zhang <[e-mail]>;
>> >Michael
>> >> > >S. Tsirkin <[e-mail]>; Paolo Bonzini
><[e-mail]>;
>> >> > >Richard Henderson <[e-mail]>; Eduardo
>Habkost
>> >> > ><[e-mail]>; Marcel Apfelbaum
>> ><[e-mail]>
>> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons
>defined
>> >by
>> >> > >spec
>> >> > >
>> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
>> >> > ><[e-mail]> wrote:
>> >> > >>
>> >> > >> From: Yu Zhang <[e-mail]>
>> >> > >>
>> >> > >> 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 <[e-mail]>
>> >> > >> Signed-off-by: Zhenzhong Duan <[e-mail]>
>> >> > >> ---
>> >> > >
>> >> > >I wonder if this could be noticed by the guest or not. If yes should
>> >> > >we consider starting to add thing like version to vtd emulation code?
>> >> >
>> >> > Kernel only dumps the reason like below:
>> >> >
>> >> > DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault addr
>> >0x1234600000
>> >> > [fault reason 0x71] SM: Present bit in first-level paging entry is clear
>> >>
>> >> Yes, guest kernel would notice it as the fault would be injected to vm.
>> >>
>> >> > Maybe bump 1.0 -> 1.1?
>> >> > My understanding version number is only informational and is far
>from
>> >> > accurate to mark if a feature supported. Driver should check cap/ecap
>> >> > bits instead.
>> >>
>> >> Should the version ID here be aligned with VT-d spec?
>> >
>> >Probably, this might be something that could be noticed by the
>> >management to migration compatibility.
>>
>> Could you elaborate what we need to do for migration compatibility?
>> I see version is already exported so libvirt can query it, see:
>>
>>     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
>
>It is the Qemu command line parameters not the version of the vmstate.
>
>For example -device intel-iommu,version=3.0
>
>Qemu then knows it should behave as 3.0.

So you want to bump vtd_vmstate.version?

In fact, this series change intel_iommu property from x-scalable-mode=["on"|"off"]"
to x-scalable-mode=["legacy"|"modern"|"off"]".

My understanding management app should use same qemu cmdline
in source and destination, so compatibility is already guaranteed even if
we don't bump vtd_vmstate.version.

Thanks
Zhenzhong

Jason Wang May 27, 2024, 3:21 a.m. UTC | #13

On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong
<[e-mail]> wrote:
>
>
>
> >-----Original Message-----
> >From: Jason Wang <[e-mail]>
> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
> >spec
> >
> >On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong
> ><[e-mail]> wrote:
> >>
> >>
> >>
> >> >-----Original Message-----
> >> >From: Jason Wang <[e-mail]>
> >> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
> >> >spec
> >> >
> >> >On Mon, May 20, 2024 at 12:15 PM Liu, Yi L <[e-mail]> wrote:
> >> >>
> >> >> > From: Duan, Zhenzhong <[e-mail]>
> >> >> > Sent: Monday, May 20, 2024 11:41 AM
> >> >> >
> >> >> >
> >> >> >
> >> >> > >-----Original Message-----
> >> >> > >From: Jason Wang <[e-mail]>
> >> >> > >Sent: Monday, May 20, 2024 8:44 AM
> >> >> > >To: Duan, Zhenzhong <[e-mail]>
> >> >> > >Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng,
> >Chao
> >> >P
> >> >> > ><[e-mail]>; Yu Zhang <[e-mail]>;
> >> >Michael
> >> >> > >S. Tsirkin <[e-mail]>; Paolo Bonzini
> ><[e-mail]>;
> >> >> > >Richard Henderson <[e-mail]>; Eduardo
> >Habkost
> >> >> > ><[e-mail]>; Marcel Apfelbaum
> >> ><[e-mail]>
> >> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons
> >defined
> >> >by
> >> >> > >spec
> >> >> > >
> >> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
> >> >> > ><[e-mail]> wrote:
> >> >> > >>
> >> >> > >> From: Yu Zhang <[e-mail]>
> >> >> > >>
> >> >> > >> 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 <[e-mail]>
> >> >> > >> Signed-off-by: Zhenzhong Duan <[e-mail]>
> >> >> > >> ---
> >> >> > >
> >> >> > >I wonder if this could be noticed by the guest or not. If yes should
> >> >> > >we consider starting to add thing like version to vtd emulation code?
> >> >> >
> >> >> > Kernel only dumps the reason like below:
> >> >> >
> >> >> > DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault addr
> >> >0x1234600000
> >> >> > [fault reason 0x71] SM: Present bit in first-level paging entry is clear
> >> >>
> >> >> Yes, guest kernel would notice it as the fault would be injected to vm.
> >> >>
> >> >> > Maybe bump 1.0 -> 1.1?
> >> >> > My understanding version number is only informational and is far
> >from
> >> >> > accurate to mark if a feature supported. Driver should check cap/ecap
> >> >> > bits instead.
> >> >>
> >> >> Should the version ID here be aligned with VT-d spec?
> >> >
> >> >Probably, this might be something that could be noticed by the
> >> >management to migration compatibility.
> >>
> >> Could you elaborate what we need to do for migration compatibility?
> >> I see version is already exported so libvirt can query it, see:
> >>
> >>     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
> >
> >It is the Qemu command line parameters not the version of the vmstate.
> >
> >For example -device intel-iommu,version=3.0
> >
> >Qemu then knows it should behave as 3.0.
>
> So you want to bump vtd_vmstate.version?

Well, as I said, it's not a direct bumping.

>
> In fact, this series change intel_iommu property from x-scalable-mode=["on"|"off"]"
> to x-scalable-mode=["legacy"|"modern"|"off"]".
>
> My understanding management app should use same qemu cmdline
> in source and destination, so compatibility is already guaranteed even if
> we don't bump vtd_vmstate.version.

Exactly, so the point is to

vtd=3.0, the device works exactly as vtd spec 3.0.
vtd=3.3, the device works exactly as vtd spec 3.3.

When migration to the old qemu, mgmt can specify e.g vtd=3.0 for
backward migration compatibility.

Thanks

>
> Thanks
> Zhenzhong

Duan, Zhenzhong May 27, 2024, 6:04 a.m. UTC | #14

>-----Original Message-----
>From: Jason Wang <[e-mail]>
>Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
>spec
>
>On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong
><[e-mail]> wrote:
>>
>>
>>
>> >-----Original Message-----
>> >From: Jason Wang <[e-mail]>
>> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
>> >spec
>> >
>> >On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong
>> ><[e-mail]> wrote:
>> >>
>> >>
>> >>
>> >> >-----Original Message-----
>> >> >From: Jason Wang <[e-mail]>
>> >> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined
>by
>> >> >spec
>> >> >
>> >> >On Mon, May 20, 2024 at 12:15 PM Liu, Yi L <[e-mail]>
>wrote:
>> >> >>
>> >> >> > From: Duan, Zhenzhong <[e-mail]>
>> >> >> > Sent: Monday, May 20, 2024 11:41 AM
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> > >-----Original Message-----
>> >> >> > >From: Jason Wang <[e-mail]>
>> >> >> > >Sent: Monday, May 20, 2024 8:44 AM
>> >> >> > >To: Duan, Zhenzhong <[e-mail]>
>> >> >> > >Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng,
>> >Chao
>> >> >P
>> >> >> > ><[e-mail]>; Yu Zhang
><[e-mail]>;
>> >> >Michael
>> >> >> > >S. Tsirkin <[e-mail]>; Paolo Bonzini
>> ><[e-mail]>;
>> >> >> > >Richard Henderson <[e-mail]>; Eduardo
>> >Habkost
>> >> >> > ><[e-mail]>; Marcel Apfelbaum
>> >> ><[e-mail]>
>> >> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons
>> >defined
>> >> >by
>> >> >> > >spec
>> >> >> > >
>> >> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
>> >> >> > ><[e-mail]> wrote:
>> >> >> > >>
>> >> >> > >> From: Yu Zhang <[e-mail]>
>> >> >> > >>
>> >> >> > >> 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 <[e-mail]>
>> >> >> > >> Signed-off-by: Zhenzhong Duan <[e-mail]>
>> >> >> > >> ---
>> >> >> > >
>> >> >> > >I wonder if this could be noticed by the guest or not. If yes should
>> >> >> > >we consider starting to add thing like version to vtd emulation
>code?
>> >> >> >
>> >> >> > Kernel only dumps the reason like below:
>> >> >> >
>> >> >> > DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault addr
>> >> >0x1234600000
>> >> >> > [fault reason 0x71] SM: Present bit in first-level paging entry is
>clear
>> >> >>
>> >> >> Yes, guest kernel would notice it as the fault would be injected to vm.
>> >> >>
>> >> >> > Maybe bump 1.0 -> 1.1?
>> >> >> > My understanding version number is only informational and is far
>> >from
>> >> >> > accurate to mark if a feature supported. Driver should check
>cap/ecap
>> >> >> > bits instead.
>> >> >>
>> >> >> Should the version ID here be aligned with VT-d spec?
>> >> >
>> >> >Probably, this might be something that could be noticed by the
>> >> >management to migration compatibility.
>> >>
>> >> Could you elaborate what we need to do for migration compatibility?
>> >> I see version is already exported so libvirt can query it, see:
>> >>
>> >>     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
>> >
>> >It is the Qemu command line parameters not the version of the vmstate.
>> >
>> >For example -device intel-iommu,version=3.0
>> >
>> >Qemu then knows it should behave as 3.0.
>>
>> So you want to bump vtd_vmstate.version?
>
>Well, as I said, it's not a direct bumping.
>
>>
>> In fact, this series change intel_iommu property from x-scalable-
>mode=["on"|"off"]"
>> to x-scalable-mode=["legacy"|"modern"|"off"]".
>>
>> My understanding management app should use same qemu cmdline
>> in source and destination, so compatibility is already guaranteed even if
>> we don't bump vtd_vmstate.version.
>
>Exactly, so the point is to
>
>vtd=3.0, the device works exactly as vtd spec 3.0.
>vtd=3.3, the device works exactly as vtd spec 3.3.

Get your point. But I have some concerns about this:
1.Exact version matching will bring vast of version check in the code,
   especially when we support more versions.
2. There are some missed features before we can update version number to 3.0,
    i.e., nested translation, Accessed/Dirty (A/D) bits, 5 level page table, etc.
3. Some features are removed in future versions, but we still need to
   implement them for intermediate version,
   i.e., ExecuteRequested (ER), Advanced Fault Logging, etc.

>
>When migration to the old qemu, mgmt can specify e.g vtd=3.0 for
>backward migration compatibility.

Yes, that makes sense for such migration.
But I'm not sure if there is a real scenario migrating to old qemu,
why not just update qemu on destination?

Thanks
Zhenzhong

Yi Liu May 27, 2024, 6:32 a.m. UTC | #15

On 2024/5/27 14:04, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Jason Wang <[e-mail]>
>> Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
>> spec
>>
>> On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong
>> <[e-mail]> wrote:
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jason Wang <[e-mail]>
>>>> Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
>>>> spec
>>>>
>>>> On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong
>>>> <[e-mail]> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Jason Wang <[e-mail]>
>>>>>> Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined
>> by
>>>>>> spec
>>>>>>
>>>>>> On Mon, May 20, 2024 at 12:15 PM Liu, Yi L <[e-mail]>
>> wrote:
>>>>>>>
>>>>>>>> From: Duan, Zhenzhong <[e-mail]>
>>>>>>>> Sent: Monday, May 20, 2024 11:41 AM
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Jason Wang <[e-mail]>
>>>>>>>>> Sent: Monday, May 20, 2024 8:44 AM
>>>>>>>>> To: Duan, Zhenzhong <[e-mail]>
>>>>>>>>> Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng,
>>>> Chao
>>>>>> P
>>>>>>>>> <[e-mail]>; Yu Zhang
>> <[e-mail]>;
>>>>>> Michael
>>>>>>>>> S. Tsirkin <[e-mail]>; Paolo Bonzini
>>>> <[e-mail]>;
>>>>>>>>> Richard Henderson <[e-mail]>; Eduardo
>>>> Habkost
>>>>>>>>> <[e-mail]>; Marcel Apfelbaum
>>>>>> <[e-mail]>
>>>>>>>>> Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons
>>>> defined
>>>>>> by
>>>>>>>>> spec
>>>>>>>>>
>>>>>>>>> On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
>>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>>
>>>>>>>>>> From: Yu Zhang <[e-mail]>
>>>>>>>>>>
>>>>>>>>>> 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 <[e-mail]>
>>>>>>>>>> Signed-off-by: Zhenzhong Duan <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> I wonder if this could be noticed by the guest or not. If yes should
>>>>>>>>> we consider starting to add thing like version to vtd emulation
>> code?
>>>>>>>>
>>>>>>>> Kernel only dumps the reason like below:
>>>>>>>>
>>>>>>>> DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault addr
>>>>>> 0x1234600000
>>>>>>>> [fault reason 0x71] SM: Present bit in first-level paging entry is
>> clear
>>>>>>>
>>>>>>> Yes, guest kernel would notice it as the fault would be injected to vm.
>>>>>>>
>>>>>>>> Maybe bump 1.0 -> 1.1?
>>>>>>>> My understanding version number is only informational and is far
>>>> from
>>>>>>>> accurate to mark if a feature supported. Driver should check
>> cap/ecap
>>>>>>>> bits instead.
>>>>>>>
>>>>>>> Should the version ID here be aligned with VT-d spec?
>>>>>>

Folks, looks like it's not necessary to be aligned with spec version.
e.g. I can see something like below. This is an old machine which is
not possible to be built according to vt-d spec 4.0. Let me check more
machines as well to confirm this.Perhaps virtual VT-d implementation
can have its own version policy? @Jason, how about your idea?

cat /sys/class/iommu/dmar0/intel-iommu/version
4:0

>>>>>> Probably, this might be something that could be noticed by the
>>>>>> management to migration compatibility.
>>>>>
>>>>> Could you elaborate what we need to do for migration compatibility?
>>>>> I see version is already exported so libvirt can query it, see:
>>>>>
>>>>>      DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
>>>>
>>>> It is the Qemu command line parameters not the version of the vmstate.
>>>>
>>>> For example -device intel-iommu,version=3.0
>>>>
>>>> Qemu then knows it should behave as 3.0.
>>>
>>> So you want to bump vtd_vmstate.version?
>>
>> Well, as I said, it's not a direct bumping.
>>
>>>
>>> In fact, this series change intel_iommu property from x-scalable-
>> mode=["on"|"off"]"
>>> to x-scalable-mode=["legacy"|"modern"|"off"]".
>>>
>>> My understanding management app should use same qemu cmdline
>>> in source and destination, so compatibility is already guaranteed even if
>>> we don't bump vtd_vmstate.version.
>>
>> Exactly, so the point is to
>>
>> vtd=3.0, the device works exactly as vtd spec 3.0.
>> vtd=3.3, the device works exactly as vtd spec 3.3.
> 
> Get your point. But I have some concerns about this:
> 1.Exact version matching will bring vast of version check in the code,
>     especially when we support more versions.
> 2. There are some missed features before we can update version number to 3.0,
>      i.e., nested translation, Accessed/Dirty (A/D) bits, 5 level page table, etc.
> 3. Some features are removed in future versions, but we still need to
>     implement them for intermediate version,
>     i.e., ExecuteRequested (ER), Advanced Fault Logging, etc.

Even the hw follows vtd spec 3.0, it is not required to implement all of
them. So it should be fine to implement part of the capabilities. :)

>>
>> When migration to the old qemu, mgmt can specify e.g vtd=3.0 for
>> backward migration compatibility.
> 
> Yes, that makes sense for such migration.
> But I'm not sure if there is a real scenario migrating to old qemu,
> why not just update qemu on destination?
> 
> Thanks
> Zhenzhong
>

Michael S. Tsirkin May 27, 2024, 6:42 a.m. UTC | #16

On Mon, May 27, 2024 at 06:04:42AM +0000, Duan, Zhenzhong wrote:
> But I'm not sure if there is a real scenario migrating to old qemu,
> why not just update qemu on destination?

Because you can not just update a huge cluster atomically.

Duan, Zhenzhong May 27, 2024, 6:44 a.m. UTC | #17

Hi Jason,

>-----Original Message-----
>From: Duan, Zhenzhong
>Subject: RE: [PATCH] intel_iommu: Use the latest fault reasons defined by
>spec
>
>
>
>>-----Original Message-----
>>From: Jason Wang <[e-mail]>
>>Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
>>spec
>>
>>On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong
>><[e-mail]> wrote:
>>>
>>>
>>>
>>> >-----Original Message-----
>>> >From: Jason Wang <[e-mail]>
>>> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined
>by
>>> >spec
>>> >
>>> >On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong
>>> ><[e-mail]> wrote:
>>> >>
>>> >>
>>> >>
>>> >> >-----Original Message-----
>>> >> >From: Jason Wang <[e-mail]>
>>> >> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons
>defined
>>by
>>> >> >spec
>>> >> >
>>> >> >On Mon, May 20, 2024 at 12:15 PM Liu, Yi L <[e-mail]>
>>wrote:
>>> >> >>
>>> >> >> > From: Duan, Zhenzhong <[e-mail]>
>>> >> >> > Sent: Monday, May 20, 2024 11:41 AM
>>> >> >> >
>>> >> >> >
>>> >> >> >
>>> >> >> > >-----Original Message-----
>>> >> >> > >From: Jason Wang <[e-mail]>
>>> >> >> > >Sent: Monday, May 20, 2024 8:44 AM
>>> >> >> > >To: Duan, Zhenzhong <[e-mail]>
>>> >> >> > >Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng,
>>> >Chao
>>> >> >P
>>> >> >> > ><[e-mail]>; Yu Zhang
>><[e-mail]>;
>>> >> >Michael
>>> >> >> > >S. Tsirkin <[e-mail]>; Paolo Bonzini
>>> ><[e-mail]>;
>>> >> >> > >Richard Henderson <[e-mail]>; Eduardo
>>> >Habkost
>>> >> >> > ><[e-mail]>; Marcel Apfelbaum
>>> >> ><[e-mail]>
>>> >> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons
>>> >defined
>>> >> >by
>>> >> >> > >spec
>>> >> >> > >
>>> >> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
>>> >> >> > ><[e-mail]> wrote:
>>> >> >> > >>
>>> >> >> > >> From: Yu Zhang <[e-mail]>
>>> >> >> > >>
>>> >> >> > >> 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 <[e-mail]>
>>> >> >> > >> Signed-off-by: Zhenzhong Duan <[e-mail]>
>>> >> >> > >> ---
>>> >> >> > >
>>> >> >> > >I wonder if this could be noticed by the guest or not. If yes
>should
>>> >> >> > >we consider starting to add thing like version to vtd emulation
>>code?
>>> >> >> >
>>> >> >> > Kernel only dumps the reason like below:
>>> >> >> >
>>> >> >> > DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault
>addr
>>> >> >0x1234600000
>>> >> >> > [fault reason 0x71] SM: Present bit in first-level paging entry is
>>clear
>>> >> >>
>>> >> >> Yes, guest kernel would notice it as the fault would be injected to
>vm.
>>> >> >>
>>> >> >> > Maybe bump 1.0 -> 1.1?
>>> >> >> > My understanding version number is only informational and is
>far
>>> >from
>>> >> >> > accurate to mark if a feature supported. Driver should check
>>cap/ecap
>>> >> >> > bits instead.
>>> >> >>
>>> >> >> Should the version ID here be aligned with VT-d spec?
>>> >> >
>>> >> >Probably, this might be something that could be noticed by the
>>> >> >management to migration compatibility.
>>> >>
>>> >> Could you elaborate what we need to do for migration compatibility?
>>> >> I see version is already exported so libvirt can query it, see:
>>> >>
>>> >>     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
>>> >
>>> >It is the Qemu command line parameters not the version of the vmstate.
>>> >
>>> >For example -device intel-iommu,version=3.0
>>> >
>>> >Qemu then knows it should behave as 3.0.
>>>
>>> So you want to bump vtd_vmstate.version?
>>
>>Well, as I said, it's not a direct bumping.
>>
>>>
>>> In fact, this series change intel_iommu property from x-scalable-
>>mode=["on"|"off"]"
>>> to x-scalable-mode=["legacy"|"modern"|"off"]".
>>>
>>> My understanding management app should use same qemu cmdline
>>> in source and destination, so compatibility is already guaranteed even if
>>> we don't bump vtd_vmstate.version.
>>
>>Exactly, so the point is to
>>
>>vtd=3.0, the device works exactly as vtd spec 3.0.
>>vtd=3.3, the device works exactly as vtd spec 3.3.

Yi just found version ID stored in VT-d VER_REG is not aligned with the VT-d spec version.
For example, we see a local hw with vtd version 6.0 which is beyond VT-d spec version.
We are asking VTD arch, will get back soon.

Or will you plan qemu vVT-d having its own version policy?

Thanks
Zhenzhong

>
>Get your point. But I have some concerns about this:
>1.Exact version matching will bring vast of version check in the code,
>   especially when we support more versions.
>2. There are some missed features before we can update version number to
>3.0,
>    i.e., nested translation, Accessed/Dirty (A/D) bits, 5 level page table, etc.
>3. Some features are removed in future versions, but we still need to
>   implement them for intermediate version,
>   i.e., ExecuteRequested (ER), Advanced Fault Logging, etc.
>
>>
>>When migration to the old qemu, mgmt can specify e.g vtd=3.0 for
>>backward migration compatibility.
>
>Yes, that makes sense for such migration.
>But I'm not sure if there is a real scenario migrating to old qemu,
>why not just update qemu on destination?
>
>Thanks
>Zhenzhong

Michael S. Tsirkin May 27, 2024, 6:49 a.m. UTC | #18

On Mon, May 27, 2024 at 02:32:46PM +0800, Yi Liu wrote:
> Folks, looks like it's not necessary to be aligned with spec version.
> e.g. I can see something like below. This is an old machine which is
> not possible to be built according to vt-d spec 4.0. Let me check more
> machines as well to confirm this.

Aligning to a spec version is preferable. We don't *have* to force
users to do it if we don't want to, but you never know what
assumptions will guests make.



>Perhaps virtual VT-d implementation
> can have its own version policy? @Jason, how about your idea?
> 
> cat /sys/class/iommu/dmar0/intel-iommu/version
> 4:0

This is PV, really best avoided if we can.

> > > > > > > Probably, this might be something that could be noticed by the
> > > > > > > management to migration compatibility.
> > > > > > 
> > > > > > Could you elaborate what we need to do for migration compatibility?
> > > > > > I see version is already exported so libvirt can query it, see:
> > > > > > 
> > > > > >      DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
> > > > > 
> > > > > It is the Qemu command line parameters not the version of the vmstate.
> > > > > 
> > > > > For example -device intel-iommu,version=3.0
> > > > > 
> > > > > Qemu then knows it should behave as 3.0.
> > > > 
> > > > So you want to bump vtd_vmstate.version?
> > > 
> > > Well, as I said, it's not a direct bumping.
> > > 
> > > > 
> > > > In fact, this series change intel_iommu property from x-scalable-
> > > mode=["on"|"off"]"
> > > > to x-scalable-mode=["legacy"|"modern"|"off"]".
> > > > 
> > > > My understanding management app should use same qemu cmdline
> > > > in source and destination, so compatibility is already guaranteed even if
> > > > we don't bump vtd_vmstate.version.
> > > 
> > > Exactly, so the point is to
> > > 
> > > vtd=3.0, the device works exactly as vtd spec 3.0.
> > > vtd=3.3, the device works exactly as vtd spec 3.3.
> > 
> > Get your point. But I have some concerns about this:
> > 1.Exact version matching will bring vast of version check in the code,
> >     especially when we support more versions.
> > 2. There are some missed features before we can update version number to 3.0,
> >      i.e., nested translation, Accessed/Dirty (A/D) bits, 5 level page table, etc.
> > 3. Some features are removed in future versions, but we still need to
> >     implement them for intermediate version,
> >     i.e., ExecuteRequested (ER), Advanced Fault Logging, etc.
> 
> Even the hw follows vtd spec 3.0, it is not required to implement all of
> them. So it should be fine to implement part of the capabilities. :)

Yes, that's better.
Specifying version # explicitly is really annoying for both
qemu and management.
I think normally we should just start with capabilities and make
the best decision we can wrt guest support etc.

Being concervative is usually a good idea, if a new version gives no
useful functionality adding that is just churn.
So i.e.  we have a set of capabilities we want and select the earliest version
that supports them.

We can let user override that and I am not sure we need to bother
actually checking it's consistent with capabilities if we do
not want to.


> > > 
> > > When migration to the old qemu, mgmt can specify e.g vtd=3.0 for
> > > backward migration compatibility.
> > 
> > Yes, that makes sense for such migration.
> > But I'm not sure if there is a real scenario migrating to old qemu,
> > why not just update qemu on destination?
> > 
> > Thanks
> > Zhenzhong
> > 
> 
> -- 
> Regards,
> Yi Liu

Michael S. Tsirkin May 27, 2024, 6:50 a.m. UTC | #19

On Mon, May 27, 2024 at 06:44:58AM +0000, Duan, Zhenzhong wrote:
> Hi Jason,
> 
> >-----Original Message-----
> >From: Duan, Zhenzhong
> >Subject: RE: [PATCH] intel_iommu: Use the latest fault reasons defined by
> >spec
> >
> >
> >
> >>-----Original Message-----
> >>From: Jason Wang <[e-mail]>
> >>Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
> >>spec
> >>
> >>On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong
> >><[e-mail]> wrote:
> >>>
> >>>
> >>>
> >>> >-----Original Message-----
> >>> >From: Jason Wang <[e-mail]>
> >>> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined
> >by
> >>> >spec
> >>> >
> >>> >On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong
> >>> ><[e-mail]> wrote:
> >>> >>
> >>> >>
> >>> >>
> >>> >> >-----Original Message-----
> >>> >> >From: Jason Wang <[e-mail]>
> >>> >> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons
> >defined
> >>by
> >>> >> >spec
> >>> >> >
> >>> >> >On Mon, May 20, 2024 at 12:15 PM Liu, Yi L <[e-mail]>
> >>wrote:
> >>> >> >>
> >>> >> >> > From: Duan, Zhenzhong <[e-mail]>
> >>> >> >> > Sent: Monday, May 20, 2024 11:41 AM
> >>> >> >> >
> >>> >> >> >
> >>> >> >> >
> >>> >> >> > >-----Original Message-----
> >>> >> >> > >From: Jason Wang <[e-mail]>
> >>> >> >> > >Sent: Monday, May 20, 2024 8:44 AM
> >>> >> >> > >To: Duan, Zhenzhong <[e-mail]>
> >>> >> >> > >Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng,
> >>> >Chao
> >>> >> >P
> >>> >> >> > ><[e-mail]>; Yu Zhang
> >><[e-mail]>;
> >>> >> >Michael
> >>> >> >> > >S. Tsirkin <[e-mail]>; Paolo Bonzini
> >>> ><[e-mail]>;
> >>> >> >> > >Richard Henderson <[e-mail]>; Eduardo
> >>> >Habkost
> >>> >> >> > ><[e-mail]>; Marcel Apfelbaum
> >>> >> ><[e-mail]>
> >>> >> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons
> >>> >defined
> >>> >> >by
> >>> >> >> > >spec
> >>> >> >> > >
> >>> >> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
> >>> >> >> > ><[e-mail]> wrote:
> >>> >> >> > >>
> >>> >> >> > >> From: Yu Zhang <[e-mail]>
> >>> >> >> > >>
> >>> >> >> > >> 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 <[e-mail]>
> >>> >> >> > >> Signed-off-by: Zhenzhong Duan <[e-mail]>
> >>> >> >> > >> ---
> >>> >> >> > >
> >>> >> >> > >I wonder if this could be noticed by the guest or not. If yes
> >should
> >>> >> >> > >we consider starting to add thing like version to vtd emulation
> >>code?
> >>> >> >> >
> >>> >> >> > Kernel only dumps the reason like below:
> >>> >> >> >
> >>> >> >> > DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault
> >addr
> >>> >> >0x1234600000
> >>> >> >> > [fault reason 0x71] SM: Present bit in first-level paging entry is
> >>clear
> >>> >> >>
> >>> >> >> Yes, guest kernel would notice it as the fault would be injected to
> >vm.
> >>> >> >>
> >>> >> >> > Maybe bump 1.0 -> 1.1?
> >>> >> >> > My understanding version number is only informational and is
> >far
> >>> >from
> >>> >> >> > accurate to mark if a feature supported. Driver should check
> >>cap/ecap
> >>> >> >> > bits instead.
> >>> >> >>
> >>> >> >> Should the version ID here be aligned with VT-d spec?
> >>> >> >
> >>> >> >Probably, this might be something that could be noticed by the
> >>> >> >management to migration compatibility.
> >>> >>
> >>> >> Could you elaborate what we need to do for migration compatibility?
> >>> >> I see version is already exported so libvirt can query it, see:
> >>> >>
> >>> >>     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
> >>> >
> >>> >It is the Qemu command line parameters not the version of the vmstate.
> >>> >
> >>> >For example -device intel-iommu,version=3.0
> >>> >
> >>> >Qemu then knows it should behave as 3.0.
> >>>
> >>> So you want to bump vtd_vmstate.version?
> >>
> >>Well, as I said, it's not a direct bumping.
> >>
> >>>
> >>> In fact, this series change intel_iommu property from x-scalable-
> >>mode=["on"|"off"]"
> >>> to x-scalable-mode=["legacy"|"modern"|"off"]".
> >>>
> >>> My understanding management app should use same qemu cmdline
> >>> in source and destination, so compatibility is already guaranteed even if
> >>> we don't bump vtd_vmstate.version.
> >>
> >>Exactly, so the point is to
> >>
> >>vtd=3.0, the device works exactly as vtd spec 3.0.
> >>vtd=3.3, the device works exactly as vtd spec 3.3.
> 
> Yi just found version ID stored in VT-d VER_REG is not aligned with the VT-d spec version.
> For example, we see a local hw with vtd version 6.0 which is beyond VT-d spec version.
> We are asking VTD arch, will get back soon.
> 
> Or will you plan qemu vVT-d having its own version policy?
> 
> Thanks
> Zhenzhong

Not unless there's a good reason to do this.

> >
> >Get your point. But I have some concerns about this:
> >1.Exact version matching will bring vast of version check in the code,
> >   especially when we support more versions.
> >2. There are some missed features before we can update version number to
> >3.0,
> >    i.e., nested translation, Accessed/Dirty (A/D) bits, 5 level page table, etc.
> >3. Some features are removed in future versions, but we still need to
> >   implement them for intermediate version,
> >   i.e., ExecuteRequested (ER), Advanced Fault Logging, etc.
> >
> >>
> >>When migration to the old qemu, mgmt can specify e.g vtd=3.0 for
> >>backward migration compatibility.
> >
> >Yes, that makes sense for such migration.
> >But I'm not sure if there is a real scenario migrating to old qemu,
> >why not just update qemu on destination?
> >
> >Thanks
> >Zhenzhong
>

Jason Wang May 28, 2024, 3:03 a.m. UTC | #20

On Mon, May 27, 2024 at 2:50 PM Michael S. Tsirkin <[e-mail]> wrote:
>
> On Mon, May 27, 2024 at 06:44:58AM +0000, Duan, Zhenzhong wrote:
> > Hi Jason,
> >
> > >-----Original Message-----
> > >From: Duan, Zhenzhong
> > >Subject: RE: [PATCH] intel_iommu: Use the latest fault reasons defined by
> > >spec
> > >
> > >
> > >
> > >>-----Original Message-----
> > >>From: Jason Wang <[e-mail]>
> > >>Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
> > >>spec
> > >>
> > >>On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong
> > >><[e-mail]> wrote:
> > >>>
> > >>>
> > >>>
> > >>> >-----Original Message-----
> > >>> >From: Jason Wang <[e-mail]>
> > >>> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined
> > >by
> > >>> >spec
> > >>> >
> > >>> >On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong
> > >>> ><[e-mail]> wrote:
> > >>> >>
> > >>> >>
> > >>> >>
> > >>> >> >-----Original Message-----
> > >>> >> >From: Jason Wang <[e-mail]>
> > >>> >> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons
> > >defined
> > >>by
> > >>> >> >spec
> > >>> >> >
> > >>> >> >On Mon, May 20, 2024 at 12:15 PM Liu, Yi L <[e-mail]>
> > >>wrote:
> > >>> >> >>
> > >>> >> >> > From: Duan, Zhenzhong <[e-mail]>
> > >>> >> >> > Sent: Monday, May 20, 2024 11:41 AM
> > >>> >> >> >
> > >>> >> >> >
> > >>> >> >> >
> > >>> >> >> > >-----Original Message-----
> > >>> >> >> > >From: Jason Wang <[e-mail]>
> > >>> >> >> > >Sent: Monday, May 20, 2024 8:44 AM
> > >>> >> >> > >To: Duan, Zhenzhong <[e-mail]>
> > >>> >> >> > >Cc: [e-mail]; Liu, Yi L <[e-mail]>; Peng,
> > >>> >Chao
> > >>> >> >P
> > >>> >> >> > ><[e-mail]>; Yu Zhang
> > >><[e-mail]>;
> > >>> >> >Michael
> > >>> >> >> > >S. Tsirkin <[e-mail]>; Paolo Bonzini
> > >>> ><[e-mail]>;
> > >>> >> >> > >Richard Henderson <[e-mail]>; Eduardo
> > >>> >Habkost
> > >>> >> >> > ><[e-mail]>; Marcel Apfelbaum
> > >>> >> ><[e-mail]>
> > >>> >> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons
> > >>> >defined
> > >>> >> >by
> > >>> >> >> > >spec
> > >>> >> >> > >
> > >>> >> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan
> > >>> >> >> > ><[e-mail]> wrote:
> > >>> >> >> > >>
> > >>> >> >> > >> From: Yu Zhang <[e-mail]>
> > >>> >> >> > >>
> > >>> >> >> > >> 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 <[e-mail]>
> > >>> >> >> > >> Signed-off-by: Zhenzhong Duan <[e-mail]>
> > >>> >> >> > >> ---
> > >>> >> >> > >
> > >>> >> >> > >I wonder if this could be noticed by the guest or not. If yes
> > >should
> > >>> >> >> > >we consider starting to add thing like version to vtd emulation
> > >>code?
> > >>> >> >> >
> > >>> >> >> > Kernel only dumps the reason like below:
> > >>> >> >> >
> > >>> >> >> > DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault
> > >addr
> > >>> >> >0x1234600000
> > >>> >> >> > [fault reason 0x71] SM: Present bit in first-level paging entry is
> > >>clear
> > >>> >> >>
> > >>> >> >> Yes, guest kernel would notice it as the fault would be injected to
> > >vm.
> > >>> >> >>
> > >>> >> >> > Maybe bump 1.0 -> 1.1?
> > >>> >> >> > My understanding version number is only informational and is
> > >far
> > >>> >from
> > >>> >> >> > accurate to mark if a feature supported. Driver should check
> > >>cap/ecap
> > >>> >> >> > bits instead.
> > >>> >> >>
> > >>> >> >> Should the version ID here be aligned with VT-d spec?
> > >>> >> >
> > >>> >> >Probably, this might be something that could be noticed by the
> > >>> >> >management to migration compatibility.
> > >>> >>
> > >>> >> Could you elaborate what we need to do for migration compatibility?
> > >>> >> I see version is already exported so libvirt can query it, see:
> > >>> >>
> > >>> >>     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
> > >>> >
> > >>> >It is the Qemu command line parameters not the version of the vmstate.
> > >>> >
> > >>> >For example -device intel-iommu,version=3.0
> > >>> >
> > >>> >Qemu then knows it should behave as 3.0.
> > >>>
> > >>> So you want to bump vtd_vmstate.version?
> > >>
> > >>Well, as I said, it's not a direct bumping.
> > >>
> > >>>
> > >>> In fact, this series change intel_iommu property from x-scalable-
> > >>mode=["on"|"off"]"
> > >>> to x-scalable-mode=["legacy"|"modern"|"off"]".
> > >>>
> > >>> My understanding management app should use same qemu cmdline
> > >>> in source and destination, so compatibility is already guaranteed even if
> > >>> we don't bump vtd_vmstate.version.
> > >>
> > >>Exactly, so the point is to
> > >>
> > >>vtd=3.0, the device works exactly as vtd spec 3.0.
> > >>vtd=3.3, the device works exactly as vtd spec 3.3.
> >
> > Yi just found version ID stored in VT-d VER_REG is not aligned with the VT-d spec version.
> > For example, we see a local hw with vtd version 6.0 which is beyond VT-d spec version.
> > We are asking VTD arch, will get back soon.
> >
> > Or will you plan qemu vVT-d having its own version policy?
> >
> > Thanks
> > Zhenzhong
>
> Not unless there's a good reason to do this.

+1

Thanks

Patch

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 */
+    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,
 };