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

From: Stefano Stabellini <sstabellini@kernel.org>
To: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	 "xen-devel@lists.xenproject.org"
	<xen-devel@lists.xenproject.org>,
	 "julien@xen.org" <julien@xen.org>,
	 Oleksandr Tyshchenko <Oleksandr_Tyshchenko@epam.com>,
	 Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	 Artem Mygaiev <Artem_Mygaiev@epam.com>,
	 "roger.pau@citrix.com" <roger.pau@citrix.com>,
	 Bertrand Marquis <bertrand.marquis@arm.com>,
	 Rahul Singh <rahul.singh@arm.com>
Subject: Re: [PATCH v2 10/11] xen/arm: Do not map PCI ECAM and MMIO space to Domain-0's p2m
Date: Tue, 28 Sep 2021 09:11:57 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.2109280911030.5022@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <32ffee9e-8786-c91b-681c-7fa243f5d3fa@epam.com>

[-- Attachment #1: Type: text/plain, Size: 3687 bytes --]

On Tue, 28 Sep 2021, Oleksandr Andrushchenko wrote:
> [snip]
> >> Sorry I didn't follow your explanation.
> >>
> >> My suggestion is to remove the #ifdef CONFIG_HAS_PCI completely from
> >> map_range_to_domain. At the beginning of map_range_to_domain, there is
> >> already this line:
> >>
> >> bool need_mapping = !dt_device_for_passthrough(dev);
> >>
> >> We can change it into:
> >>
> >> bool need_mapping = !dt_device_for_passthrough(dev) &&
> >>                       !mr_data->skip_mapping;
> >>
> >>
> >> Then, in map_device_children we can set mr_data->skip_mapping to true
> >> for PCI devices.
> > This is the key. I am fine with this, but it just means we move the
> >
> > check to the outside of this function which looks good. Will do
> >
> >>    There is already a pci check there:
> >>
> >>    if ( dt_device_type_is_equal(dev, "pci") )
> >>
> >> so it should be easy to do. What am I missing?
> >>
> >>
> >
> I did some experiments. If we move the check to map_device_children
> 
> it is not enough because part of the ranges is still mapped at handle_device level:
> 
> handle_device:
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr fd0e0000
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr fd480000
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr 8000000000
> 
> map_device_children:
> (XEN) Mapping children of /axi/pcie@fd0e0000 to guest skip 1
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr e0000000
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr 600000000
> 
> pci_host_bridge_mappings:
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr fd0e0000
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr fd480000
> 
> So, I did more intrusive change:
> 
> @@ -1540,6 +1534,12 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
>       int res;
>       u64 addr, size;
>       bool need_mapping = !dt_device_for_passthrough(dev);
> +    struct map_range_data mr_data = {
> +        .d = d,
> +        .p2mt = p2mt,
> +        .skip_mapping = is_pci_passthrough_enabled() &&
> +                        (device_get_class(dev) == DEVICE_PCI)
> +    };
> 
> With this I see that now mappings are done correctly:
> 
> handle_device:
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr fd0e0000
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr fd480000
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr 8000000000
> 
> map_device_children:
> (XEN) Mapping children of /axi/pcie@fd0e0000 to guest skip 1
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr e0000000
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 0 addr 600000000
> 
> pci_host_bridge_mappings:
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr fd0e0000
> (XEN) --- /axi/pcie@fd0e0000 need_mapping 1 addr fd480000
> 
> So, handle_device seems to be the right place. While at it I have also
> 
> optimized the way we setup struct map_range_data mr_data in both
> 
> handle_device and map_device_children: I removed structure initialization
> 
> from within the relevant loop and also pass mr_data to map_device_children,
> 
> so it doesn't need to create its own copy of the same and perform yet
> 
> another computation for .skip_mapping: it does need to not only know
> 
> that dev is a PCI device (this is done by the dt_device_type_is_equal(dev, "pci")
> 
> check, but also account on is_pci_passthrough_enabled().
> 
> Thus, the change will be more intrusive, but I hope will simplify things.
> 
> I am attaching the fixup patch for just in case you want more details.

Yes, thanks, this is what I had in mind. Hopefully the resulting
combined patch will be simpler.

Cheers,

Stefano