On 19-02-12 14:27:28, Peter Xu wrote: > On Wed, Jan 30, 2019 at 01:09:12PM +0800, Yi Sun wrote: > > From: "Liu, Yi L" <yi.l....@intel.com> > > > > Per Intel(R) VT-d 3.0, the qi_desc is 256 bits in Scalable > > Mode. This patch adds emulation of 256bits qi_desc. > > > > [Yi Sun is co-developer to rebase and refine the patch.] > > Signed-off-by: Yi Sun <yi.y....@linux.intel.com> > > Signed-off-by: Liu, Yi L <yi.l....@intel.com> > > Same here; I think you should have your s-o-b last. ;) > Sure.
> > --- > > hw/i386/intel_iommu.c | 182 > > +++++++++++++++++++++++++---------------- > > hw/i386/intel_iommu_internal.h | 8 +- > > include/hw/i386/intel_iommu.h | 1 + > > 3 files changed, 116 insertions(+), 75 deletions(-) > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index 396ac8e..3664a00 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -38,6 +38,7 @@ > > #include "trace.h" > > > > #define vtd_devfn_check(devfn) ((devfn & VTD_DEVFN_CHECK_MASK) ? true : > > false) > > +#define vtd_ecap_smts(s) ((s)->ecap & VTD_ECAP_SMTS) > > I'd prefer capital letters for macros. Your call. > Will change them. > > > > /* context entry operations */ > > #define vtd_get_ce_size(s, ce) \ > > @@ -65,6 +66,9 @@ > > #define vtd_pe_get_slpt_base(pe) ((pe)->val[0] & > > VTD_SM_PASID_ENTRY_SLPTPTR) > > #define vtd_pe_get_domain_id(pe) VTD_SM_PASID_ENTRY_DID((pe)->val[1]) > > > > +/* invalidation desc */ > > +#define vtd_get_inv_desc_width(s) ((s)->iq_dw ? 32 : 16) > > Nit: I'll prefer dropping all the "get" wordings in these macros to be > "vtd_inv_desc_width" since that "get" doesn't help much on > understanding its meanings. But it's personal preference too. > That is fine. > And since you've already have the iq_dw variable - why not store the > width directly into it? An uint8_t would suffice. > iq_dw corresponds to VTD_IQA_DW_MASK (Descriptor Width defined in IQA register). 1 means 256-bit descriptor, 0 means 128-bit descriptor. It is also used in vtd_handle_gcmd_qie() and VTD_IQT_QT() by checking if its value is 1. So, I would prefer to keep the original design. > > + > > static void vtd_address_space_refresh_all(IntelIOMMUState *s); > > static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n); > > > > @@ -1759,6 +1763,11 @@ static void vtd_root_table_setup(IntelIOMMUState *s) > > s->root_scalable = s->root & VTD_RTADDR_SMT; > > s->root &= VTD_RTADDR_ADDR_MASK(s->aw_bits); > > > > + /* if Scalable mode is not enabled, enforce iq_dw to be 16 byte */ > > + if (!s->root_scalable) { > > + s->iq_dw = 0; > > This is ok but I feel it a bit awkward to change IQ setup specifically > in root table setup. You can simply do this: > > - in vtd_init(), always set iq_dw=16. This will cover system resets > or IOMMU device reset, then > Per above explanation, I can make iq_dw=0 in vtd_init(). > - only update iq_dw to 32 in vtd_mem_write() where guest specified > May add check of s->root_scalable in vtd_mem_write(). > > + } > > + > > trace_vtd_reg_dmar_root(s->root, s->root_extended); > > } > > > > @@ -2052,7 +2061,7 @@ static void vtd_handle_gcmd_qie(IntelIOMMUState *s, > > bool en) > > if (en) { > > s->iq = iqa_val & VTD_IQA_IQA_MASK(s->aw_bits); > > /* 2^(x+8) entries */ > > - s->iq_size = 1UL << ((iqa_val & VTD_IQA_QS) + 8); > > + s->iq_size = 1UL << ((iqa_val & VTD_IQA_QS) + 8 - (s->iq_dw ? 1 : > > 0)); > > s->qi_enabled = true; > > trace_vtd_inv_qi_setup(s->iq, s->iq_size); > > /* Ok - report back to driver */ > > @@ -2219,54 +2228,66 @@ static void vtd_handle_iotlb_write(IntelIOMMUState > > *s) > > } > > > > /* Fetch an Invalidation Descriptor from the Invalidation Queue */ > > -static bool vtd_get_inv_desc(dma_addr_t base_addr, uint32_t offset, > > +static bool vtd_get_inv_desc(IntelIOMMUState *s, > > VTDInvDesc *inv_desc) > > { > > - dma_addr_t addr = base_addr + offset * sizeof(*inv_desc); > > - if (dma_memory_read(&address_space_memory, addr, inv_desc, > > - sizeof(*inv_desc))) { > > - error_report_once("Read INV DESC failed"); > > - inv_desc->lo = 0; > > - inv_desc->hi = 0; > > + dma_addr_t base_addr = s->iq; > > + uint32_t offset = s->iq_head; > > + uint32_t dw = vtd_get_inv_desc_width(s); > > + dma_addr_t addr = base_addr + offset * dw; > > + > > + /* init */ > > + inv_desc->val[0] = 0; > > + inv_desc->val[1] = 0; > > + inv_desc->val[2] = 0; > > + inv_desc->val[3] = 0; > > No need? > This is necessary. Per my test, the val[] are not 0 by default. That makes bug happen. > > + > > + if (dma_memory_read(&address_space_memory, addr, inv_desc, dw)) { > > + error_report_once("Read INV DESC failed."); > > return false; > > } > > - inv_desc->lo = le64_to_cpu(inv_desc->lo); > > - inv_desc->hi = le64_to_cpu(inv_desc->hi); > > + inv_desc->val[0] = le64_to_cpu(inv_desc->val[0]); > > + inv_desc->val[1] = le64_to_cpu(inv_desc->val[1]); > > + inv_desc->val[2] = le64_to_cpu(inv_desc->val[2]); > > + inv_desc->val[3] = le64_to_cpu(inv_desc->val[3]); > > Similar comments here on VTDInvDesc - you can keep the hi/lo fields, > but instead you can define the val[4] into the union too. Then: > > if (dw == 16) { > inv_desc->lo = ... > inv_desc->hi = ... > } else { > inv_desc->val[0] = ... > inv_desc->val[1] = ... > inv_desc->val[2] = ... > inv_desc->val[3] = ... > } > > Then this patch can be greatly simplified too since you don't need to > switch VTDInvDesc.{lo|hi} to val[0|1]. > Ok, I will consider it. > [...] > > > @@ -2525,7 +2558,7 @@ static void vtd_handle_iqt_write(IntelIOMMUState *s) > > { > > uint64_t val = vtd_get_quad_raw(s, DMAR_IQT_REG); > > > > - s->iq_tail = VTD_IQT_QT(val); > > + s->iq_tail = VTD_IQT_QT(s->iq_dw, val); > > You can check against bit 4 when iq_dw==32 and report error otherwise. > That's explicitly mentioned by the spec (chap 10.4.22). > OK, will add this check. Thanks! > > trace_vtd_inv_qi_tail(s->iq_tail); > > > > if (s->qi_enabled && !(vtd_get_long_raw(s, DMAR_FSTS_REG) & > > VTD_FSTS_IQE)) { > > [...] > > > Regards, > > -- > Peter Xu