[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: Wed, 1 Sep 2021 10:20:20 +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=nOlixGbmhdbdoOvOwStsrHan61/add9PscjBGtXzO5k=; b=dNTQ3HSHpNyXRfHWrOWk0pbjrUHC+fKOKsnSjYug/vvm3aHYxf/EmHhjT6jk5YWNAPWxLcqzTvos3XpDJ0qyjhpl94L1FyKH1d0y8D5Hqq9kQiF/jcGTtae/7ZV0efvXHm6ggT7e7AmMzrAFnT6fQYehZsSx2QHQNIE3KfgathEbyNmwpRygoUqFyp3CmF/pDRKwuYVVEImeEA2TA/cBZzsOBe4LVcNptZ14/WeAgnteiZaNV+VwZZ4XqEsb0YIeNjTSBK31oYg6uSoRKzRoDhyxROxHPP+dONLCozd736lxMnEfmZ2UdRBQAH8S+BWFS6VROshlSNWZEOvkr7OYqg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FugZK8BywBKbAYT5Q3bZ5Ri8iaQirK+8uQe3t8N3VNOeFOqtADjqoXBH+M9WiQTumFS1TIBX4aBKbe5FkqYtEC5E/qHQxIqC5eEjihPMqxxWKz3u1h5KncnbL7OJ2uEOplpqbJGCOkO7EimS4dl87E0Enpes9h0RUNtCotPfaNnREXsEPRvHDIZD5dCtaq0Sp/C/1Y5B/rPCq16sdhPzS7P2km1tehtemNYGLZnLQtH8lncvH0zWsSGf3shufHB8QAFbSIUJwgfRY21MwiFV0ZhHlLBhpey7gIvja60ar/hSunzW4ozyfaQ4aj5XTjNt3rYp2k51tVNevwqEta+XUg==
  • 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: Wed, 01 Sep 2021 08:20:47 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 01.09.2021 06:50, Oleksandr Andrushchenko wrote:
> On 31.08.21 11:35, Jan Beulich wrote:
>> 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
>> patches.
> Unfortunately I don't have any. But, could you please at least document a bit
> more what is happening here with DomXEN: either in the commit message or
> in the code itself, so it is easier to understand why vpci has this code at 
> all...

Well, the commit message imo already says what needs saying. I'll extend
the pre-existing code comment, though. But of course the primary purpose
of this RFC is to potentially have a better approach pointed out in the
first place, which I guess will mean to wait with v1 until Roger's return.




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