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

[Julien Grall <julien@xxxxxxx>]

On 25/10/2021 12:37, Oleksandr Andrushchenko wrote:
> Hi, Julien!
Hi Oleksandr,

> On 13.10.21 19:17, Julien Grall wrote:
> > On 08/10/2021 06:55, Oleksandr Andrushchenko wrote:
> > > +    };
> > >          naddr = dt_number_of_address(dev);
> > >    @@ -1884,7 +1889,6 @@ static int __init handle_device(struct domain *d, 
> > > struct dt_device_node *dev,
> > >        /* Give permission and map MMIOs */
> > >        for ( i = 0; i < naddr; i++ )
> > >        {
> > > -        struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
> > >            res = dt_device_get_address(dev, i, &addr, &size);
> > >            if ( res )
> > >            {
> > > @@ -1898,7 +1902,7 @@ static int __init handle_device(struct domain *d, struct 
> > > dt_device_node *dev,
> > >                return res;
> > >        }
> > >    -    res = map_device_children(d, dev, p2mt);
> > > +    res = map_device_children(dev, &mr_data);
> > >        if ( res )
> > >            return res;
> > >    @@ -3056,7 +3060,14 @@ static int __init construct_dom0(struct domain *d)
> > >            return rc;
> > >          if ( acpi_disabled )
> > > +    {
> > >            rc = prepare_dtb_hwdom(d, &kinfo);
> > > +        if ( rc < 0 )
> > > +            return rc;
> > > +#ifdef CONFIG_HAS_PCI
> > > +        rc = pci_host_bridge_mappings(d, p2m_mmio_direct_c);
> > It is not clear to me why you are passing p2m_mmio_direct_c and not p2mt here?
> There is no p2mt in the caller function, e.g. construct_dom0
> > If you really want to force a type, then I think it should be p2m_mmio_direct.
> I just followed the defaults found in:
> static int __init prepare_dtb_hwdom(struct domain *d, struct kernel_info *kinfo)
> {
>       const p2m_type_t default_p2mt = p2m_mmio_direct_c;
> which is also called from construct_dom0
We use "p2m_mmio_direct_c" (cacheable mapping) by default because we 
don't know what how the region will be accessed  (e.g. this may be an 
SRAM). With this type, there is no restriction and dom0 is responsible 
to set to proper attribute in the stage-1 page-tables.
In this hostbridge cases, I see limited reasons at the moment for 
someone to map the non-BAR regions with cacheable attributes. So I think 
it is better to chose the most restricting attribute in the stage-2 
page-table.
> > But then why is it a parameter of pci_host_bridge_mappings? Do you expect 
> > someone else to modify it?
> No, I don't expect to modify that, I just don't want PCI code to make decisions 
> on that.
> So, I feel more comfortable if that decision is taken in construct_dom0.
Only the hostbridge driver is really aware of how the region will be 
accessed. So I think it is better to let...
> So, what do we want here? Pass as parameter (then p2m_mmio_direct I guess, not 
> p2m_mmio_direct_c)?
> Or let PCI code use p2m_mmio_direct inside pci_host_bridge_mappings?
... pci_host_bridge_mappings() decide the attribute. This can 
potentially allow us to have a per-hostbridge type if needed in the future.
[...]

> > This is also exported but not used. I would prefer if we only exposed when the 
> > first external user will be introduced.
> ZynqMP is the first user yet in this patch. More to come probably later on when 
> we add other host bridges.
Ah, sorry I didn't spot the use.

Cheers,

--
Julien Grall