On 19-02-14 11:24:35, Peter Xu wrote:
> 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?
>
Here, my intention is to simplify the codes. If inv_desc.val[] has been
initialized to 0, the error will not happen no matter 128bits or 256bits.
> 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?
>
Although the high 128bits are not used now, they are defined by spec
so that we should handle them. At least we can know if there is error
by checking if high 128bits are 0. Furthermore, we handle them in codes
now can avoid some changes in the future.
> Regards,
>
> --
> Peter Xu
