Re: [Qemu-devel] [PATCH] intel_iommu: a fix to vtd_dev_get_trans_type()

[Peter Xu]

On Mon, Dec 25, 2017 at 02:19:33PM +0800, Liu, Yi L wrote:
> On Mon, Dec 25, 2017 at 01:45:22PM +0800, Peter Xu wrote:
> > On Thu, Dec 21, 2017 at 02:15:19PM +0800, Liu, Yi L wrote:
> > > vtd_ce_get_type() returns uin32_t and vtd_dev_get_trans_type() returns
> > > the value from vtd_ce_get_type(). However, vtd_dev_get_trans_type()
> > > returns int. This patch switchs to return the translation type by
> > > parameter. It avoids unsigned to int transfer and also avoid potential
> > > reading confusion.
> > 
> > Frankly speaking I would still prefer the old way to do it: return
> > type when >=0 and error when <0.  After all we have a comment for
> > vtd_dev_get_trans_type() already:
> > 
> > /*
> >  * Fetch translation type for specific device. Returns <0 if error
> >  * happens, otherwise return the shifted type to check against
> >  * VTD_CONTEXT_TT_*.
> >  */
> 
> Peter,
> 
> I knew your point. It's not a bug. However, it depends on
> vtd_ce_get_type(), if it returns a value with the most significant
> bit=1, then it may be wrongly treated as a negative value. It is not
> possible so far, but it may be an issue if future spec place the
> translation type in bit 31:29, and type 3'b100 is valid. It's an
> assumption. Then the return value of vtd_ce_get_type() is 0x80000000,
> and it would be treated as -2147483648. This should be a mistake.
> That's why I want to separate the return value and the translation type.

Is it forseeable that we'll use any of the bits there?  AFAIK now 11b
is still reserved for translation type so at least we still have one
extra new type.  Even if so, I would prefer to do it when QEMU VT-d
supports that new type.  Or, convert current "shifted bitmask" into an
enum.  That would be even cleaner to me.  Thanks,

-- 
Peter Xu