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

[Peter Xu]

On Thu, Feb 14, 2019 at 02:27:30PM +0800, Yi Sun wrote:
> 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.

You set something to zero and then you check it against zero... IMHO
it doesn't simplify the code but instead it is more confusing.  I
would prefer to only check when necessary, then drop the inits of the
stack variables.

> 
> > 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.

Yes I understand you should implement the code by following the spec.
My question was to the spec not the code.  Please feel free to skip
this question if you don't know the answer.

Regards,

-- 
Peter Xu