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
