Re: [PATCH v5 7/9] xen/arm: Fix mapping for PCI bridge mmio region

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 10/20/23 14:17, Julien Grall wrote:
> Hi Stewart,
> 
> On 04/10/2023 15:55, Stewart Hildebrand wrote:
>> From: Rahul Singh <rahul.singh@xxxxxxx>
>>
>> Current code skip the mapping for PCI bridge MMIO region to dom0 when
>> pci_passthrough_enabled flag is set. Mapping should be skip when
>> has_vpci(d) is enabled for the domain, as we need to skip the mapping
>> only when VPCI handler are registered for ECAM.
>>
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>> ---
>> v4->v5:
>> * new patch
> 
> I am a bit lost. How is this a new patch but...
> 
>> * rebase on top of "dynamic node programming using overlay dtbo" series
>> * replace !is_pci_passthrough_enabled() check with 
>> !IS_ENABLED(CONFIG_HAS_PCI)
>>    instead of removing
> 
> ... there is a changelog. Did you get the patch from somewhere?

Yes, it was picked up from [1]. The changelog is describing what changed since 
getting the patch from [1]. I'll clarify this here in the next rev.

[1] https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg00483.html

> 
>> ---
>>   xen/arch/arm/device.c       | 2 +-
>>   xen/arch/arm/domain_build.c | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
>> index 1f631d327441..4d69c298858d 100644
>> --- a/xen/arch/arm/device.c
>> +++ b/xen/arch/arm/device.c
>> @@ -330,7 +330,7 @@ int handle_device(struct domain *d, struct 
>> dt_device_node *dev, p2m_type_t p2mt,
>>           .d = d,
>>           .p2mt = p2mt,
>>           .skip_mapping = !own_device ||
>> -                        (is_pci_passthrough_enabled() &&
>> +                        (has_vpci(d) &&
>>                           (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
>>           .iomem_ranges = iomem_ranges,
>>           .irq_ranges = irq_ranges
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 7da254709d17..2c55528a62d4 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1064,7 +1064,7 @@ static void __init assign_static_memory_11(struct 
>> domain *d,
>>   #endif
>>
>>   /*
>> - * When PCI passthrough is available we want to keep the
>> + * When HAS_PCI is enabled we want to keep the
>>    * "linux,pci-domain" in sync for every host bridge.
>>    *
>>    * Xen may not have a driver for all the host bridges. So we have
>> @@ -1080,7 +1080,7 @@ static int __init handle_linux_pci_domain(struct 
>> kernel_info *kinfo,
>>       uint16_t segment;
>>       int res;
>>
>> -    if ( !is_pci_passthrough_enabled() )
>> +    if ( !IS_ENABLED(CONFIG_HAS_PCI) )
> 
> I don't understand why this wants to be HAS_PCI rather than has_vcpi()?
> This also doesn't seem to be mentioned in the commit message.

This particular change was essentially preparation for reverting the 
pci-passthrough option to ensure we didn't break the ability to bisect. Since 
we will not revert the pci-passthrough option after all, I will drop this 
change in domain_build.c for the next rev.

> 
>>           return 0;
>>
>>       if ( !dt_device_type_is_equal(node, "pci") )
> 
> Cheers,
> 
> -- 
> Julien Grall