On Mon, 2014-10-20 at 20:14 +0800, Le Tan wrote: > Hi Markus, > > 2014-10-20 19:41 GMT+08:00 Markus Armbruster <arm...@redhat.com>: > > "Michael S. Tsirkin" <m...@redhat.com> writes: > > > >> (((sid) >> 8) && 0xff) makes no sense > >> (((sid) >> 8) & 0xff) seems to be what was meant. > >> > >> Suggested-by: Markus Armbruster <arm...@redhat.com> > > > > Actually by the reporter of https://bugs.launchpad.net/bugs/1382477 > > > >> Signed-off-by: Michael S. Tsirkin <m...@redhat.com> > >> --- > >> > >> 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 <knut.om...@oracle.com> Knut > > Reviewed-by: Markus Armbruster <arm...@redhat.com> >