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
