[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v5 2/4] xen: do not return -EEXIST if iommu_add_dt_device is called twice

  • To: Julien Grall <julien@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 3 Aug 2021 08:57:57 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=DDOJD6ysxFaxuiOooFdk4/TKPo2IFXQAkBP5yX8zKi4=; b=mmeiPx/BHky4+1HsgP+/OVQmlajkNrYDlhEgMbB+llfTUiuZSgH7SKiTTmHK6J86dCAXNsuclft+H8xXtbcFCJY3xv/sZ2XYGm06FLmnD3oTzPS9d0VLh+aR4/w+j7Yut5abmxeHIsZpPiF2O4X5U70P3HZp7Ulda6hJ9Si5+stlk/BXt0reMmmMssJq/ozMF+fXoUBizYTpbNmLG06Cy8BDiJqDqx/5g1EtUnOIxdoGTs+h2ddqKzrDntKHEl4v4ykGaqcsf/GiFr8N3px9BBAL2oPKakTmdTZNtDgP8g5H2+aD6Teh2VChRk+s/xNElR2Z7gRE5HQHpygheGXxcQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=VvBDuFjm7asv52KvGuB8mvrSEpOj4FPNNmj+4NJM5Xkm6HwQChCpodAXyN+EDjHyZplsaA+B8lBhuvXNQlo4jO8jb1Uk9LNJOePAHe37UtEckzG0qKCanAH0NYcfn/1gtra1WPI9qkiwhuDFxRek9VoyDiXhaa4qko5g3xnp698lt8mLSqICONjA6jp9hOdAu5vVShkZgsUmpE70bSpvtowD/KyMTAbXox7eK1VBZkrqbmXJ35GaTiRgZ4Zh0/w0hVxfYXqb7xXQX6heRDxhGC4tCXRSZE87GSyk2dvBXMrAYdQI6mULV/j/3ZXkcttk7IjAV/rRbPa9JSkwKjoYqw==
  • Authentication-results: kernel.org; dkim=none (message not signed) header.d=none;kernel.org; dmarc=none action=none header.from=suse.com;
  • Cc: Bertrand.Marquis@xxxxxxx, Volodymyr_Babchuk@xxxxxxxx, rahul.singh@xxxxxxx, brian.woods@xxxxxxxxxx, Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • Delivery-date: Tue, 03 Aug 2021 06:58:07 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 26.07.2021 17:45, Julien Grall wrote:
> On 23/07/2021 14:02, Jan Beulich wrote:
>> On 23.07.2021 11:28, Julien Grall wrote:
>>> On 23/07/2021 07:31, Jan Beulich wrote:
>>>> On 23.07.2021 01:36, Stefano Stabellini wrote:
>>>>> --- a/xen/drivers/passthrough/device_tree.c
>>>>> +++ b/xen/drivers/passthrough/device_tree.c
>>>>> @@ -140,8 +140,13 @@ int iommu_add_dt_device(struct dt_device_node *np)
>>>>>        if ( !ops )
>>>>>            return -EINVAL;
>>>>> +    /*
>>>>> +     * Some Device Trees may expose both legacy SMMU and generic
>>>>> +     * IOMMU bindings together. If both are present, the device
>>>>> +     * can be already added.
>>>>> +     */
>>>>>        if ( dev_iommu_fwspec_get(dev) )
>>>>> -        return -EEXIST;
>>>>> +        return 0;
>>>> Since the xen: prefix in the subject made me go look (I wouldn't have
>>>> if it had been e.g. dt: ), I may as well ask: Since previously there
>>>> was concern about bogus duplicate entries, does this concern go away
>>>> no altogether?
>>> The check wasn't originally added because of legacy vs generic binding.
>>> It was added because in some circumstances iommu_add_dt_device() could
>>> genuinely be called twice (for instance if the device is re-assigned).
>>> This was returning -EEXIST rather than 0 so the caller can decide
>>> whether it is normal that the device is already added.
>> Okay. If that distinction is of no interest anymore, then I can see
>> this wanting dropping.
>>> Calling iommu_add_dt_device() twice doesn't hurt but after patch #1
>>> (this patch should really be first), dev_iommu_fwspec_get() will return
>>> a non-NULL pointer as the legacy devices are added when the IOMMU is probed.
>>>> It's one thing for there to be a legacy and a generic
>>>> binding, but another if you found two legacy or two generic ones, I
>>>> would think.
>>> I am not quite too sure what you mean by "two legacy" and "two generic".
>>> Can you clarify it?
>> Well, I'm having trouble describing it in different terms. I mean
>> two entries of the same kind (both legacy or both generic) referring
>> to the same device, thus leading to the function recognizing the 2nd > time 
>> round that the device is already there.
> I think you are misunderstanding the purpose of this function. It is 
> called when we discover a new device rather than discovering a new entry 
> in the IOMMU. The function will then sort out what to do for the device.

I'm struggling with assigning meaning to "discovering a new entry in the
IOMMU". Otoh to "discover a new device" means the device wasn't (supposed
to be) known before, which to me means -EEXIST is appropriate.

> The legacy binding is somewhat specific because it bypass the function 
> as the discovering is done per IOMMU rather than per device.

Well, I then guess I'm lacking too much context here.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.