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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 23/11/2021 16:41, Oleksandr Andrushchenko wrote:
> On 23.11.21 18:12, Julien Grall wrote:
> > On 23/11/2021 06:58, Oleksandr Andrushchenko wrote:
> > > 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.
> While I do agree that MSI part and virtual bus topology are not belonging to 
> this
> patch I can't agree with the rest: we already have MMIO handlers for guest 
> domains
> and we introduce domain_vpci_get_num_mmio_handlers which must also account
> on guests and stay consistent.
> So, despite the patch has "hardware domain" in its name it doesn't mean we 
> should
> break guests here.
We were already registering the handler for guest domain before your 
patch. So this is nothing new.
At the moment, we always allocate an extra 16 slot for IO handlers (see 
MAX_IO_HANDLER). So we are not breaking anything. Instead, this is 
simply a latent bug.
> Thus I do think the above is still correct wrt this patch.
The idea of splitting patch is to separate bug fix from new code. This 
helps backporting and review.
In this case, we don't care about backport (PCI passthrough is no 
supported) and the fix a simple enough. So I am not going to insist on 
splitting to a separate patch.
However, this change *must* be explained in the commit message.

Cheers,

--
Julien Grall