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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Julien!

On 28.10.21 14:13, Julien Grall wrote:
>
>
> 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.
It is all clear now, thank you!
>
> [...]
>
>>>
>>> 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,
>
Thank you,
Oleksandr