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

Yi Liu July 2, 2024, 8:42 a.m. UTC | #21

Hi Michael, Jason,

On 2024/5/28 11:03, Jason Wang wrote:
> 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

Had more studying in the spec. Bumping up to 3.0 might not be trivial. :(

The VT-d spec has some requirements based on the version number. Below is a
list of it. Although they are defined for hardware, but vVT-d may need to
respect it as the same iommu driver runs for both the vVT-d and the hw
VT-d. Per 1), if bumping up the major value to be 6 or higher, the vVT-d
needs to ensure the register-based invalidation interface is not available.
Per 2), if bump up the major version to be 2 or higher, the vVT-d needs to
by default drain write and read requests no matter the software requests it
or not.

1) Chapter 6.5 Invalidation of Translation Caches

For software to invalidate the various caching structures, the architecture 
supports the following two
types of invalidation interfaces:
• Register-based invalidation interface: A legacy invalidation interface 
with limited capabilities.
This interface is supported by hardware implementations of this 
architecture with Major Version 5
or lower (VER_REG). In all other hardware implementations, all requests are 
treated as invalid
requests and will be ignored (for details see the CAIG field in the Context 
Command Register and
the IAIG field in the IOTLB Invalidate Register).

2) Chapter 6.5.2.3 IOTLB Invalidate
• Drain Reads (DR): Software sets this flag to indicate hardware must drain 
read requests that are
already processed by the remapping hardware, but queued within the 
Root-Complex to be
completed. When the value of this flag is 1, hardware must drain the 
relevant reads before the
next Invalidation Wait Descriptor (see Section 6.5.2.8) is completed. 
Section 6.5.4 describes
hardware support for draining. Hardware implementations with Major Version 
2 or higher
(VER_REG) will ignore this flag and always drain relevant reads before the 
next Invalidation Wait
Descriptor is completed.
• Drain Writes (DW): Software sets this flag to indicate hardware must 
drain relevant write
requests that are already processed by the remapping hardware, but queued 
within the RootComplex to be completed. When the value of this flag is 1, 
hardware must drain the relevant
writes before the next Invalidation Wait Descriptor is completed. Section 
6.5.4 describes
hardware support for draining. Hardware implementations with Major Version 
2 or higher
(VER_REG) will ignore this flag and always drain relevant writes before the 
next Invalidation Wait
Descriptor is completed.

3) Chapter 11.4.2 Capability Register
Hardware implementations with Major Version 6 or higher
(VER_REG) reporting the second-stage translation support (SSTS)
field as Clear also report this field as 0.

4) Chapter 11.4.8.1 Protected Memory Enable Register
• Hardware implementations with Major Version 2 or higher
(VER_REG) block all DMA requests accessing protected
memory regions whether or not DMA remapping is
enabled.

Also, as replied in the prior email, the VT-d architecture reports
capability via the cap/ecap registers. The new fault reasons in this
patch is meaningful only when the ecap.SMTS bit is set. So bumping version
does not mean too much about the introduction of new capabilities. @Jason,
given the above statements, can we reconsider if it is necessary to bump
up the version number?

Duan, Zhenzhong July 17, 2024, 3:30 a.m. UTC | #22

Hi Michael, Jason,

Based on Yi's analysis, is keeping current VERSION value acceptable for you?
Look forward to your comments, currently this open blocks us from sending the next version.

Thanks
Zhenzhong

