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

[Julien Grall <julien@xen.org>]

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@epam.com>
>>>>>
>>>>> 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@epam.com>
>>>>> ---
>>>>>     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