Re: [Qemu-devel] [PATCH] intel_iommu: fix VTD_SID_TO_BUS

[Knut Omang]

On Mon, 2014-10-20 at 20:14 +0800, Le Tan wrote:
> Hi Markus,
> 
> 2014-10-20 19:41 GMT+08:00 Markus Armbruster <address@hidden>:
> > "Michael S. Tsirkin" <address@hidden> writes:
> >
> >> (((sid) >> 8) && 0xff)  makes no sense
> >> (((sid) >> 8) & 0xff) seems to be what was meant.
> >>
> >> Suggested-by: Markus Armbruster <address@hidden>
> >
> > Actually by the reporter of https://bugs.launchpad.net/bugs/1382477
> >
> >> Signed-off-by: Michael S. Tsirkin <address@hidden>
> >> ---
> >>
> >> Compile-tested only.
> >>
> >>  include/hw/i386/intel_iommu.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> >> index f4701e1..e321ee4 100644
> >> --- a/include/hw/i386/intel_iommu.h
> >> +++ b/include/hw/i386/intel_iommu.h
> >> @@ -37,7 +37,7 @@
> >>  #define VTD_PCI_DEVFN_MAX           256
> >>  #define VTD_PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
> >>  #define VTD_PCI_FUNC(devfn)         ((devfn) & 0x07)
> >> -#define VTD_SID_TO_BUS(sid)         (((sid) >> 8) && 0xff)
> >> +#define VTD_SID_TO_BUS(sid)         (((sid) >> 8) & 0xff)
> >>  #define VTD_SID_TO_DEVFN(sid)       ((sid) & 0xff)
> >>
> >>  #define DMAR_REG_SIZE               0x230
> >
> > Can't find the spec right now, but it looks plausible enough.
> 
> Yes, this is a typo. I am sorry that I introduced such a mistake.
> The spec is here in Section 3.4 :
> http://www.intel.com/content/www/us/en/intelligent-systems/intel-technology/vt-directed-io-spec.html
> 
> VTD_SID_TO_BUS(sid) is intended to be used to get the bus id from the
> source identifier.
> 
> Thanks very much!
> 
> Regards,
> Le
> 
> > Only use is in vtd_context_device_invalidate().  Bug's impact isn't
> > obvious to me.

It means that invalidation will not work as intended if a device is
place on another bus than 01:xx.x (or 0 in theory) as this bus_num
always evaluate to 1 or 0 as a boolean. I have been doing quite some
testing of the virtual iommu, but by luck or unluck depending on
viewpoint my device so far has always been in bus 1...

Note that input is always supposed to be a 16 bit value here so the and
is in theory not really necessary unless from a documentation and
precaution point of view.

Reviewed-by: Knut Omang <address@hidden>

Knut

> > Reviewed-by: Markus Armbruster <address@hidden>
>