>-----Original Message-----
>From: Liu, Yi L <[e-mail]>
>Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
>spec
>
>Hi Michael, Jason,
>
>On 2024/5/28 11:03, Jason Wang wrote:
>> 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
>
>Had more studying in the spec. Bumping up to 3.0 might not be trivial. :(
>
>The VT-d spec has some requirements based on the version number. Below
>is a
>list of it. Although they are defined for hardware, but vVT-d may need to
>respect it as the same iommu driver runs for both the vVT-d and the hw
>VT-d. Per 1), if bumping up the major value to be 6 or higher, the vVT-d
>needs to ensure the register-based invalidation interface is not available.
>Per 2), if bump up the major version to be 2 or higher, the vVT-d needs to
>by default drain write and read requests no matter the software requests it
>or not.
>
>1) Chapter 6.5 Invalidation of Translation Caches
>
>For software to invalidate the various caching structures, the architecture
>supports the following two
>types of invalidation interfaces:
>• Register-based invalidation interface: A legacy invalidation interface
>with limited capabilities.
>This interface is supported by hardware implementations of this
>architecture with Major Version 5
>or lower (VER_REG). In all other hardware implementations, all requests are
>treated as invalid
>requests and will be ignored (for details see the CAIG field in the Context
>Command Register and
>the IAIG field in the IOTLB Invalidate Register).
>
>2) Chapter 6.5.2.3 IOTLB Invalidate
>• Drain Reads (DR): Software sets this flag to indicate hardware must drain
>read requests that are
>already processed by the remapping hardware, but queued within the
>Root-Complex to be
>completed. When the value of this flag is 1, hardware must drain the
>relevant reads before the
>next Invalidation Wait Descriptor (see Section 6.5.2.8) is completed.
>Section 6.5.4 describes
>hardware support for draining. Hardware implementations with Major
>Version
>2 or higher
>(VER_REG) will ignore this flag and always drain relevant reads before the
>next Invalidation Wait
>Descriptor is completed.
>• Drain Writes (DW): Software sets this flag to indicate hardware must
>drain relevant write
>requests that are already processed by the remapping hardware, but queued
>within the RootComplex to be completed. When the value of this flag is 1,
>hardware must drain the relevant
>writes before the next Invalidation Wait Descriptor is completed. Section
>6.5.4 describes
>hardware support for draining. Hardware implementations with Major
>Version
>2 or higher
>(VER_REG) will ignore this flag and always drain relevant writes before the
>next Invalidation Wait
>Descriptor is completed.
>
>3) Chapter 11.4.2 Capability Register
>Hardware implementations with Major Version 6 or higher
>(VER_REG) reporting the second-stage translation support (SSTS)
>field as Clear also report this field as 0.
>
>4) Chapter 11.4.8.1 Protected Memory Enable Register
>• Hardware implementations with Major Version 2 or higher
>(VER_REG) block all DMA requests accessing protected
>memory regions whether or not DMA remapping is
>enabled.
>
>Also, as replied in the prior email, the VT-d architecture reports
>capability via the cap/ecap registers. The new fault reasons in this
>patch is meaningful only when the ecap.SMTS bit is set. So bumping version
>does not mean too much about the introduction of new capabilities. @Jason,
>given the above statements, can we reconsider if it is necessary to bump
>up the version number?
>
>--
>Regards,
>Yi Liu

Jason Wang July 17, 2024, 3:53 a.m. UTC | #23

On Wed, Jul 17, 2024 at 11:30 AM Duan, Zhenzhong
<[e-mail]> wrote:
>
> Hi Michael, Jason,
>
> Based on Yi's analysis, is keeping current VERSION value acceptable for you?

Fine with me.

> Look forward to your comments, currently this open blocks us from sending the next version.
>
> Thanks
> Zhenzhong

Thanks

>
> >-----Original Message-----
> >From: Liu, Yi L <[e-mail]>
> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
> >spec
> >
> >Hi Michael, Jason,
> >
> >On 2024/5/28 11:03, Jason Wang wrote:
> >> 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
> >
> >Had more studying in the spec. Bumping up to 3.0 might not be trivial. :(
> >
> >The VT-d spec has some requirements based on the version number. Below
> >is a
> >list of it. Although they are defined for hardware, but vVT-d may need to
> >respect it as the same iommu driver runs for both the vVT-d and the hw
> >VT-d. Per 1), if bumping up the major value to be 6 or higher, the vVT-d
> >needs to ensure the register-based invalidation interface is not available.
> >Per 2), if bump up the major version to be 2 or higher, the vVT-d needs to
> >by default drain write and read requests no matter the software requests it
> >or not.
> >
> >1) Chapter 6.5 Invalidation of Translation Caches
> >
> >For software to invalidate the various caching structures, the architecture
> >supports the following two
> >types of invalidation interfaces:
> >• Register-based invalidation interface: A legacy invalidation interface
> >with limited capabilities.
> >This interface is supported by hardware implementations of this
> >architecture with Major Version 5
> >or lower (VER_REG). In all other hardware implementations, all requests are
> >treated as invalid
> >requests and will be ignored (for details see the CAIG field in the Context
> >Command Register and
> >the IAIG field in the IOTLB Invalidate Register).
> >
> >2) Chapter 6.5.2.3 IOTLB Invalidate
> >• Drain Reads (DR): Software sets this flag to indicate hardware must drain
> >read requests that are
> >already processed by the remapping hardware, but queued within the
> >Root-Complex to be
> >completed. When the value of this flag is 1, hardware must drain the
> >relevant reads before the
> >next Invalidation Wait Descriptor (see Section 6.5.2.8) is completed.
> >Section 6.5.4 describes
> >hardware support for draining. Hardware implementations with Major
> >Version
> >2 or higher
> >(VER_REG) will ignore this flag and always drain relevant reads before the
> >next Invalidation Wait
> >Descriptor is completed.
> >• Drain Writes (DW): Software sets this flag to indicate hardware must
> >drain relevant write
> >requests that are already processed by the remapping hardware, but queued
> >within the RootComplex to be completed. When the value of this flag is 1,
> >hardware must drain the relevant
> >writes before the next Invalidation Wait Descriptor is completed. Section
> >6.5.4 describes
> >hardware support for draining. Hardware implementations with Major
> >Version
> >2 or higher
> >(VER_REG) will ignore this flag and always drain relevant writes before the
> >next Invalidation Wait
> >Descriptor is completed.
> >
> >3) Chapter 11.4.2 Capability Register
> >Hardware implementations with Major Version 6 or higher
> >(VER_REG) reporting the second-stage translation support (SSTS)
> >field as Clear also report this field as 0.
> >
> >4) Chapter 11.4.8.1 Protected Memory Enable Register
> >• Hardware implementations with Major Version 2 or higher
> >(VER_REG) block all DMA requests accessing protected
> >memory regions whether or not DMA remapping is
> >enabled.
> >
> >Also, as replied in the prior email, the VT-d architecture reports
> >capability via the cap/ecap registers. The new fault reasons in this
> >patch is meaningful only when the ecap.SMTS bit is set. So bumping version
> >does not mean too much about the introduction of new capabilities. @Jason,
> >given the above statements, can we reconsider if it is necessary to bump
> >up the version number?
> >
> >--
> >Regards,
> >Yi Liu

