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

Re: [PATCH RFC] vPCI: account for hidden devices in modify_bars()

  • To: Oleksandr Andrushchenko <andr2000@xxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 31 Aug 2021 10:35:04 +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=UDD8iCT0ur9si+zDyGYDe/Nu7kW3jpmLE6Gqgg2++Yk=; b=FIUsUnHWp89x2GKN8X8C6Qoxi4HRtTMaifn4UMxLbcpADSF2zI9SUFhvM+3qthW4JuBDTm89rii1Pj8PI/O0zNHDj75OPkJj71ki+QNfOA1ibd+tDEn+Be67mJp1t9iP9/pJLEF2w22nXD90MmIGTbQ8hI4wUvdruJRLIb9LUiIwIxzGoY4KtZ9AZZNqZ8GIKmUGCYwHSE5A0j8mre+LtQ7mpCS1EKzHXhsZ8nAJVagmFexjNYYdPcE9i8YdGs8J8bgz7rPLPPTfow0CSU8mhBRHv8MYdiHEfzPc8lIfPYIYhwGmO++V7WY51UpvfOvO84I3tzZqTtbdnGFnK8a3yA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=d6073pccS/mTkVIenM0f6n79zI4JjyfWT/X1mVGbsDQ0WymO2P5cfiircRW9QkGCdS8jg5s8jicqZiS39kU1nKoMolKe4ri7cE3VcnJhsVzxpUFZXnTsctOKPKrM5HfzjPh5WTvRCfSo+YFj2R5HaezDItam+QA21aByEPAhfiUjfhsxfDy+xQLrk1Tuc7fyyjfTgC9c2sw/6F7VLNtfn6dInyElewDcB4X6K3+5Iet50I7QXBUA2aQ5JNYslonrZXbo2kpAPnkO0kfS9xyOlwMcQEZ5Cw9Fn905sQXiAY/9+uQuWfcT5hD0FVpOp2qJ7dhBBPMz/Xpuwg4yCVnF6g==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 31 Aug 2021 08:35:23 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 31.08.2021 10:14, Oleksandr Andrushchenko wrote:
> On 31.08.21 11:05, Jan Beulich wrote:
>> On 31.08.2021 09:56, Oleksandr Andrushchenko wrote:
>>> On 31.08.21 10:47, Jan Beulich wrote:
>>>> On 31.08.2021 09:06, Oleksandr Andrushchenko wrote:
>>>>> On 31.08.21 09:51, Jan Beulich wrote:
>>>>>> On 31.08.2021 07:35, Oleksandr Andrushchenko wrote:
>>>>>>> On 30.08.21 16:04, Jan Beulich wrote:
>>>>>>>> @@ -265,7 +266,8 @@ static int modify_bars(const struct pci_
>>>>>>>>           * Check for overlaps with other BARs. Note that only BARs 
>>>>>>>> that are
>>>>>>>>           * currently mapped (enabled) are checked for overlaps.
>>>>>>>>           */
>>>>>>>> -    for_each_pdev ( pdev->domain, tmp )
>>>>>>>> +for ( d = pdev->domain; ; d = dom_xen ) {//todo
>>>>>>> I am not quite sure this will be correct for the cases where 
>>>>>>> pdev->domain != dom0,
>>>>>>> e.g. in the series for PCI passthrough for Arm this can be any guest. 
>>>>>>> For such cases
>>>>>>> we'll force running the loop for dom_xen which I am not sure is 
>>>>>>> desirable.
>>>>>> It is surely not desirable, but it also doesn't happen - see the
>>>>>> is_hardware_domain() check further down (keeping context below).
>>>>> Right
>>>>>>> Another question is why such a hidden device has its pdev->domain not 
>>>>>>> set correctly,
>>>>>>> so we need to work this around?
>>>>>> Please see _setup_hwdom_pci_devices() and commit e46ea4d44dc0
>>>>>> ("PCI: don't allow guest assignment of devices used by Xen")
>>>>>> introducing that temporary override. To permit limited
>>>>>> visibility to Dom0, these devices still need setting up in the
>>>>>> IOMMU for Dom0. Consequently BAR overlap detection also needs
>>>>>> to take these into account (i.e. the goal here is not just to
>>>>>> prevent triggering the ASSERT() in question).
>>>>> So, why don't we set pdev->domain = dom_xen for such devices and call
>>>>> modify_bars or something from pci_hide_device for instance (I didn't get 
>>>>> too
>>>>> much into implementation details though)? If pci_hide_device already 
>>>>> handles
>>>>> such exceptions, so it should also take care of the correct BAR overlaps 
>>>>> etc.
>>>> How would it? It runs long before Dom0 gets created, let alone when
>>>> Dom0 may make adjustments to the BAR arrangement.
>>> So, why don't we call "yet another hide function" while creating Dom0 for 
>>> that
>>> exactly reason, e.g. BAR overlap handling? E.g. make it 2-stage hide for 
>>> special
>>> devices such as console etc.
>> This might be an option, but is imo going to result not only in more
>> code churn, but also in redundant code. After all what modify_bars()
>> needs is the union of BARs from Dom0's and DomXEN's devices.
> To me DomXEN here is yet another workaround as strictly speaking
> vpci code didn't need and doesn't(?) need it at the moment. Yes, at least on 
> Arm.
> So, I do understand why you want it there, but this then does need a very
> good description of what is happening and why...
>>>> The temporary overriding of pdev->domain is because other IOMMU code
>>>> takes the domain to act upon from that field.
>>> So, you mean pdev->domain in that case is pointing to what?
>> Did you look at the function I've pointed you at? DomXEN there gets
>> temporarily overridden to Dom0.
> This looks like yet another workaround to me which is not cute.
> So, the overall solution is spread over multiple subsystems, each
> introducing something which is hard to follow

If you have any better suggestions, I'm all ears. Or feel free to send




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