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

[Peter Xu <peterx@redhat.com>]

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.

[...]

> > >  /* 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?

Regards,

-- 
Peter Xu