Re: [PATCH 09/11] xen/arm: Setup MMIO range trap handlers for hardware domain

[Julien Grall <julien@xxxxxxx>]

On 10/09/2021 12:43, Oleksandr Andrushchenko wrote:
> Hi, Julien!
Hi Oleksandr,

> On 09.09.21 20:43, Julien Grall wrote:
> > Hi Oleksandr,
> >
> > On 03/09/2021 09:33, Oleksandr Andrushchenko wrote:
> > > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > >
> > > In order vPCI to work it needs all access to PCI configuration space
> > > (ECAM) to be synchronized among all entities, e.g. hardware domain and
> > > guests.
> > I am not entirely sure what you mean by "synchronized" here. Are you refer to 
> > the content of the configuration space?
> We maintain hwdom's and guest's view of the registers we are interested in
>
> So, to have hwdom's view we also need to trap its access to the configuration 
> space.
>
> Probably "synchronized" is not the right wording here.
I would simply say that we want to expose an emulated hostbridge to the 
dom0 so we need to unmap the configuration space.
> > > For that implement PCI host bridge specific callbacks to
> > > properly setup those ranges depending on particular host bridge
> > > implementation.
> > >
> > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > > ---
> > >    xen/arch/arm/pci/ecam.c            | 11 +++++++++++
> > >    xen/arch/arm/pci/pci-host-common.c | 16 ++++++++++++++++
> > >    xen/arch/arm/vpci.c                | 13 +++++++++++++
> > >    xen/include/asm-arm/pci.h          |  8 ++++++++
> > >    4 files changed, 48 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
> > > index 91c691b41fdf..92ecb2e0762b 100644
> > > --- a/xen/arch/arm/pci/ecam.c
> > > +++ b/xen/arch/arm/pci/ecam.c
> > > @@ -42,6 +42,16 @@ void __iomem *pci_ecam_map_bus(struct pci_host_bridge 
> > > *bridge,
> > >        return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where;
> > >    }
> > >    +static int pci_ecam_register_mmio_handler(struct domain *d,
> > > +                                          struct pci_host_bridge *bridge,
> > > +                                          const struct mmio_handler_ops *ops)
> > > +{
> > > +    struct pci_config_window *cfg = bridge->sysdata;
> > > +
> > > +    register_mmio_handler(d, ops, cfg->phys_addr, cfg->size, NULL);
> > We have a fixed array for handling the MMIO handlers.
> Didn't know that:
>
> xen/include/asm-arm/mmio.h:27:#define MAX_IO_HANDLER  16
>
> > So you need to make sure we have enough space in it to store one handler per 
> > handler.
> >
> > This is quite similar to the problem we had with the re-distribuor on GICv3. 
> > Have a look there to see how we dealt with it.
> Could you please point me to that solution? I can only see
>
>       /* Register mmio handle for the Distributor */
>       register_mmio_handler(d, &vgic_distr_mmio_handler, d->arch.vgic.dbase,
>                             SZ_64K, NULL);
>
>       /*
>        * Register mmio handler per contiguous region occupied by the
>        * redistributors. The handler will take care to choose which
>        * redistributor is targeted.
>        */
>       for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
>       {
>           struct vgic_rdist_region *region = &d->arch.vgic.rdist_regions[i];
>
>           register_mmio_handler(d, &vgic_rdistr_mmio_handler,
>                                 region->base, region->size, region);
>       }
> which IMO doesn't care about the number of MMIOs we can handle
Please see vgic_v3_init(). We update mmio_count that is then used as a 
the second argument for domain_io_init().
Cheers,

--
Julien Grall