Message ID | 20240517102334.81943-1-zhenzhong.duan@intel.com |
---|---|
State | New |
Headers | show |
Series | intel_iommu: Use the latest fault reasons defined by spec | expand |
Hi Zhenzhong On 17/05/2024 12:23, Zhenzhong Duan wrote: > Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe. > > > From: Yu Zhang <yu.c.zhang@linux.intel.com> > > Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. > Update with more detailed fault reasons listed in VT-d spec 7.2.3. > > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/i386/intel_iommu_internal.h | 8 +++++++- > hw/i386/intel_iommu.c | 25 ++++++++++++++++--------- > 2 files changed, 23 insertions(+), 10 deletions(-) > > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > index f8cf99bddf..666e2cf2ce 100644 > --- a/hw/i386/intel_iommu_internal.h > +++ b/hw/i386/intel_iommu_internal.h > @@ -311,7 +311,13 @@ typedef enum VTDFaultReason { > * request while disabled */ > VTD_FR_IR_SID_ERR = 0x26, /* Invalid Source-ID */ > > - VTD_FR_PASID_TABLE_INV = 0x58, /*Invalid PASID table entry */ > + /* PASID directory entry access failure */ > + VTD_FR_PASID_DIR_ACCESS_ERR = 0x50, > + /* The Present(P) field of pasid directory entry is 0 */ > + VTD_FR_PASID_DIR_ENTRY_P = 0x51, > + VTD_FR_PASID_TABLE_ACCESS_ERR = 0x58, /* PASID table entry access failure */ > + VTD_FR_PASID_ENTRY_P = 0x59, /* The Present(P) field of pasidt-entry is 0 */ s/pasidt/pasid > + 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
> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> > Sent: Friday, May 17, 2024 9:13 PM > > Hi Zhenzhong > > On 17/05/2024 12:23, Zhenzhong Duan wrote: > > Caution: External email. Do not open attachments or click links, unless this email > comes from a known sender and you know the content is safe. > > > > > > From: Yu Zhang <yu.c.zhang@linux.intel.com> > > > > Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. > > Update with more detailed fault reasons listed in VT-d spec 7.2.3. > > > > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > > --- > > hw/i386/intel_iommu_internal.h | 8 +++++++- > > hw/i386/intel_iommu.c | 25 ++++++++++++++++--------- > > 2 files changed, 23 insertions(+), 10 deletions(-) > > > > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > > index f8cf99bddf..666e2cf2ce 100644 > > --- a/hw/i386/intel_iommu_internal.h > > +++ b/hw/i386/intel_iommu_internal.h > > @@ -311,7 +311,13 @@ typedef enum VTDFaultReason { > > * request while disabled */ > > VTD_FR_IR_SID_ERR = 0x26, /* Invalid Source-ID */ > > > > - VTD_FR_PASID_TABLE_INV = 0x58, /*Invalid PASID table entry */ > > + /* PASID directory entry access failure */ > > + VTD_FR_PASID_DIR_ACCESS_ERR = 0x50, > > + /* The Present(P) field of pasid directory entry is 0 */ > > + VTD_FR_PASID_DIR_ENTRY_P = 0x51, > > + VTD_FR_PASID_TABLE_ACCESS_ERR = 0x58, /* PASID table entry access failure */ > > + VTD_FR_PASID_ENTRY_P = 0x59, /* The Present(P) field of pasidt-entry is 0 */ > s/pasidt/pasid Per spec, it is pasid table entry. So Zhenzhong may need to use the same word With the line below. E.g. PASID Table entry. Regards, Yi Liu > > + VTD_FR_PASID_TABLE_ENTRY_INV = 0x5b, /*Invalid PASID table entry */ > > > > /* Output address in the interrupt address range for scalable mode */ > > VTD_FR_SM_INTERRUPT_ADDR = 0x87, > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index cc8e59674e..0951ebb71d 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -771,7 +771,7 @@ static int vtd_get_pdire_from_pdir_table(dma_addr_t > pasid_dir_base, > > addr = pasid_dir_base + index * entry_size; > > if (dma_memory_read(&address_space_memory, addr, > > pdire, entry_size, MEMTXATTRS_UNSPECIFIED)) { > > - return -VTD_FR_PASID_TABLE_INV; > > + return -VTD_FR_PASID_DIR_ACCESS_ERR; > > } > > > > pdire->val = le64_to_cpu(pdire->val); > > @@ -789,6 +789,7 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState > *s, > > dma_addr_t addr, > > VTDPASIDEntry *pe) > > { > > + uint8_t pgtt; > > uint32_t index; > > dma_addr_t entry_size; > > X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); > > @@ -798,7 +799,7 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState > *s, > > addr = addr + index * entry_size; > > if (dma_memory_read(&address_space_memory, addr, > > pe, entry_size, MEMTXATTRS_UNSPECIFIED)) { > > - return -VTD_FR_PASID_TABLE_INV; > > + return -VTD_FR_PASID_TABLE_ACCESS_ERR; > > } > > for (size_t i = 0; i < ARRAY_SIZE(pe->val); i++) { > > pe->val[i] = le64_to_cpu(pe->val[i]); > > @@ -806,11 +807,13 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState > *s, > > > > /* Do translation type check */ > > if (!vtd_pe_type_check(x86_iommu, pe)) { > > - return -VTD_FR_PASID_TABLE_INV; > > + return -VTD_FR_PASID_TABLE_ENTRY_INV; > > } > > > > - if (!vtd_is_level_supported(s, VTD_PE_GET_LEVEL(pe))) { > > - return -VTD_FR_PASID_TABLE_INV; > > + pgtt = VTD_PE_GET_TYPE(pe); > > + if (pgtt == VTD_SM_PASID_ENTRY_SLT && > > + !vtd_is_level_supported(s, VTD_PE_GET_LEVEL(pe))) { > > + return -VTD_FR_PASID_TABLE_ENTRY_INV; > > } > > > > return 0; > > @@ -851,7 +854,7 @@ static int vtd_get_pe_from_pasid_table(IntelIOMMUState *s, > > } > > > > if (!vtd_pdire_present(&pdire)) { > > - return -VTD_FR_PASID_TABLE_INV; > > + return -VTD_FR_PASID_DIR_ENTRY_P; > > } > > > > ret = vtd_get_pe_from_pdire(s, pasid, &pdire, pe); > > @@ -860,7 +863,7 @@ static int vtd_get_pe_from_pasid_table(IntelIOMMUState *s, > > } > > > > if (!vtd_pe_present(pe)) { > > - return -VTD_FR_PASID_TABLE_INV; > > + return -VTD_FR_PASID_ENTRY_P; > > } > > > > return 0; > > @@ -913,7 +916,7 @@ static int vtd_ce_get_pasid_fpd(IntelIOMMUState *s, > > } > > > > if (!vtd_pdire_present(&pdire)) { > > - return -VTD_FR_PASID_TABLE_INV; > > + return -VTD_FR_PASID_DIR_ENTRY_P; > > } > > > > /* > > @@ -1770,7 +1773,11 @@ static const bool vtd_qualified_faults[] = { > > [VTD_FR_ROOT_ENTRY_RSVD] = false, > > [VTD_FR_PAGING_ENTRY_RSVD] = true, > > [VTD_FR_CONTEXT_ENTRY_TT] = true, > > - [VTD_FR_PASID_TABLE_INV] = false, > > + [VTD_FR_PASID_DIR_ACCESS_ERR] = false, > > + [VTD_FR_PASID_DIR_ENTRY_P] = true, > > + [VTD_FR_PASID_TABLE_ACCESS_ERR] = false, > > + [VTD_FR_PASID_ENTRY_P] = true, > > + [VTD_FR_PASID_TABLE_ENTRY_INV] = true, > > [VTD_FR_SM_INTERRUPT_ADDR] = true, > > [VTD_FR_MAX] = false, > > }; > > -- > > 2.34.1 > > > > > lgtm
> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> > Sent: Friday, May 17, 2024 9:13 PM > > Hi Zhenzhong > > On 17/05/2024 12:23, Zhenzhong Duan wrote: > > Caution: External email. Do not open attachments or click links, unless this email > comes from a known sender and you know the content is safe. > > > > > > From: Yu Zhang <yu.c.zhang@linux.intel.com> > > > > Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. > > Update with more detailed fault reasons listed in VT-d spec 7.2.3. > > > > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > > --- > > hw/i386/intel_iommu_internal.h | 8 +++++++- > > hw/i386/intel_iommu.c | 25 ++++++++++++++++--------- > > 2 files changed, 23 insertions(+), 10 deletions(-) > > > > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > > index f8cf99bddf..666e2cf2ce 100644 > > --- a/hw/i386/intel_iommu_internal.h > > +++ b/hw/i386/intel_iommu_internal.h > > @@ -311,7 +311,13 @@ typedef enum VTDFaultReason { > > * request while disabled */ > > VTD_FR_IR_SID_ERR = 0x26, /* Invalid Source-ID */ > > > > - VTD_FR_PASID_TABLE_INV = 0x58, /*Invalid PASID table entry */ > > + /* PASID directory entry access failure */ > > + VTD_FR_PASID_DIR_ACCESS_ERR = 0x50, > > + /* The Present(P) field of pasid directory entry is 0 */ > > + VTD_FR_PASID_DIR_ENTRY_P = 0x51, > > + VTD_FR_PASID_TABLE_ACCESS_ERR = 0x58, /* PASID table entry access failure */ > > + VTD_FR_PASID_ENTRY_P = 0x59, /* The Present(P) field of pasidt-entry is 0 */ > s/pasidt/pasid Per spec, it is pasid table entry. So Zhenzhong may need to use the same word With the line below. E.g. PASID Table entry. Regards, Yi Liu 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
On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan <zhenzhong.duan@intel.com> wrote: > > From: Yu Zhang <yu.c.zhang@linux.intel.com> > > Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. > Update with more detailed fault reasons listed in VT-d spec 7.2.3. > > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- 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
>-----Original Message----- >From: Liu, Yi L <yi.l.liu@intel.com> >Subject: RE: [PATCH] intel_iommu: Use the latest fault reasons defined by >spec > >> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >> Sent: Friday, May 17, 2024 9:13 PM >> >> Hi Zhenzhong >> >> On 17/05/2024 12:23, Zhenzhong Duan wrote: >> > Caution: External email. Do not open attachments or click links, unless >this email >> comes from a known sender and you know the content is safe. >> > >> > >> > From: Yu Zhang <yu.c.zhang@linux.intel.com> >> > >> > Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. >> > Update with more detailed fault reasons listed in VT-d spec 7.2.3. >> > >> > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> >> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> > --- >> > hw/i386/intel_iommu_internal.h | 8 +++++++- >> > hw/i386/intel_iommu.c | 25 ++++++++++++++++--------- >> > 2 files changed, 23 insertions(+), 10 deletions(-) >> > >> > diff --git a/hw/i386/intel_iommu_internal.h >b/hw/i386/intel_iommu_internal.h >> > index f8cf99bddf..666e2cf2ce 100644 >> > --- a/hw/i386/intel_iommu_internal.h >> > +++ b/hw/i386/intel_iommu_internal.h >> > @@ -311,7 +311,13 @@ typedef enum VTDFaultReason { >> > * request while disabled */ >> > VTD_FR_IR_SID_ERR = 0x26, /* Invalid Source-ID */ >> > >> > - VTD_FR_PASID_TABLE_INV = 0x58, /*Invalid PASID table entry */ >> > + /* PASID directory entry access failure */ >> > + VTD_FR_PASID_DIR_ACCESS_ERR = 0x50, >> > + /* The Present(P) field of pasid directory entry is 0 */ >> > + VTD_FR_PASID_DIR_ENTRY_P = 0x51, >> > + VTD_FR_PASID_TABLE_ACCESS_ERR = 0x58, /* PASID table entry >access failure */ >> > + VTD_FR_PASID_ENTRY_P = 0x59, /* The Present(P) field of pasidt- >entry is 0 */ >> s/pasidt/pasid > >Per spec, it is pasid table entry. So Zhenzhong may need to use the same >word >With the line below. E.g. PASID Table entry. 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
>-----Original Message----- >From: Jason Wang <jasowang@redhat.com> >Sent: Monday, May 20, 2024 8:44 AM >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao P ><chao.p.peng@intel.com>; Yu Zhang <yu.c.zhang@linux.intel.com>; Michael >S. Tsirkin <mst@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>; >Richard Henderson <richard.henderson@linaro.org>; Eduardo Habkost ><eduardo@habkost.net>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by >spec > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan ><zhenzhong.duan@intel.com> wrote: >> >> From: Yu Zhang <yu.c.zhang@linux.intel.com> >> >> Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. >> Update with more detailed fault reasons listed in VT-d spec 7.2.3. >> >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- > >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
> From: Duan, Zhenzhong <zhenzhong.duan@intel.com> > Sent: Monday, May 20, 2024 11:41 AM > > > > >-----Original Message----- > >From: Jason Wang <jasowang@redhat.com> > >Sent: Monday, May 20, 2024 8:44 AM > >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> > >Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao P > ><chao.p.peng@intel.com>; Yu Zhang <yu.c.zhang@linux.intel.com>; Michael > >S. Tsirkin <mst@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>; > >Richard Henderson <richard.henderson@linaro.org>; Eduardo Habkost > ><eduardo@habkost.net>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by > >spec > > > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan > ><zhenzhong.duan@intel.com> wrote: > >> > >> From: Yu Zhang <yu.c.zhang@linux.intel.com> > >> > >> Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. > >> Update with more detailed fault reasons listed in VT-d spec 7.2.3. > >> > >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > >> --- > > > >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
>-----Original Message----- >From: Liu, Yi L <yi.l.liu@intel.com> >Subject: RE: [PATCH] intel_iommu: Use the latest fault reasons defined by >spec > >> From: Duan, Zhenzhong <zhenzhong.duan@intel.com> >> Sent: Monday, May 20, 2024 11:41 AM >> >> >> >> >-----Original Message----- >> >From: Jason Wang <jasowang@redhat.com> >> >Sent: Monday, May 20, 2024 8:44 AM >> >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >> >Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao P >> ><chao.p.peng@intel.com>; Yu Zhang <yu.c.zhang@linux.intel.com>; >Michael >> >S. Tsirkin <mst@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>; >> >Richard Henderson <richard.henderson@linaro.org>; Eduardo Habkost >> ><eduardo@habkost.net>; Marcel Apfelbaum ><marcel.apfelbaum@gmail.com> >> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by >> >spec >> > >> >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan >> ><zhenzhong.duan@intel.com> wrote: >> >> >> >> From: Yu Zhang <yu.c.zhang@linux.intel.com> >> >> >> >> Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. >> >> Update with more detailed fault reasons listed in VT-d spec 7.2.3. >> >> >> >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> >> --- >> > >> >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
On Mon, May 20, 2024 at 12:15 PM Liu, Yi L <yi.l.liu@intel.com> wrote: > > > From: Duan, Zhenzhong <zhenzhong.duan@intel.com> > > Sent: Monday, May 20, 2024 11:41 AM > > > > > > > > >-----Original Message----- > > >From: Jason Wang <jasowang@redhat.com> > > >Sent: Monday, May 20, 2024 8:44 AM > > >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> > > >Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao P > > ><chao.p.peng@intel.com>; Yu Zhang <yu.c.zhang@linux.intel.com>; Michael > > >S. Tsirkin <mst@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>; > > >Richard Henderson <richard.henderson@linaro.org>; Eduardo Habkost > > ><eduardo@habkost.net>; Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by > > >spec > > > > > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan > > ><zhenzhong.duan@intel.com> wrote: > > >> > > >> From: Yu Zhang <yu.c.zhang@linux.intel.com> > > >> > > >> Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. > > >> Update with more detailed fault reasons listed in VT-d spec 7.2.3. > > >> > > >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > > >> --- > > > > > >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
>-----Original Message----- >From: Jason Wang <jasowang@redhat.com> >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 <yi.l.liu@intel.com> wrote: >> >> > From: Duan, Zhenzhong <zhenzhong.duan@intel.com> >> > Sent: Monday, May 20, 2024 11:41 AM >> > >> > >> > >> > >-----Original Message----- >> > >From: Jason Wang <jasowang@redhat.com> >> > >Sent: Monday, May 20, 2024 8:44 AM >> > >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >> > >Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao >P >> > ><chao.p.peng@intel.com>; Yu Zhang <yu.c.zhang@linux.intel.com>; >Michael >> > >S. Tsirkin <mst@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>; >> > >Richard Henderson <richard.henderson@linaro.org>; Eduardo Habkost >> > ><eduardo@habkost.net>; Marcel Apfelbaum ><marcel.apfelbaum@gmail.com> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined >by >> > >spec >> > > >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan >> > ><zhenzhong.duan@intel.com> wrote: >> > >> >> > >> From: Yu Zhang <yu.c.zhang@linux.intel.com> >> > >> >> > >> Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. >> > >> Update with more detailed fault reasons listed in VT-d spec 7.2.3. >> > >> >> > >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> >> > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> > >> --- >> > > >> > >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
On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong <zhenzhong.duan@intel.com> wrote: > > > > >-----Original Message----- > >From: Jason Wang <jasowang@redhat.com> > >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 <yi.l.liu@intel.com> wrote: > >> > >> > From: Duan, Zhenzhong <zhenzhong.duan@intel.com> > >> > Sent: Monday, May 20, 2024 11:41 AM > >> > > >> > > >> > > >> > >-----Original Message----- > >> > >From: Jason Wang <jasowang@redhat.com> > >> > >Sent: Monday, May 20, 2024 8:44 AM > >> > >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> > >> > >Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao > >P > >> > ><chao.p.peng@intel.com>; Yu Zhang <yu.c.zhang@linux.intel.com>; > >Michael > >> > >S. Tsirkin <mst@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>; > >> > >Richard Henderson <richard.henderson@linaro.org>; Eduardo Habkost > >> > ><eduardo@habkost.net>; Marcel Apfelbaum > ><marcel.apfelbaum@gmail.com> > >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined > >by > >> > >spec > >> > > > >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan > >> > ><zhenzhong.duan@intel.com> wrote: > >> > >> > >> > >> From: Yu Zhang <yu.c.zhang@linux.intel.com> > >> > >> > >> > >> Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. > >> > >> Update with more detailed fault reasons listed in VT-d spec 7.2.3. > >> > >> > >> > >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > >> > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > >> > >> --- > >> > > > >> > >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 >
>-----Original Message----- >From: Jason Wang <jasowang@redhat.com> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by >spec > >On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong ><zhenzhong.duan@intel.com> wrote: >> >> >> >> >-----Original Message----- >> >From: Jason Wang <jasowang@redhat.com> >> >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 <yi.l.liu@intel.com> wrote: >> >> >> >> > From: Duan, Zhenzhong <zhenzhong.duan@intel.com> >> >> > Sent: Monday, May 20, 2024 11:41 AM >> >> > >> >> > >> >> > >> >> > >-----Original Message----- >> >> > >From: Jason Wang <jasowang@redhat.com> >> >> > >Sent: Monday, May 20, 2024 8:44 AM >> >> > >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >> >> > >Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, >Chao >> >P >> >> > ><chao.p.peng@intel.com>; Yu Zhang <yu.c.zhang@linux.intel.com>; >> >Michael >> >> > >S. Tsirkin <mst@redhat.com>; Paolo Bonzini ><pbonzini@redhat.com>; >> >> > >Richard Henderson <richard.henderson@linaro.org>; Eduardo >Habkost >> >> > ><eduardo@habkost.net>; Marcel Apfelbaum >> ><marcel.apfelbaum@gmail.com> >> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons >defined >> >by >> >> > >spec >> >> > > >> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan >> >> > ><zhenzhong.duan@intel.com> wrote: >> >> > >> >> >> > >> From: Yu Zhang <yu.c.zhang@linux.intel.com> >> >> > >> >> >> > >> Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. >> >> > >> Update with more detailed fault reasons listed in VT-d spec 7.2.3. >> >> > >> >> >> > >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> >> >> > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> >> > >> --- >> >> > > >> >> > >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
On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong <zhenzhong.duan@intel.com> wrote: > > > > >-----Original Message----- > >From: Jason Wang <jasowang@redhat.com> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by > >spec > > > >On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong > ><zhenzhong.duan@intel.com> wrote: > >> > >> > >> > >> >-----Original Message----- > >> >From: Jason Wang <jasowang@redhat.com> > >> >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 <yi.l.liu@intel.com> wrote: > >> >> > >> >> > From: Duan, Zhenzhong <zhenzhong.duan@intel.com> > >> >> > Sent: Monday, May 20, 2024 11:41 AM > >> >> > > >> >> > > >> >> > > >> >> > >-----Original Message----- > >> >> > >From: Jason Wang <jasowang@redhat.com> > >> >> > >Sent: Monday, May 20, 2024 8:44 AM > >> >> > >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> > >> >> > >Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, > >Chao > >> >P > >> >> > ><chao.p.peng@intel.com>; Yu Zhang <yu.c.zhang@linux.intel.com>; > >> >Michael > >> >> > >S. Tsirkin <mst@redhat.com>; Paolo Bonzini > ><pbonzini@redhat.com>; > >> >> > >Richard Henderson <richard.henderson@linaro.org>; Eduardo > >Habkost > >> >> > ><eduardo@habkost.net>; Marcel Apfelbaum > >> ><marcel.apfelbaum@gmail.com> > >> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons > >defined > >> >by > >> >> > >spec > >> >> > > > >> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan > >> >> > ><zhenzhong.duan@intel.com> wrote: > >> >> > >> > >> >> > >> From: Yu Zhang <yu.c.zhang@linux.intel.com> > >> >> > >> > >> >> > >> Currently we use only VTD_FR_PASID_TABLE_INV as fault reason. > >> >> > >> Update with more detailed fault reasons listed in VT-d spec 7.2.3. > >> >> > >> > >> >> > >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > >> >> > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > >> >> > >> --- > >> >> > > > >> >> > >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
>-----Original Message----- >From: Jason Wang <jasowang@redhat.com> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by >spec > >On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong ><zhenzhong.duan@intel.com> wrote: >> >> >> >> >-----Original Message----- >> >From: Jason Wang <jasowang@redhat.com> >> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by >> >spec >> > >> >On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong >> ><zhenzhong.duan@intel.com> wrote: >> >> >> >> >> >> >> >> >-----Original Message----- >> >> >From: Jason Wang <jasowang@redhat.com> >> >> >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 <yi.l.liu@intel.com> >wrote: >> >> >> >> >> >> > From: Duan, Zhenzhong <zhenzhong.duan@intel.com> >> >> >> > Sent: Monday, May 20, 2024 11:41 AM >> >> >> > >> >> >> > >> >> >> > >> >> >> > >-----Original Message----- >> >> >> > >From: Jason Wang <jasowang@redhat.com> >> >> >> > >Sent: Monday, May 20, 2024 8:44 AM >> >> >> > >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >> >> >> > >Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, >> >Chao >> >> >P >> >> >> > ><chao.p.peng@intel.com>; Yu Zhang ><yu.c.zhang@linux.intel.com>; >> >> >Michael >> >> >> > >S. Tsirkin <mst@redhat.com>; Paolo Bonzini >> ><pbonzini@redhat.com>; >> >> >> > >Richard Henderson <richard.henderson@linaro.org>; Eduardo >> >Habkost >> >> >> > ><eduardo@habkost.net>; Marcel Apfelbaum >> >> ><marcel.apfelbaum@gmail.com> >> >> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons >> >defined >> >> >by >> >> >> > >spec >> >> >> > > >> >> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan >> >> >> > ><zhenzhong.duan@intel.com> wrote: >> >> >> > >> >> >> >> > >> From: Yu Zhang <yu.c.zhang@linux.intel.com> >> >> >> > >> >> >> >> > >> Currently we use only VTD_FR_PASID_TABLE_INV as fault >reason. >> >> >> > >> Update with more detailed fault reasons listed in VT-d spec >7.2.3. >> >> >> > >> >> >> >> > >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> >> >> >> > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> >> >> > >> --- >> >> >> > > >> >> >> > >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
On 2024/5/27 14:04, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Jason Wang <jasowang@redhat.com> >> Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by >> spec >> >> On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong >> <zhenzhong.duan@intel.com> wrote: >>> >>> >>> >>>> -----Original Message----- >>>> From: Jason Wang <jasowang@redhat.com> >>>> Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by >>>> spec >>>> >>>> On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong >>>> <zhenzhong.duan@intel.com> wrote: >>>>> >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Jason Wang <jasowang@redhat.com> >>>>>> 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 <yi.l.liu@intel.com> >> wrote: >>>>>>> >>>>>>>> From: Duan, Zhenzhong <zhenzhong.duan@intel.com> >>>>>>>> Sent: Monday, May 20, 2024 11:41 AM >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> -----Original Message----- >>>>>>>>> From: Jason Wang <jasowang@redhat.com> >>>>>>>>> Sent: Monday, May 20, 2024 8:44 AM >>>>>>>>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >>>>>>>>> Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, >>>> Chao >>>>>> P >>>>>>>>> <chao.p.peng@intel.com>; Yu Zhang >> <yu.c.zhang@linux.intel.com>; >>>>>> Michael >>>>>>>>> S. Tsirkin <mst@redhat.com>; Paolo Bonzini >>>> <pbonzini@redhat.com>; >>>>>>>>> Richard Henderson <richard.henderson@linaro.org>; Eduardo >>>> Habkost >>>>>>>>> <eduardo@habkost.net>; Marcel Apfelbaum >>>>>> <marcel.apfelbaum@gmail.com> >>>>>>>>> Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons >>>> defined >>>>>> by >>>>>>>>> spec >>>>>>>>> >>>>>>>>> On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan >>>>>>>>> <zhenzhong.duan@intel.com> wrote: >>>>>>>>>> >>>>>>>>>> From: Yu Zhang <yu.c.zhang@linux.intel.com> >>>>>>>>>> >>>>>>>>>> Currently we use only VTD_FR_PASID_TABLE_INV as fault >> reason. >>>>>>>>>> Update with more detailed fault reasons listed in VT-d spec >> 7.2.3. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> >>>>>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>>>>>>> --- >>>>>>>>> >>>>>>>>> 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 >
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.
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 <jasowang@redhat.com> >>Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by >>spec >> >>On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong >><zhenzhong.duan@intel.com> wrote: >>> >>> >>> >>> >-----Original Message----- >>> >From: Jason Wang <jasowang@redhat.com> >>> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined >by >>> >spec >>> > >>> >On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong >>> ><zhenzhong.duan@intel.com> wrote: >>> >> >>> >> >>> >> >>> >> >-----Original Message----- >>> >> >From: Jason Wang <jasowang@redhat.com> >>> >> >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 <yi.l.liu@intel.com> >>wrote: >>> >> >> >>> >> >> > From: Duan, Zhenzhong <zhenzhong.duan@intel.com> >>> >> >> > Sent: Monday, May 20, 2024 11:41 AM >>> >> >> > >>> >> >> > >>> >> >> > >>> >> >> > >-----Original Message----- >>> >> >> > >From: Jason Wang <jasowang@redhat.com> >>> >> >> > >Sent: Monday, May 20, 2024 8:44 AM >>> >> >> > >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >>> >> >> > >Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, >>> >Chao >>> >> >P >>> >> >> > ><chao.p.peng@intel.com>; Yu Zhang >><yu.c.zhang@linux.intel.com>; >>> >> >Michael >>> >> >> > >S. Tsirkin <mst@redhat.com>; Paolo Bonzini >>> ><pbonzini@redhat.com>; >>> >> >> > >Richard Henderson <richard.henderson@linaro.org>; Eduardo >>> >Habkost >>> >> >> > ><eduardo@habkost.net>; Marcel Apfelbaum >>> >> ><marcel.apfelbaum@gmail.com> >>> >> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons >>> >defined >>> >> >by >>> >> >> > >spec >>> >> >> > > >>> >> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan >>> >> >> > ><zhenzhong.duan@intel.com> wrote: >>> >> >> > >> >>> >> >> > >> From: Yu Zhang <yu.c.zhang@linux.intel.com> >>> >> >> > >> >>> >> >> > >> Currently we use only VTD_FR_PASID_TABLE_INV as fault >>reason. >>> >> >> > >> Update with more detailed fault reasons listed in VT-d spec >>7.2.3. >>> >> >> > >> >>> >> >> > >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> >>> >> >> > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> >> >> > >> --- >>> >> >> > > >>> >> >> > >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
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
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 <jasowang@redhat.com> > >>Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by > >>spec > >> > >>On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong > >><zhenzhong.duan@intel.com> wrote: > >>> > >>> > >>> > >>> >-----Original Message----- > >>> >From: Jason Wang <jasowang@redhat.com> > >>> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined > >by > >>> >spec > >>> > > >>> >On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong > >>> ><zhenzhong.duan@intel.com> wrote: > >>> >> > >>> >> > >>> >> > >>> >> >-----Original Message----- > >>> >> >From: Jason Wang <jasowang@redhat.com> > >>> >> >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 <yi.l.liu@intel.com> > >>wrote: > >>> >> >> > >>> >> >> > From: Duan, Zhenzhong <zhenzhong.duan@intel.com> > >>> >> >> > Sent: Monday, May 20, 2024 11:41 AM > >>> >> >> > > >>> >> >> > > >>> >> >> > > >>> >> >> > >-----Original Message----- > >>> >> >> > >From: Jason Wang <jasowang@redhat.com> > >>> >> >> > >Sent: Monday, May 20, 2024 8:44 AM > >>> >> >> > >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> > >>> >> >> > >Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, > >>> >Chao > >>> >> >P > >>> >> >> > ><chao.p.peng@intel.com>; Yu Zhang > >><yu.c.zhang@linux.intel.com>; > >>> >> >Michael > >>> >> >> > >S. Tsirkin <mst@redhat.com>; Paolo Bonzini > >>> ><pbonzini@redhat.com>; > >>> >> >> > >Richard Henderson <richard.henderson@linaro.org>; Eduardo > >>> >Habkost > >>> >> >> > ><eduardo@habkost.net>; Marcel Apfelbaum > >>> >> ><marcel.apfelbaum@gmail.com> > >>> >> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons > >>> >defined > >>> >> >by > >>> >> >> > >spec > >>> >> >> > > > >>> >> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan > >>> >> >> > ><zhenzhong.duan@intel.com> wrote: > >>> >> >> > >> > >>> >> >> > >> From: Yu Zhang <yu.c.zhang@linux.intel.com> > >>> >> >> > >> > >>> >> >> > >> Currently we use only VTD_FR_PASID_TABLE_INV as fault > >>reason. > >>> >> >> > >> Update with more detailed fault reasons listed in VT-d spec > >>7.2.3. > >>> >> >> > >> > >>> >> >> > >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > >>> >> >> > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > >>> >> >> > >> --- > >>> >> >> > > > >>> >> >> > >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 >
On Mon, May 27, 2024 at 2:50 PM Michael S. Tsirkin <mst@redhat.com> 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 <jasowang@redhat.com> > > >>Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by > > >>spec > > >> > > >>On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong > > >><zhenzhong.duan@intel.com> wrote: > > >>> > > >>> > > >>> > > >>> >-----Original Message----- > > >>> >From: Jason Wang <jasowang@redhat.com> > > >>> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined > > >by > > >>> >spec > > >>> > > > >>> >On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong > > >>> ><zhenzhong.duan@intel.com> wrote: > > >>> >> > > >>> >> > > >>> >> > > >>> >> >-----Original Message----- > > >>> >> >From: Jason Wang <jasowang@redhat.com> > > >>> >> >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 <yi.l.liu@intel.com> > > >>wrote: > > >>> >> >> > > >>> >> >> > From: Duan, Zhenzhong <zhenzhong.duan@intel.com> > > >>> >> >> > Sent: Monday, May 20, 2024 11:41 AM > > >>> >> >> > > > >>> >> >> > > > >>> >> >> > > > >>> >> >> > >-----Original Message----- > > >>> >> >> > >From: Jason Wang <jasowang@redhat.com> > > >>> >> >> > >Sent: Monday, May 20, 2024 8:44 AM > > >>> >> >> > >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> > > >>> >> >> > >Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>; Peng, > > >>> >Chao > > >>> >> >P > > >>> >> >> > ><chao.p.peng@intel.com>; Yu Zhang > > >><yu.c.zhang@linux.intel.com>; > > >>> >> >Michael > > >>> >> >> > >S. Tsirkin <mst@redhat.com>; Paolo Bonzini > > >>> ><pbonzini@redhat.com>; > > >>> >> >> > >Richard Henderson <richard.henderson@linaro.org>; Eduardo > > >>> >Habkost > > >>> >> >> > ><eduardo@habkost.net>; Marcel Apfelbaum > > >>> >> ><marcel.apfelbaum@gmail.com> > > >>> >> >> > >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons > > >>> >defined > > >>> >> >by > > >>> >> >> > >spec > > >>> >> >> > > > > >>> >> >> > >On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan > > >>> >> >> > ><zhenzhong.duan@intel.com> wrote: > > >>> >> >> > >> > > >>> >> >> > >> From: Yu Zhang <yu.c.zhang@linux.intel.com> > > >>> >> >> > >> > > >>> >> >> > >> Currently we use only VTD_FR_PASID_TABLE_INV as fault > > >>reason. > > >>> >> >> > >> Update with more detailed fault reasons listed in VT-d spec > > >>7.2.3. > > >>> >> >> > >> > > >>> >> >> > >> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com> > > >>> >> >> > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > > >>> >> >> > >> --- > > >>> >> >> > > > > >>> >> >> > >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
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, };