Michael S. Tsirkin July 17, 2024, 6:04 a.m. UTC | #24

On Wed, Jul 17, 2024 at 03:30:03AM +0000, Duan, Zhenzhong wrote:
> Hi Michael, Jason,
> 
> Based on Yi's analysis, is keeping current VERSION value acceptable for you?

I think it's fine.

> Look forward to your comments, currently this open blocks us from sending the next version.
> 
> Thanks
> Zhenzhong
> 
> >-----Original Message-----
> >From: Liu, Yi L <[e-mail]>
> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by
> >spec
> >
> >Hi Michael, Jason,
> >
> >On 2024/5/28 11:03, Jason Wang wrote:
> >> 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
> >
> >Had more studying in the spec. Bumping up to 3.0 might not be trivial. :(
> >
> >The VT-d spec has some requirements based on the version number. Below
> >is a
> >list of it. Although they are defined for hardware, but vVT-d may need to
> >respect it as the same iommu driver runs for both the vVT-d and the hw
> >VT-d. Per 1), if bumping up the major value to be 6 or higher, the vVT-d
> >needs to ensure the register-based invalidation interface is not available.
> >Per 2), if bump up the major version to be 2 or higher, the vVT-d needs to
> >by default drain write and read requests no matter the software requests it
> >or not.
> >
> >1) Chapter 6.5 Invalidation of Translation Caches
> >
> >For software to invalidate the various caching structures, the architecture
> >supports the following two
> >types of invalidation interfaces:
> >• Register-based invalidation interface: A legacy invalidation interface
> >with limited capabilities.
> >This interface is supported by hardware implementations of this
> >architecture with Major Version 5
> >or lower (VER_REG). In all other hardware implementations, all requests are
> >treated as invalid
> >requests and will be ignored (for details see the CAIG field in the Context
> >Command Register and
> >the IAIG field in the IOTLB Invalidate Register).
> >
> >2) Chapter 6.5.2.3 IOTLB Invalidate
> >• Drain Reads (DR): Software sets this flag to indicate hardware must drain
> >read requests that are
> >already processed by the remapping hardware, but queued within the
> >Root-Complex to be
> >completed. When the value of this flag is 1, hardware must drain the
> >relevant reads before the
> >next Invalidation Wait Descriptor (see Section 6.5.2.8) is completed.
> >Section 6.5.4 describes
> >hardware support for draining. Hardware implementations with Major
> >Version
> >2 or higher
> >(VER_REG) will ignore this flag and always drain relevant reads before the
> >next Invalidation Wait
> >Descriptor is completed.
> >• Drain Writes (DW): Software sets this flag to indicate hardware must
> >drain relevant write
> >requests that are already processed by the remapping hardware, but queued
> >within the RootComplex to be completed. When the value of this flag is 1,
> >hardware must drain the relevant
> >writes before the next Invalidation Wait Descriptor is completed. Section
> >6.5.4 describes
> >hardware support for draining. Hardware implementations with Major
> >Version
> >2 or higher
> >(VER_REG) will ignore this flag and always drain relevant writes before the
> >next Invalidation Wait
> >Descriptor is completed.
> >
> >3) Chapter 11.4.2 Capability Register
> >Hardware implementations with Major Version 6 or higher
> >(VER_REG) reporting the second-stage translation support (SSTS)
> >field as Clear also report this field as 0.
> >
> >4) Chapter 11.4.8.1 Protected Memory Enable Register
> >• Hardware implementations with Major Version 2 or higher
> >(VER_REG) block all DMA requests accessing protected
> >memory regions whether or not DMA remapping is
> >enabled.
> >
> >Also, as replied in the prior email, the VT-d architecture reports
> >capability via the cap/ecap registers. The new fault reasons in this
> >patch is meaningful only when the ecap.SMTS bit is set. So bumping version
> >does not mean too much about the introduction of new capabilities. @Jason,
> >given the above statements, can we reconsider if it is necessary to bump
> >up the version number?
> >
> >--
> >Regards,
> >Yi Liu

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,
 };