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

[Peter Xu]

On Thu, Feb 14, 2019 at 09:52:04AM +0800, Yi Sun wrote:

[...]

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

Ok so you're checking val[2] & val[3] unconditionally:

    if (inv_desc.val[3] || inv_desc.val[2]) {
        error_report_once("%s: invalid inv desc: val[3]=%"PRIx64
                          ", val[2]=%"PRIx64
                          " (detect reserve non-zero)", __func__,
                          inv_desc.val[3],
                          inv_desc.val[2]);
        return false;
    }

Why?  Shouldn't they invalid if inv desc width is 128bits?

When 256 bits invalidation descriptor is used, the guest driver
should be responsible to fill in zeros into reserved fields.

Another question: is val[2] & val[3] used in any place even with
256bits mode?  From what I see from the spec (chap 6.5.2), all of them
seems to be reserved as zeros, then I don't understand why bother
extending this to 256bits...  Did I miss something?

Regards,

-- 
Peter Xu