Re: [Qemu-devel] [RFC v1 2/3] intel_iommu: add 256 bits qi_desc support

[Yi Sun]

On 19-02-13 18:42:24, Peter Xu wrote:
> On Wed, Feb 13, 2019 at 05:00:41PM +0800, Yi Sun wrote:
> 
> [...]
> 
> > > >  
> > > >  /* 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.
> 
> It's ok.   But please make it a boolean.  Now iq_dw can be 0x800.
> 
Sure.

> [...]
> 
> > > >  /* 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.
> 
> I agree, it's a stack variable. However...
> 
> > That makes bug happen.
> 
> ... could you explain the bug?
> 
Below error can be observed.

qemu-system-x86_64: vtd_process_inv_desc: invalid inv desc: val[3]=10, val[2]=0 
(detect reserve non-zero)

> Regards,
> 
> -- 
> Peter Xu