From mboxrd@z Thu Jan  1 00:00:00 1970
From: Malcolm Crossley <malcolm.crossley@citrix.com>
Subject: Re: [PATCH v3] IOMMU: Prevent VT-d device IOTLB
 operations on wrong IOMMU
Date: Wed, 18 Jun 2014 11:24:28 +0100
Message-ID: <53A168DC.1040805@citrix.com>
References: <0ec71ff08df1f10b1d41.1403015866@malcolmc.uk.xensource.com>
	<53A07589020000780001B253@mail.emea.novell.com>
	<53A06993.9010608@citrix.com>
	<53A1756C02000078000B697C@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <53A1756C02000078000B697C@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <jbeulich@suse.com>
Cc: kevin.tian@intel.com, andrew.cooper3@citrix.com, xen-devel@lists.xen.org, paul.durrant@citrix.com, Aravind.Gopalakrishnan@amd.com, suravee.suthikulpanit@amd.com, yang.z.zhang@intel.com
List-Id: xen-devel@lists.xenproject.org

On 18/06/14 11:18, Jan Beulich wrote:
>>>> Malcolm Crossley <malcolm.crossley@citrix.com> 06/17/14 6:15 PM >>>
>> On 17/06/14 16:06, Jan Beulich wrote:
>>>>>> On 17.06.14 at 16:37, <malcolm.crossley@citrix.com> wrote:
>>>> --- a/xen/drivers/passthrough/vtd/x86/ats.c
>>>> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
>>>> @@ -120,6 +120,10 @@ int dev_invalidate_iotlb(struct iommu *i
>>>>      {
>>>>          sid = (pdev->bus << 8) | pdev->devfn;
>>>>  
>>>> +        /* Only invalidate devices that belong to this IOMMU */
>>>> +        if ( !pdev->iommu || pdev->iommu != iommu )
>>>> +            continue;
>>>
>>> I meant to ask before and then forgot: What is the first half of this
>>> condition good/needed for?
>>>
>> Defensive coding to prevent a NULL pointer deference if the Intel or
>> core ATS code goes wrong.
> 
> But there is no de-reference of the pointer anywhere - you added it just as
> a token for comparison purposes. If pdev->iommu was NULL, the right side
> comparison should still produce "false"...
> 

Your right, I'm going crazy and thinking I'm dereferencing the IOMMU
pointer when I'm not. (A non submitted version of the code was looking
at the index field of struct IOMMU) Do you want me to resubmit an
updated patch or are you happy to fix it up on commit?

Malcolm

> Jan
>