Re: [PATCH v3 03/11] vpci/header: Move register assignments from init_bars

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 27.10.21 16:23, Roger Pau Monné wrote:
> On Wed, Oct 27, 2021 at 11:59:47AM +0000, Oleksandr Andrushchenko wrote:
>> Hi, Roger!
>>
>> On 27.10.21 13:17, Oleksandr Andrushchenko wrote:
>>> Hi, Roger!
>>>
>>> On 13.10.21 16:51, Roger Pau Monné wrote:
>>>> On Thu, Sep 30, 2021 at 10:52:15AM +0300, Oleksandr Andrushchenko wrote:
>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>>>
>>>>> This is in preparation for dynamic assignment of the vPCI register
>>>>> handlers depending on the domain: hwdom or guest.
>>>>> The need for this step is that it is easier to have all related 
>>>>> functionality
>>>>> put at one place. When the subsequent patches add decisions on which
>>>>> handlers to install, e.g. hwdom or guest handlers, then this is easily
>>>>> achievable.
>>>> Won't it be possible to select the handlers to install in init_bars
>>>> itself?
>>> It is possible
>>>> Splitting it like that means you need to iterate over the numbers of
>>>> BARs twice (one in add_bar_handlers and one in init_bars), which makes
>>>> it more likely to introduce errors or divergences.
>>>>
>>>> Decoupling the filling of vpci_bar data with setting the handlers
>>>> seems slightly confusing.
>>> Ok, I won't introduce add_bar_handlers, thus rendering this patch useless.
>>> I'll drop it and re-work the upcoming patches with this respect
>> On the other hand after thinking a bit more.
>> What actually init_bars do?
>> 1. Runs once per each pdev (__init?)
>> 2. Sizes the BARs and detects their type, sets up pdev->vpci->header BAR 
>> values
>> 3. Adds register handlers.
>>
>> For DomU we only need 3), so we can setup guest handlers.
> I think you assume that there will always be a hardware domain with
> vPCI enabled that will get the device assigned and thus init_bars will
> be executed prior to assigning to a domU.
Yes, this is the current assumption...
>
> But what about dom0less,
it was decided to put dom0less out of scope for now
>   or when using a classic PV dom0?
I thought that vPCI is only used for PVH Dom0 and it is enough for now
(yes, this is a weak argument, but we do not want PCI passthrough on Arm
to become a never ending game... since 2015...)
>   In that case
> the device won't get assigned to a hardware domain with vPCI support,
> so the vpci structure won't be allocated or filled,
Yes, this is true. But because of the 3 functionflities of the init_bars is
doing it might still need some dis-aggregation, e.g. BAR sizing
is not needed and might not be possible while assigning to a DomU.
So, I think that init_bars will need to be split in any case.
>   and hence
> init_bars would have to be executed when assigning to a domU.
Please see above: not sure init_bars can exist in its form to achieve that.
One of the steps this patch is doing is we split init_bars into
a) register assignment
b) all the reset: initial pdev's header initialization, sizing etc.

The same is true for MSI/MSI-X. When we add support for MSI/MSI-X on Arm
you will see the same: we need to split [1] (this is WIP).

So, I am still convinced that we need add_bar_handlers in some form.
> Thanks, Roger.
>
[1] 
https://gitlab.com/rahsingh/xen-integration/-/commit/7b898601261fc3ad834ac3d06cc4c784f33c95bb