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

Re: [PATCH v2 5/8] pci/arm: Use iommu_add_dt_pci_device()


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
  • Date: Thu, 18 May 2023 17:05:08 -0400
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=suse.com smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=gI3C+9aSno4SDqtQbilkfQ8C+dQxysKiAvmQPjqz+uc=; b=lk1M/dLFp0S9W82kBj2fzru6eSulFc+ZGtfp8+uvylEpp0xTAPYIbOlQFFrSqFu/qiApwN0bdY+3U+hSRNduT+PwQxEcIz19pXtdf4UPklO51ARZg7UK4+aFn/CXs7fmzCbV20/GxpSI4jTuCrmi6UsVXDsYNIa12Og9qeC8QDfPwm8hMAYeNnQjtMbzaXD7Wec9vNptKrG8ifwCfSBRwqsj2lbdwJV3R5qmxKH2lvu2tzzKekmAE5BLDjei85WM11A1g0QYp0elOIc5Zu8jX2A77Z2ws8SXE+LPObY2MSakVWwpTFCOB9dw3SPG8t87DDewtCfts7mT7qDfaDzu2w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Sq/eoa7MsJgIR48I+cvuARLPY59iuq/bbLsVeeknsYXk/8GjZxlxCUBufnzKcChZv583EAKTi094jKOuSs7LWxWVKND2MMCLuK57HLPws+ZheciBaT/gCTF3fvhu41t4yMWPd5FDLRqU3MdP609zD+Amv0SFoldesENhgqP4A65UBMafR5zpK3J9jNjqOezV1QGAh5Ze7BpCPmxQSiNFmJG+ZSFIX5za3FcSGfIMfm4lFxeeG+KbUw/h4GsbvDj6k2RqIMXksmY8+wHD5UU+ZBVchJDKFOBOA66j9Hd+ITxu1+IRhgMSnSrFsmUyYx5NOZfsf9uZwKVfrfXMYEfr7w==
  • Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>, Paul Durrant <paul@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Rahul Singh <rahul.singh@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 18 May 2023 21:05:21 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 5/15/23 03:30, Jan Beulich wrote:
> On 12.05.2023 23:03, Stewart Hildebrand wrote:
>> On 5/12/23 03:25, Jan Beulich wrote:
>>> On 11.05.2023 21:16, Stewart Hildebrand wrote:
>>>> @@ -762,9 +767,20 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>>              pdev->domain = NULL;
>>>>              goto out;
>>>>          }
>>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>> +        ret = iommu_add_dt_pci_device(pdev);
>>>> +        if ( ret < 0 )
>>>> +        {
>>>> +            printk(XENLOG_ERR "pci-iommu translation failed: %d\n", ret);
>>>> +            goto out;
>>>> +        }
>>>> +#endif
>>>>          ret = iommu_add_device(pdev);
>>>
>>> Hmm, am I misremembering that in the earlier patch you had #else to
>>> invoke the alternative behavior?
>>
>> You are remembering correctly. v1 had an #else, v2 does not.
>>
>>> Now you end up calling both functions;
>>> if that's indeed intended,
>>
>> Yes, this is intentional.
>>
>>> this may still want doing differently.
>>> Looking at the earlier patch introducing the function, I can't infer
>>> though whether that's intended: iommu_add_dt_pci_device() checks that
>>> the add_device hook is present, but then I didn't find any use of this
>>> hook. The revlog there suggests the check might be stale.
>>
>> Ah, right, the ops->add_device check is stale in the other patch. Good 
>> catch, I'll remove it there.
>>
>>> If indeed the function does only preparatory work, I don't see why it
>>> would need naming "iommu_..."; I'd rather consider pci_add_dt_device()
>>> then.
>>
>> The function has now been reduced to reading SMMU configuration data from DT 
>> and mapping RID/BDF -> AXI stream ID. However, it is still SMMU related, and 
>> it is still invoking another iommu_ops hook function, dt_xlate (which is yet 
>> another AXI stream ID translation, separate from what is being discussed 
>> here). Does this justify keeping "iommu_..." in the name? I'm not convinced 
>> pci_add_dt_device() is a good name for it either (more on this below).
> 
> The function being SMMU-related pretty strongly suggests it wants to be
> invoked via a hook. If the add_device() one isn't suitable, perhaps we
> need a new (optional) prepare_device() one? With pci_add_device() then
> calling iommu_prepare_device(), wrapping the hook invocation?
> 
> But just to be clear: A new hook would need enough justification as to
> the existing one being unsuitable.

I'll move it to the add_device hook in v3



 


Rackspace

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