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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

[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.

Thank you,

Oleksandr

Attachment: 0001-Fixes-4480fb1a5c83-xen-arm-Do-not-map-PCI-ECAM-and-M.patch
Description: 0001-Fixes-4480fb1a5c83-xen-arm-Do-not-map-PCI-ECAM-and-M.patch