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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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?

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

> 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
>
> The regions will only mapped once and never be unmapped.
>
> Cheers,
>
Thank you,

Oleksandr