Re: [PATCH 10/11] xen/arm: Do not map PCI ECAM space to Domain-0's p2m

[Julien Grall <julien@xxxxxxx>]

On 10/09/2021 15:01, Oleksandr Andrushchenko wrote:
> On 10.09.21 16:18, Julien Grall wrote:
> > On 10/09/2021 13:37, Oleksandr Andrushchenko wrote:
> > > Hi, Julien!
> > Hi Oleksandr,
> >
> > > On 09.09.21 20:58, Julien Grall wrote:
> > > > On 03/09/2021 09:33, Oleksandr Andrushchenko wrote:
> > > > > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > > > >
> > > > > Host bridge controller's ECAM space is mapped into Domain-0's p2m,
> > > > > thus it is not possible to trap the same for vPCI via MMIO handlers.
> > > > > For this to work we need to not map those while constructing the domain.
> > > > >
> > > > > Note, that during Domain-0 creation there is no pci_dev yet allocated for
> > > > > host bridges, thus we cannot match PCI host and its associated
> > > > > bridge by SBDF. Use dt_device_node field for checks instead.
> > > > >
> > > > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > > > > ---
> > > > >     xen/arch/arm/domain_build.c        |  3 +++
> > > > >     xen/arch/arm/pci/ecam.c            | 17 +++++++++++++++++
> > > > >     xen/arch/arm/pci/pci-host-common.c | 22 ++++++++++++++++++++++
> > > > >     xen/include/asm-arm/pci.h          | 12 ++++++++++++
> > > > >     4 files changed, 54 insertions(+)
> > > > >
> > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > > > index da427f399711..76f5b513280c 100644
> > > > > --- a/xen/arch/arm/domain_build.c
> > > > > +++ b/xen/arch/arm/domain_build.c
> > > > > @@ -1257,6 +1257,9 @@ static int __init map_range_to_domain(const struct 
> > > > > dt_device_node *dev,
> > > > >             }
> > > > >         }
> > > > >     +    if ( need_mapping && (device_get_class(dev) == DEVICE_PCI) ) > 
> > > > > +        need_mapping = pci_host_bridge_need_p2m_mapping(d, dev,
> > > > addr, len);
> > > >
> > > > AFAICT, with device_get_class(dev), you know whether the hostbridge is used by 
> > > > Xen. Therefore, I would expect that we don't want to map all the regions of the 
> > > > hostbridges in dom0 (including the BARs).
> > > >
> > > > Can you clarify it?
> > > We only want to trap ECAM, not MMIOs and any other memory regions as the bridge 
> > > is
> > >
> > > initialized and used by Domain-0 completely.
> > What do you mean by "used by Domain-0 completely"? The hostbridge is owned by 
> > Xen so I don't think we can let dom0 access any MMIO regions by
> > default.
> Now it's my time to ask why do you think Xen owns the bridge?
Because nothing in this series indicates either way. So I assumed this 
should be owned by Xen because it will drive it.
From what you wrote below, it sounds like this is not the case. I think 
you want to have a design document sent along the series so we can 
easily know what's the expected design and validate that the code match 
the agreement.
There was already a design document sent a few months ago. So you may 
want to refresh it (if needed) and send it again with this series.
> All the bridges are passed to Domain-0, are fully visible to Domain-0, 
> initialized in Domain-0.
>
> Enumeration etc. is done in Domain-0. So how comes that Xen owns the bridge? In 
> what way it does?
>
> Xen just accesses the ECAM when it needs it, but that's it. Xen traps ECAM 
> access - that is true.
>
> But it in no way uses MMIOs etc. of the bridge - those under direct control of 
> Domain-0
>
> > In particular, we may want to hide a device from dom0 for security reasons. 
> > This is not going to be possible if you map by default everything to dom0.
> Then the bridge most probably will become unusable as we do not have relevant 
> drivers in Xen.
>
> At best we may rely on the firmware doing the initialization for us, then yes, 
> we can control an ECAM bridge in Xen.
>
> But this is not the case now: we rely on Domain-0 to initialize and setup the 
> bridge
>
> > Instead, the BARs should be mapped on demand when dom0 when we trap access to 
> > the configuration space.
> >
> > For other regions, could you provide an example of what you are referring too?
> Registers of the bridge for example, all what is listed in "reg" property
> > > > > +
> > > > > +    /*
> > > > > +     * We do not want ECAM address space to be mapped in domain's p2m,
> > > > > +     * so we can trap access to it.
> > > > > +     */
> > > > > +    return cfg->phys_addr != addr;
> > > > > +}
> > > > > +
> > > > >     /* ECAM ops */
> > > > >     const struct pci_ecam_ops pci_generic_ecam_ops = {
> > > > >         .bus_shift  = 20,
> > > > > @@ -60,6 +76,7 @@ const struct pci_ecam_ops pci_generic_ecam_ops = {
> > > > >             .read                   = pci_generic_config_read,
> > > > >             .write                  = pci_generic_config_write,
> > > > >             .register_mmio_handler  = pci_ecam_register_mmio_handler,
> > > > > +        .need_p2m_mapping       = pci_ecam_need_p2m_mapping,
> > > > >         }
> > > > >     };
> > > > >     diff --git a/xen/arch/arm/pci/pci-host-common.c 
> > > > > b/xen/arch/arm/pci/pci-host-common.c
> > > > > index a89112bfbb7c..c04be636452d 100644
> > > > > --- a/xen/arch/arm/pci/pci-host-common.c
> > > > > +++ b/xen/arch/arm/pci/pci-host-common.c
> > > > > @@ -334,6 +334,28 @@ int pci_host_iterate_bridges(struct domain *d,
> > > > >         }
> > > > >         return 0;
> > > > >     }
> > > > > +
> > > > > +bool pci_host_bridge_need_p2m_mapping(struct domain *d,
> > > > > +                                      const struct dt_device_node *node,
> > > > > +                                      uint64_t addr, uint64_t len)
> > > > > +{
> > > > > +    struct pci_host_bridge *bridge;
> > > > > +
> > > > > +    list_for_each_entry( bridge, &pci_host_bridges, node )
> > > > > +    {
> > > > > +        if ( bridge->dt_node != node )
> > > > > +            continue;
> > > > > +
> > > > > +        if ( !bridge->ops->need_p2m_mapping )
> > > > > +            return true;
> > > > > +
> > > > > +        return bridge->ops->need_p2m_mapping(d, bridge, addr, len);
> > > > > +    }
> > > > > +    printk(XENLOG_ERR "Unable to find PCI bridge for %s segment %d, addr 
> > > > > %lx\n",
> > > > > +           node->full_name, bridge->segment, addr);
> > > > > +    return true;
> > > > > +}
> > > > If you really need to map the hostbridges, then I would suggest to defer the 
> > > > P2M mappings for all of them and then walk all the bridge in one go to do the 
> > > > mappings.
> > > >
> > > > This would avoid going through the bridges every time during setup.
> > > Well, this can be done, but: my implementation prevents mappings and what
> > >
> > > you suggest will require unmapping. So, the cost in my solution is we need
> > >
> > > to go over all the bridges (until we find the one we need) and in what you
> > >
> > > suggest we'll need to unmap what we have just mapped.
> > >
> > > I think preventing unneeded mappings is cheaper than unmapping afterwards.
> > I think you misundertood what I am suggesting. What I said is you could defer 
> > the mappings (IOW not do the mapping) until later for the hostbridges.
> For each device tree device we call
>
> static int __init map_range_to_domain(const struct dt_device_node *dev,
>                                         u64 addr, u64 len,
>                                         void *data)
> which will call
>
>           res = map_regions_p2mt(d,
> So, ECAM will be mapped and then we'll need to unmap it
Well yes in the current code. But I specifically wrote "defer" as in 
adding a check "if hostbridge then return 0".
> > And then you can walk all the hostbridges to decide how to map them.
> We don't want to map ECAM, we want to trap it
I know that... But as you wrote above, you want to map other regions of 
the hostbridge. So if you defer *all* the mappings for the hostbridge, 
then you can walk the hostbridges and check decide which regions should 
be mapped.
Cheers,

--
Julien Grall