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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Oct 27, 2021 at 02:06:40PM +0000, Oleksandr Andrushchenko wrote:
> 
> 
> 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...)

I understand that not everything will be supported, that's perfectly
fine, but we should aim to not make supporting those use cases
harder in the future.

> >   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.

I understand that BAR sizing will not be needed if the structure is
pre-initialized, but I also cannot see why it would be impossible, at
least on x86.

> >   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.

I'm fine to split it if there's a hard requirement, but I'm afraid so
far I'm not convinced it's required. Maybe if you could elaborate on
why BAR sizing might not be possible when assigning to domU I could be
convinced.

Another option might be to just modify init_bars to have slightly
different paths for dom0 vs domU.

Thanks, Roger.