Re: [PATCH v6 3/7] xen/arm: setup MMIO range trap handlers for hardware domain

[Julien Grall <julien@xxxxxxx>]

On 23/11/2021 06:58, Oleksandr Andrushchenko wrote:
> Hi, Julien!
Hi Oleksandr,

> On 22.11.21 19:36, Julien Grall wrote:
> > On 18/11/2021 10:46, Oleksandr Andrushchenko wrote:
> > > On 18.11.21 09:27, Oleksandr Andrushchenko wrote:
> > > > > > +    unsigned int count;
> > > > > > +
> > > > > > +    if ( is_hardware_domain(d) )
> > > > > > +        /* For each PCI host bridge's configuration space. */
> > > > > > +        count = pci_host_get_num_bridges();
> > > > > This first part makes sense to me. But...
> > > > >
> > > > > > +    else
> > > > > ... I don't understand how the else is related to this commit. Can you clarify 
> > > > > it?
> > > > >
> > > > > > +        /*
> > > > > > +         * There's a single MSI-X MMIO handler that deals with both PBA
> > > > > > +         * and MSI-X tables per each PCI device being passed through.
> > > > > > +         * Maximum number of supported devices is 32 as virtual bus
> > > > > > +         * topology emulates the devices as embedded endpoints.
> > > > > > +         * +1 for a single emulated host bridge's configuration space.
> > > > > > +         */
> > > > > > +        count = 1;
> > > > > > +#ifdef CONFIG_HAS_PCI_MSI
> > > > > > +        count += 32;
> > > > > Surely, this is a decision that is based on other factor in the vPCI code. So 
> > > > > can use a define and avoid hardcoding the number?
> > > > Well, in the later series [1] this is defined via PCI_SLOT(~0) + 1 and there is 
> > > > no dedicated
> > > > constant for that. I can use the same here, e.g. s/32/PCI_SLOT(~0) + 1
> > I would prefer if we introduce a new constant for that. This makes easier to 
> > update the code if we decide to increase the number of virtual devices.
> >
> > However, I am still not sure how the 'else' part is related to this commit. Can 
> > you please clarify it?
> Well, yes, this is too early for this patch to introduce some future knowledge, 
> so I'll instead have:
>
> unsigned int domain_vpci_get_num_mmio_handlers(struct domain *d)
> {
>       if ( !has_vpci(d) )
>           return 0;
>
>       if ( is_hardware_domain(d) )
>       {
>           int ret = pci_host_iterate_bridges_and_count(d, 
> vpci_get_num_handlers_cb);
>
>           return ret < 0 ? 0 : ret;
>       }
>
>       /*
>        * This is a guest domain:
>        *
>        * 1 for a single emulated host bridge's configuration space.
>        */
>       return 1;
I am afraid that my question stands even with this approach. This patch 
is only meant to handle the hardware domain, therefore the change seems 
to be out of context.
I would prefer if this change is done separately.

Cheers,

--
Julien Grall