[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH RFC] vPCI: account for hidden devices in modify_bars()
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. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |