pci_get_pdev_by_domain() and pci_get_pdev() functions find the pdev in
the pseg list. If pdev is not in the pseg list, the functions will try
to find the pdev in the next segment. It is not right to find the pdev
in the next segment as this will result in the corruption of another
device in a different segment with the same BDF.

An issue that was observed when implementing the PCI passthrough on ARM.
When we deassign the device from domU guest, the device is assigned
to dom_io and not to dom0, but the tool stack that runs in dom0 will try
to configure the device from dom0. vpci will find the device based on
conversion of GPA to SBDF and will try to find the device in dom0, but
because device is assigned to dom_io, pci_get_pdev_by_domain() will
return pdev with same BDF from next segment.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/passthrough/pci.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 938821e593..29356d59a7 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -593,13 +593,10 @@ struct pci_dev *pci_get_pdev(int seg, int bus, int devfn)
             return NULL;
     }
 
-    do {
-        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
-            if ( (pdev->bus == bus || bus == -1) &&
-                 (pdev->devfn == devfn || devfn == -1) )
-                return pdev;
-    } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
-                                     pseg->nr + 1, 1) );
+    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
+        if ( (pdev->bus == bus || bus == -1) &&
+             (pdev->devfn == devfn || devfn == -1) )
+            return pdev;
 
     return NULL;
 }
@@ -642,14 +639,11 @@ struct pci_dev *pci_get_pdev_by_domain(const struct domain *d, int seg,
             return NULL;
     }
 
-    do {
-        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
-            if ( (pdev->bus == bus || bus == -1) &&
-                 (pdev->devfn == devfn || devfn == -1) &&
-                 (pdev->domain == d) )
-                return pdev;
-    } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
-                                     pseg->nr + 1, 1) );
+    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
+        if ( (pdev->bus == bus || bus == -1) &&
+             (pdev->devfn == devfn || devfn == -1) &&
+             (pdev->domain == d) )
+            return pdev;
 
     return NULL;
 }
-- 
2.25.1

On 05.08.2022 17:43, Rahul Singh wrote:
> pci_get_pdev_by_domain() and pci_get_pdev() functions find the pdev in
> the pseg list. If pdev is not in the pseg list, the functions will try
> to find the pdev in the next segment. It is not right to find the pdev
> in the next segment as this will result in the corruption of another
> device in a different segment with the same BDF.
> 
> An issue that was observed when implementing the PCI passthrough on ARM.
> When we deassign the device from domU guest, the device is assigned
> to dom_io and not to dom0, but the tool stack that runs in dom0 will try
> to configure the device from dom0. vpci will find the device based on
> conversion of GPA to SBDF and will try to find the device in dom0, but
> because device is assigned to dom_io, pci_get_pdev_by_domain() will
> return pdev with same BDF from next segment.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

Actually one more thing: While you're working on vPCI as I understand,
the subject prefix here really wants to mention PCI, not vPCI.

Jan

Hi Jan,

> On 9 Aug 2022, at 4:15 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 05.08.2022 17:43, Rahul Singh wrote:
>> pci_get_pdev_by_domain() and pci_get_pdev() functions find the pdev in
>> the pseg list. If pdev is not in the pseg list, the functions will try
>> to find the pdev in the next segment. It is not right to find the pdev
>> in the next segment as this will result in the corruption of another
>> device in a different segment with the same BDF.
>> 
>> An issue that was observed when implementing the PCI passthrough on ARM.
>> When we deassign the device from domU guest, the device is assigned
>> to dom_io and not to dom0, but the tool stack that runs in dom0 will try
>> to configure the device from dom0. vpci will find the device based on
>> conversion of GPA to SBDF and will try to find the device in dom0, but
>> because device is assigned to dom_io, pci_get_pdev_by_domain() will
>> return pdev with same BDF from next segment.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> Actually one more thing: While you're working on vPCI as I understand,
> the subject prefix here really wants to mention PCI, not vPCI.

Ack.

Regards,
Rahul

On 05.08.2022 17:43, Rahul Singh wrote:
> pci_get_pdev_by_domain() and pci_get_pdev() functions find the pdev in
> the pseg list. If pdev is not in the pseg list, the functions will try
> to find the pdev in the next segment. It is not right to find the pdev
> in the next segment as this will result in the corruption of another
> device in a different segment with the same BDF.
> 
> An issue that was observed when implementing the PCI passthrough on ARM.
> When we deassign the device from domU guest, the device is assigned
> to dom_io and not to dom0, but the tool stack that runs in dom0 will try
> to configure the device from dom0. vpci will find the device based on
> conversion of GPA to SBDF and will try to find the device in dom0, but
> because device is assigned to dom_io, pci_get_pdev_by_domain() will
> return pdev with same BDF from next segment.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

This wants a Fixes: tag.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -593,13 +593,10 @@ struct pci_dev *pci_get_pdev(int seg, int bus, int devfn)
>              return NULL;
>      }
>  
> -    do {
> -        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> -            if ( (pdev->bus == bus || bus == -1) &&
> -                 (pdev->devfn == devfn || devfn == -1) )
> -                return pdev;
> -    } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
> -                                     pseg->nr + 1, 1) );
> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> +        if ( (pdev->bus == bus || bus == -1) &&
> +             (pdev->devfn == devfn || devfn == -1) )
> +            return pdev;
>  
>      return NULL;
>  }
> @@ -642,14 +639,11 @@ struct pci_dev *pci_get_pdev_by_domain(const struct domain *d, int seg,
>              return NULL;
>      }
>  
> -    do {
> -        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> -            if ( (pdev->bus == bus || bus == -1) &&
> -                 (pdev->devfn == devfn || devfn == -1) &&
> -                 (pdev->domain == d) )
> -                return pdev;
> -    } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
> -                                     pseg->nr + 1, 1) );
> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> +        if ( (pdev->bus == bus || bus == -1) &&
> +             (pdev->devfn == devfn || devfn == -1) &&
> +             (pdev->domain == d) )
> +            return pdev;
>  
>      return NULL;
>  }

Indeed present behavior is wrong - thanks for spotting. However in
both cases you're moving us from one wrongness to another: The
lookup of further segments _is_ necessary when the incoming "seg"
is -1 (and apparently when this logic was introduced that was the
only case considered).

As an aside - my mail UI shows me unexpected threading between
this patch and two subsequent ones. If they were actually meant
to be a series, can they please be marked n/3?

Jan

Hi Jan,

> On 9 Aug 2022, at 11:02 am, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 05.08.2022 17:43, Rahul Singh wrote:
>> pci_get_pdev_by_domain() and pci_get_pdev() functions find the pdev in
>> the pseg list. If pdev is not in the pseg list, the functions will try
>> to find the pdev in the next segment. It is not right to find the pdev
>> in the next segment as this will result in the corruption of another
>> device in a different segment with the same BDF.
>> 
>> An issue that was observed when implementing the PCI passthrough on ARM.
>> When we deassign the device from domU guest, the device is assigned
>> to dom_io and not to dom0, but the tool stack that runs in dom0 will try
>> to configure the device from dom0. vpci will find the device based on
>> conversion of GPA to SBDF and will try to find the device in dom0, but
>> because device is assigned to dom_io, pci_get_pdev_by_domain() will
>> return pdev with same BDF from next segment.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> This wants a Fixes: tag.

Ack. 
> 
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -593,13 +593,10 @@ struct pci_dev *pci_get_pdev(int seg, int bus, int devfn)
>>             return NULL;
>>     }
>> 
>> -    do {
>> -        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>> -            if ( (pdev->bus == bus || bus == -1) &&
>> -                 (pdev->devfn == devfn || devfn == -1) )
>> -                return pdev;
>> -    } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
>> -                                     pseg->nr + 1, 1) );
>> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>> +        if ( (pdev->bus == bus || bus == -1) &&
>> +             (pdev->devfn == devfn || devfn == -1) )
>> +            return pdev;
>> 
>>     return NULL;
>> }
>> @@ -642,14 +639,11 @@ struct pci_dev *pci_get_pdev_by_domain(const struct domain *d, int seg,
>>             return NULL;
>>     }
>> 
>> -    do {
>> -        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>> -            if ( (pdev->bus == bus || bus == -1) &&
>> -                 (pdev->devfn == devfn || devfn == -1) &&
>> -                 (pdev->domain == d) )
>> -                return pdev;
>> -    } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
>> -                                     pseg->nr + 1, 1) );
>> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>> +        if ( (pdev->bus == bus || bus == -1) &&
>> +             (pdev->devfn == devfn || devfn == -1) &&
>> +             (pdev->domain == d) )
>> +            return pdev;
>> 
>>     return NULL;
>> }
> 
> Indeed present behavior is wrong - thanks for spotting. However in
> both cases you're moving us from one wrongness to another: The
> lookup of further segments _is_ necessary when the incoming "seg"
> is -1 (and apparently when this logic was introduced that was the
> only case considered).

If I understand correctly then fixed code should be like this:                                        
   
—snip— 
….                                                                  
    if ( !pseg )                                                                
    {                                                                           
        if ( seg == -1 )                                                        
            radix_tree_gang_lookup(&pci_segments, (void **)&pseg, 0, 1);        
        if ( !pseg )                                                            
            return NULL;                                                        
                                                                                
        do {                                                                    
        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )         
            if ( (pdev->bus == bus || bus == -1) &&                             
                 (pdev->devfn == devfn || devfn == -1) )                        
                return pdev;                                                    
        } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,         
                                     pseg->nr + 1, 1) );                        
        return NULL;                                                            
    }                                                                           
                                                                                
    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )             
        if ( (pdev->bus == bus || bus == -1) &&                                 
             (pdev->devfn == devfn || devfn == -1) )                            
            return pdev;                                                        
                                                                                
    return NULL;                                                                
}  


> 
> As an aside - my mail UI shows me unexpected threading between
> this patch and two subsequent ones. If they were actually meant
> to be a series, can they please be marked n/3?

Sorry for the confusion all the patches are independent of each other.
Maybe this is because I send them via a single git send-mail command.
I will fix that in the next version. 

Regards,
Rahul

On 09.08.2022 17:06, Rahul Singh wrote:
>> On 9 Aug 2022, at 11:02 am, Jan Beulich <jbeulich@suse.com> wrote:
>> On 05.08.2022 17:43, Rahul Singh wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -593,13 +593,10 @@ struct pci_dev *pci_get_pdev(int seg, int bus, int devfn)
>>>             return NULL;
>>>     }
>>>
>>> -    do {
>>> -        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>> -            if ( (pdev->bus == bus || bus == -1) &&
>>> -                 (pdev->devfn == devfn || devfn == -1) )
>>> -                return pdev;
>>> -    } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
>>> -                                     pseg->nr + 1, 1) );
>>> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>> +        if ( (pdev->bus == bus || bus == -1) &&
>>> +             (pdev->devfn == devfn || devfn == -1) )
>>> +            return pdev;
>>>
>>>     return NULL;
>>> }
>>> @@ -642,14 +639,11 @@ struct pci_dev *pci_get_pdev_by_domain(const struct domain *d, int seg,
>>>             return NULL;
>>>     }
>>>
>>> -    do {
>>> -        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>> -            if ( (pdev->bus == bus || bus == -1) &&
>>> -                 (pdev->devfn == devfn || devfn == -1) &&
>>> -                 (pdev->domain == d) )
>>> -                return pdev;
>>> -    } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
>>> -                                     pseg->nr + 1, 1) );
>>> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>> +        if ( (pdev->bus == bus || bus == -1) &&
>>> +             (pdev->devfn == devfn || devfn == -1) &&
>>> +             (pdev->domain == d) )
>>> +            return pdev;
>>>
>>>     return NULL;
>>> }
>>
>> Indeed present behavior is wrong - thanks for spotting. However in
>> both cases you're moving us from one wrongness to another: The
>> lookup of further segments _is_ necessary when the incoming "seg"
>> is -1 (and apparently when this logic was introduced that was the
>> only case considered).
> 
> If I understand correctly then fixed code should be like this:                                        
>    
> —snip— 
> ….                                                                  
>     if ( !pseg )                                                                
>     {                                                                           
>         if ( seg == -1 )                                                        
>             radix_tree_gang_lookup(&pci_segments, (void **)&pseg, 0, 1);        
>         if ( !pseg )                                                            
>             return NULL;                                                        
>                                                                                 
>         do {                                                                    
>         list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )         
>             if ( (pdev->bus == bus || bus == -1) &&                             
>                  (pdev->devfn == devfn || devfn == -1) )                        
>                 return pdev;                                                    
>         } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,         
>                                      pseg->nr + 1, 1) );                        
>         return NULL;                                                            
>     }                                                                           
>                                                                                 
>     list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )             
>         if ( (pdev->bus == bus || bus == -1) &&                                 
>              (pdev->devfn == devfn || devfn == -1) )                            
>             return pdev;                                                        
>                                                                                 
>     return NULL;                                                                
> }  

That would about double the code in the functions. Imo all it takes
is to alter the while() conditions, prefixing what is there with
"seg == -1 &&".

Actually while looking there I've noticed the get_pseg() uses in
both functions aren't quite right for the "seg == -1" case either.
I'll make a patch there, which I think shouldn't collide with yours.

Jan

Hi Jan,

> On 9 Aug 2022, at 4:13 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 09.08.2022 17:06, Rahul Singh wrote:
>>> On 9 Aug 2022, at 11:02 am, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 05.08.2022 17:43, Rahul Singh wrote:
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -593,13 +593,10 @@ struct pci_dev *pci_get_pdev(int seg, int bus, int devfn)
>>>>            return NULL;
>>>>    }
>>>> 
>>>> -    do {
>>>> -        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>>> -            if ( (pdev->bus == bus || bus == -1) &&
>>>> -                 (pdev->devfn == devfn || devfn == -1) )
>>>> -                return pdev;
>>>> -    } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
>>>> -                                     pseg->nr + 1, 1) );
>>>> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>>> +        if ( (pdev->bus == bus || bus == -1) &&
>>>> +             (pdev->devfn == devfn || devfn == -1) )
>>>> +            return pdev;
>>>> 
>>>>    return NULL;
>>>> }
>>>> @@ -642,14 +639,11 @@ struct pci_dev *pci_get_pdev_by_domain(const struct domain *d, int seg,
>>>>            return NULL;
>>>>    }
>>>> 
>>>> -    do {
>>>> -        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>>> -            if ( (pdev->bus == bus || bus == -1) &&
>>>> -                 (pdev->devfn == devfn || devfn == -1) &&
>>>> -                 (pdev->domain == d) )
>>>> -                return pdev;
>>>> -    } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
>>>> -                                     pseg->nr + 1, 1) );
>>>> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
>>>> +        if ( (pdev->bus == bus || bus == -1) &&
>>>> +             (pdev->devfn == devfn || devfn == -1) &&
>>>> +             (pdev->domain == d) )
>>>> +            return pdev;
>>>> 
>>>>    return NULL;
>>>> }
>>> 
>>> Indeed present behavior is wrong - thanks for spotting. However in
>>> both cases you're moving us from one wrongness to another: The
>>> lookup of further segments _is_ necessary when the incoming "seg"
>>> is -1 (and apparently when this logic was introduced that was the
>>> only case considered).
>> 
>> If I understand correctly then fixed code should be like this:                                        
>> 
>> —snip— 
>> ….                                                                  
>>    if ( !pseg )                                                                
>>    {                                                                           
>>        if ( seg == -1 )                                                        
>>            radix_tree_gang_lookup(&pci_segments, (void **)&pseg, 0, 1);        
>>        if ( !pseg )                                                            
>>            return NULL;                                                        
>> 
>>        do {                                                                    
>>        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )         
>>            if ( (pdev->bus == bus || bus == -1) &&                             
>>                 (pdev->devfn == devfn || devfn == -1) )                        
>>                return pdev;                                                    
>>        } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,         
>>                                     pseg->nr + 1, 1) );                        
>>        return NULL;                                                            
>>    }                                                                           
>> 
>>    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )             
>>        if ( (pdev->bus == bus || bus == -1) &&                                 
>>             (pdev->devfn == devfn || devfn == -1) )                            
>>            return pdev;                                                        
>> 
>>    return NULL;                                                                
>> }  
> 
> That would about double the code in the functions. Imo all it takes
> is to alter the while() conditions, prefixing what is there with
> "seg == -1 &&".

I agree with you this will avoid duplication of the code.

> 
> Actually while looking there I've noticed the get_pseg() uses in
> both functions aren't quite right for the "seg == -1" case either.
> I'll make a patch there, which I think shouldn't collide with yours.

Ok. I will test the patch once you sent it..

Regards,
Rahul

When devices are deassigned/assigned, SMMU global fault is observed
because SMEs are freed in detach function and not allocated again when
the device is assigned back to the guest.

Don't free the SMEs when devices are deassigned, set the s2cr to type
fault. This way the SMMU will generate a fault if a DMA access is done
by a device not assigned to a guest

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 32 +++++++++++++++---------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 69511683b4..141948decd 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1598,21 +1598,6 @@ out_err:
 	return ret;
 }
 
-static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg)
-{
-    struct arm_smmu_device *smmu = cfg->smmu;
-	int i, idx;
-	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
-
-	spin_lock(&smmu->stream_map_lock);
-	for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
-		if (arm_smmu_free_sme(smmu, idx))
-			arm_smmu_write_sme(smmu, idx);
-		cfg->smendx[i] = INVALID_SMENDX;
-	}
-	spin_unlock(&smmu->stream_map_lock);
-}
-
 static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 				      struct arm_smmu_master_cfg *cfg)
 {
@@ -1635,6 +1620,20 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 	return 0;
 }
 
+static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
+				      struct arm_smmu_master_cfg *cfg)
+{
+	int i, idx;
+	struct arm_smmu_device *smmu = smmu_domain->smmu;
+	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
+	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
+
+	for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
+		s2cr[idx] = s2cr_init_val;
+		arm_smmu_write_s2cr(smmu, idx);
+	}
+}
+
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
 	int ret;
@@ -1684,10 +1683,11 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 static void arm_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
 {
+	struct arm_smmu_domain *smmu_domain = domain->priv;
 	struct arm_smmu_master_cfg *cfg = find_smmu_master_cfg(dev);
 
 	if (cfg)
-		arm_smmu_master_free_smes(cfg);
+		return arm_smmu_domain_remove_master(smmu_domain, cfg);
 
 }
 
-- 
2.25.1

Hi Rahul,

title: The driver is for both smmuv1 and v2. Are you suggesting the 
issue only occurs on v1?

On 05/08/2022 16:43, Rahul Singh wrote:
> When devices are deassigned/assigned, SMMU global fault is observed
> because SMEs are freed in detach function and not allocated again when
> the device is assigned back to the guest.
> 
> Don't free the SMEs when devices are deassigned, set the s2cr to type
> fault. This way the SMMU will generate a fault if a DMA access is done
> by a device not assigned to a guest
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

AFAICT, this is fixing 0435784cc75d ("xen/arm: smmuv1: Intelligent SMR 
allocation"). If I am correct, can you add a Fixes tag?

> ---
>   xen/drivers/passthrough/arm/smmu.c | 32 +++++++++++++++---------------
>   1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 69511683b4..141948decd 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -1598,21 +1598,6 @@ out_err:
>   	return ret;
>   }
>   
> -static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg)

IIUC, the function needs to be moved because you need to use 
arm_smmu_write_s2cr(). If so, I would suggest to mention in the commit 
message because at first it seems unwarranted.

> -{
> -    struct arm_smmu_device *smmu = cfg->smmu;
> -	int i, idx;
> -	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
> -
> -	spin_lock(&smmu->stream_map_lock);
> -	for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
> -		if (arm_smmu_free_sme(smmu, idx))
> -			arm_smmu_write_sme(smmu, idx);
> -		cfg->smendx[i] = INVALID_SMENDX;
> -	}
> -	spin_unlock(&smmu->stream_map_lock);
> -}
> -
>   static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>   				      struct arm_smmu_master_cfg *cfg)
>   {
> @@ -1635,6 +1620,20 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>   	return 0;
>   }
>   
> +static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
> +				      struct arm_smmu_master_cfg *cfg)
> +{
> +	int i, idx;

NIT: I would suggest to take the opportunity to switch to "unsigned int" 
and ...

> +	struct arm_smmu_device *smmu = smmu_domain->smmu;
> +	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
> +	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);

... use const here. "cfg" and "smmu" can't be consistent but 
"smmu_domain" technically could (thanks to how C works). That said, I 
quite dislike it as the code ends up to be confusing...

> +
> +	for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
> +		s2cr[idx] = s2cr_init_val;
> +		arm_smmu_write_s2cr(smmu, idx);
> +	}
> +}
> +
>   static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   {
>   	int ret;
> @@ -1684,10 +1683,11 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   
>   static void arm_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
>   {
> +	struct arm_smmu_domain *smmu_domain = domain->priv;
>   	struct arm_smmu_master_cfg *cfg = find_smmu_master_cfg(dev);
>   
>   	if (cfg)
> -		arm_smmu_master_free_smes(cfg);
> +		return arm_smmu_domain_remove_master(smmu_domain, cfg);

Why are you using adding a 'return' here?

Cheers,

-- 
Julien Grall

Hi Julien,

> On 9 Aug 2022, at 7:19 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> title: The driver is for both smmuv1 and v2. Are you suggesting the issue only occurs on v1?

Issue occurs on both v1 & v2. I will fix this in next version.
> 
> On 05/08/2022 16:43, Rahul Singh wrote:
>> When devices are deassigned/assigned, SMMU global fault is observed
>> because SMEs are freed in detach function and not allocated again when
>> the device is assigned back to the guest.
>> Don't free the SMEs when devices are deassigned, set the s2cr to type
>> fault. This way the SMMU will generate a fault if a DMA access is done
>> by a device not assigned to a guest
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> AFAICT, this is fixing 0435784cc75d ("xen/arm: smmuv1: Intelligent SMR allocation"). If I am correct, can you add a Fixes tag?

Yes, I will add the fixes tag.
> 
>> ---
>>  xen/drivers/passthrough/arm/smmu.c | 32 +++++++++++++++---------------
>>  1 file changed, 16 insertions(+), 16 deletions(-)
>> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
>> index 69511683b4..141948decd 100644
>> --- a/xen/drivers/passthrough/arm/smmu.c
>> +++ b/xen/drivers/passthrough/arm/smmu.c
>> @@ -1598,21 +1598,6 @@ out_err:
>>  	return ret;
>>  }
>>  -static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg)
> 
> IIUC, the function needs to be moved because you need to use arm_smmu_write_s2cr(). If so, I would suggest to mention in the commit message because at first it seems unwarranted.

Ack. I will add that in commit msg.
> 
>> -{
>> -    struct arm_smmu_device *smmu = cfg->smmu;
>> -	int i, idx;
>> -	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
>> -
>> -	spin_lock(&smmu->stream_map_lock);
>> -	for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
>> -		if (arm_smmu_free_sme(smmu, idx))
>> -			arm_smmu_write_sme(smmu, idx);
>> -		cfg->smendx[i] = INVALID_SMENDX;
>> -	}
>> -	spin_unlock(&smmu->stream_map_lock);
>> -}
>> -
>>  static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>>  				      struct arm_smmu_master_cfg *cfg)
>>  {
>> @@ -1635,6 +1620,20 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>>  	return 0;
>>  }
>>  +static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
>> +				      struct arm_smmu_master_cfg *cfg)
>> +{
>> +	int i, idx;
> 
> NIT: I would suggest to take the opportunity to switch to "unsigned int" and ...

Ack. 
> 
>> +	struct arm_smmu_device *smmu = smmu_domain->smmu;
>> +	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
>> +	struct iommu_fwspec *fwspec = arm_smmu_get_fwspec(cfg);
> 
> ... use const here. "cfg" and "smmu" can't be consistent but "smmu_domain" technically could (thanks to how C works). That said, I quite dislike it as the code ends up to be confusing...

Ack. 
> 
>> +
>> +	for_each_cfg_sme(cfg, i, idx, fwspec->num_ids) {
>> +		s2cr[idx] = s2cr_init_val;
>> +		arm_smmu_write_s2cr(smmu, idx);
>> +	}
>> +}
>> +
>>  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>  {
>>  	int ret;
>> @@ -1684,10 +1683,11 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>    static void arm_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
>>  {
>> +	struct arm_smmu_domain *smmu_domain = domain->priv;
>>  	struct arm_smmu_master_cfg *cfg = find_smmu_master_cfg(dev);
>>    	if (cfg)
>> -		arm_smmu_master_free_smes(cfg);
>> +		return arm_smmu_domain_remove_master(smmu_domain, cfg);
> 
> Why are you using adding a 'return' here?

Not required. I will remove “return”.

Regards,
Rahul

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

is_memory_hole was implemented for x86 and not for ARM when introduced.
Replace is_memory_hole call to pci_check_bar as function should check
if device BAR is in defined memory range. Also, add an implementation
for ARM which is required for PCI passthrough.

On x86, pci_check_bar will call is_memory_hole which will check if BAR
is not overlapping with any memory region defined in the memory map.

On ARM, pci_check_bar will go through the host bridge ranges and check
if the BAR is in the range of defined ranges.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/arch/arm/include/asm/pci.h     | 12 ++++++++++
 xen/arch/arm/pci/pci-host-common.c | 35 ++++++++++++++++++++++++++++++
 xen/arch/x86/include/asm/pci.h     | 10 +++++++++
 xen/drivers/passthrough/pci.c      |  8 +++----
 4 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 7c7449d64f..5c4ab2c4dc 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -91,6 +91,16 @@ struct pci_ecam_ops {
     int (*init)(struct pci_config_window *);
 };
 
+/*
+ * struct to hold pci device bar.
+ */
+struct pdev_bar
+{
+    mfn_t start;
+    mfn_t end;
+    bool is_valid;
+};
+
 /* Default ECAM ops */
 extern const struct pci_ecam_ops pci_generic_ecam_ops;
 
@@ -125,6 +135,8 @@ int pci_host_iterate_bridges_and_count(struct domain *d,
 
 int pci_host_bridge_mappings(struct domain *d);
 
+bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
+
 #else   /*!CONFIG_HAS_PCI*/
 
 struct arch_pci_dev { };
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index fd8c0f837a..8ea1aaeece 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -363,6 +363,41 @@ int __init pci_host_bridge_mappings(struct domain *d)
     return 0;
 }
 
+static int is_bar_valid(const struct dt_device_node *dev,
+                        u64 addr, u64 len, void *data)
+{
+    struct pdev_bar *bar_data = data;
+    unsigned long s = mfn_x(bar_data->start);
+    unsigned long e = mfn_x(bar_data->end);
+
+    if ( (s < e) && (s >= PFN_DOWN(addr)) && (e<= PFN_DOWN(addr + len - 1)) )
+        bar_data->is_valid =  true;
+
+    return 0;
+}
+
+bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
+{
+    int ret;
+    struct dt_device_node *dt_node;
+    struct device *dev = (struct device *)pci_to_dev(pdev);
+    struct pdev_bar bar_data =  {
+        .start = start,
+        .end = end,
+        .is_valid = false
+    };
+
+    dt_node = pci_find_host_bridge_node(dev);
+
+    ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
+    if ( ret < 0 )
+        return ret;
+
+    if ( !bar_data.is_valid )
+        return false;
+
+    return true;
+}
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index c8e1a9ecdb..f4a58c8acf 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -57,4 +57,14 @@ static always_inline bool is_pci_passthrough_enabled(void)
 
 void arch_pci_init_pdev(struct pci_dev *pdev);
 
+static inline bool pci_check_bar(const struct pci_dev *pdev,
+                                 mfn_t start, mfn_t end)
+{
+    /*
+     * Check if BAR is not overlapping with any memory region defined
+     * in the memory map.
+     */
+    return is_memory_hole(start, end);
+}
+
 #endif /* __X86_PCI_H__ */
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 29356d59a7..52453a05fe 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -304,8 +304,8 @@ static void check_pdev(const struct pci_dev *pdev)
         if ( rc < 0 )
             /* Unable to size, better leave memory decoding disabled. */
             return;
-        if ( size && !is_memory_hole(maddr_to_mfn(addr),
-                                     maddr_to_mfn(addr + size - 1)) )
+        if ( size && !pci_check_bar(pdev, maddr_to_mfn(addr),
+                                    maddr_to_mfn(addr + size - 1)) )
         {
             /*
              * Return without enabling memory decoding if BAR position is not
@@ -331,8 +331,8 @@ static void check_pdev(const struct pci_dev *pdev)
 
         if ( rc < 0 )
             return;
-        if ( size && !is_memory_hole(maddr_to_mfn(addr),
-                                     maddr_to_mfn(addr + size - 1)) )
+        if ( size && !pci_check_bar(pdev, maddr_to_mfn(addr),
+                                    maddr_to_mfn(addr + size - 1)) )
         {
             printk(warn, &pdev->sbdf, "ROM ", PFN_DOWN(addr),
                    PFN_DOWN(addr + size - 1));
-- 
2.25.1

On 05.08.22 18:43, Rahul Singh wrote:


Hello Rahul


Thank you very much for that patch!


> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

I am not 100% sure regarding that. This is *completely* different patch 
from what Oleksandr initially made here:

https://lore.kernel.org/xen-devel/20220719174253.541965-2-olekstysh@gmail.com/

Copy below for the convenience:


+bool is_memory_hole(mfn_t start, mfn_t end)
+{
+    /* TODO: this needs to be properly implemented. */
+    return true;
+}




Patch looks good, just a couple of minor comments/nits.

>
> is_memory_hole was implemented for x86 and not for ARM when introduced.
> Replace is_memory_hole call to pci_check_bar as function should check
> if device BAR is in defined memory range. Also, add an implementation
> for ARM which is required for PCI passthrough.
>
> On x86, pci_check_bar will call is_memory_hole which will check if BAR
> is not overlapping with any memory region defined in the memory map.
>
> On ARM, pci_check_bar will go through the host bridge ranges and check
> if the BAR is in the range of defined ranges.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
>   xen/arch/arm/include/asm/pci.h     | 12 ++++++++++
>   xen/arch/arm/pci/pci-host-common.c | 35 ++++++++++++++++++++++++++++++
>   xen/arch/x86/include/asm/pci.h     | 10 +++++++++
>   xen/drivers/passthrough/pci.c      |  8 +++----
>   4 files changed, 61 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
> index 7c7449d64f..5c4ab2c4dc 100644
> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -91,6 +91,16 @@ struct pci_ecam_ops {
>       int (*init)(struct pci_config_window *);
>   };
>   
> +/*
> + * struct to hold pci device bar.
> + */
> +struct pdev_bar
> +{
> +    mfn_t start;
> +    mfn_t end;
> +    bool is_valid;
> +};


Nit: This is only used by pci-host-common.c, so I think it could be 
declared there.



> +
>   /* Default ECAM ops */
>   extern const struct pci_ecam_ops pci_generic_ecam_ops;
>   
> @@ -125,6 +135,8 @@ int pci_host_iterate_bridges_and_count(struct domain *d,
>   
>   int pci_host_bridge_mappings(struct domain *d);
>   
> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
> +
>   #else   /*!CONFIG_HAS_PCI*/
>   
>   struct arch_pci_dev { };
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index fd8c0f837a..8ea1aaeece 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -363,6 +363,41 @@ int __init pci_host_bridge_mappings(struct domain *d)
>       return 0;
>   }
>   
> +static int is_bar_valid(const struct dt_device_node *dev,
> +                        u64 addr, u64 len, void *data)
> +{
> +    struct pdev_bar *bar_data = data;
> +    unsigned long s = mfn_x(bar_data->start);
> +    unsigned long e = mfn_x(bar_data->end);
> +
> +    if ( (s < e) && (s >= PFN_DOWN(addr)) && (e<= PFN_DOWN(addr + len - 1)) )


Nit: white space after 'e' is missed in the last part of the check


> +        bar_data->is_valid =  true;
> +
> +    return 0;
> +}
> +
> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
> +{
> +    int ret;
> +    struct dt_device_node *dt_node;
> +    struct device *dev = (struct device *)pci_to_dev(pdev);


The cast is present here because of the const?

I would consider passing "const struct pci_dev *pdev" instead of "struct 
device *dev" to pci_find_host_bridge_node() and dropping conversion 
(pci<->dev) in both functions.


Something like below (not tested):

diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 5c4ab2c4dc..a17ef32252 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -116,7 +116,7 @@ bool pci_ecam_need_p2m_hwdom_mapping(struct domain *d,
                                       struct pci_host_bridge *bridge,
                                       uint64_t addr);
  struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t 
bus);
-struct dt_device_node *pci_find_host_bridge_node(struct device *dev);
+struct dt_device_node *pci_find_host_bridge_node(const struct pci_dev 
*pdev);
  int pci_get_host_bridge_segment(const struct dt_device_node *node,
                                  uint16_t *segment);

diff --git a/xen/arch/arm/pci/pci-host-common.c 
b/xen/arch/arm/pci/pci-host-common.c
index 8ea1aaeece..3a64a7350f 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -243,10 +243,9 @@ err_exit:
  /*
   * Get host bridge node given a device attached to it.
   */
-struct dt_device_node *pci_find_host_bridge_node(struct device *dev)
+struct dt_device_node *pci_find_host_bridge_node(const struct pci_dev 
*pdev)
  {
      struct pci_host_bridge *bridge;
-    struct pci_dev *pdev = dev_to_pci(dev);

      bridge = pci_find_host_bridge(pdev->seg, pdev->bus);
      if ( unlikely(!bridge) )
@@ -380,14 +379,13 @@ bool pci_check_bar(const struct pci_dev *pdev, 
mfn_t start, mfn_t end)
  {
      int ret;
      struct dt_device_node *dt_node;
-    struct device *dev = (struct device *)pci_to_dev(pdev);
      struct pdev_bar bar_data =  {
          .start = start,
          .end = end,
          .is_valid = false
      };

-    dt_node = pci_find_host_bridge_node(dev);
+    dt_node = pci_find_host_bridge_node(pdev);

      ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
      if ( ret < 0 )


> +    struct pdev_bar bar_data =  {
> +        .start = start,
> +        .end = end,
> +        .is_valid = false
> +    };
> +
> +    dt_node = pci_find_host_bridge_node(dev);

     if ( !dt_node )
         return false;


> +
> +    ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
> +    if ( ret < 0 )
> +        return ret;

s/return ret;/return false;


> +
> +    if ( !bar_data.is_valid )
> +        return false;
> +
> +    return true;
> +}
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
> index c8e1a9ecdb..f4a58c8acf 100644
> --- a/xen/arch/x86/include/asm/pci.h
> +++ b/xen/arch/x86/include/asm/pci.h
> @@ -57,4 +57,14 @@ static always_inline bool is_pci_passthrough_enabled(void)
>   
>   void arch_pci_init_pdev(struct pci_dev *pdev);
>   
> +static inline bool pci_check_bar(const struct pci_dev *pdev,
> +                                 mfn_t start, mfn_t end)
> +{
> +    /*
> +     * Check if BAR is not overlapping with any memory region defined
> +     * in the memory map.
> +     */
> +    return is_memory_hole(start, end);
> +}


Nit: I would use simple #define instead of static inline here

But I am not 100% sure that x86 maintainers would be happy.


> +
>   #endif /* __X86_PCI_H__ */
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 29356d59a7..52453a05fe 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -304,8 +304,8 @@ static void check_pdev(const struct pci_dev *pdev)
>           if ( rc < 0 )
>               /* Unable to size, better leave memory decoding disabled. */
>               return;
> -        if ( size && !is_memory_hole(maddr_to_mfn(addr),
> -                                     maddr_to_mfn(addr + size - 1)) )
> +        if ( size && !pci_check_bar(pdev, maddr_to_mfn(addr),
> +                                    maddr_to_mfn(addr + size - 1)) )
>           {
>               /*
>                * Return without enabling memory decoding if BAR position is not
> @@ -331,8 +331,8 @@ static void check_pdev(const struct pci_dev *pdev)
>   
>           if ( rc < 0 )
>               return;
> -        if ( size && !is_memory_hole(maddr_to_mfn(addr),
> -                                     maddr_to_mfn(addr + size - 1)) )
> +        if ( size && !pci_check_bar(pdev, maddr_to_mfn(addr),
> +                                    maddr_to_mfn(addr + size - 1)) )
>           {
>               printk(warn, &pdev->sbdf, "ROM ", PFN_DOWN(addr),
>                      PFN_DOWN(addr + size - 1));

-- 
Regards,

Oleksandr Tyshchenko

Hi Oleksandr,


> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@gmail.com> wrote:
> 
> 
> On 05.08.22 18:43, Rahul Singh wrote:
> 
> 
> Hello Rahul
> 
> 
> Thank you very much for that patch!
> 
> 
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> I am not 100% sure regarding that. This is *completely* different patch from what Oleksandr initially made here:
> 
> https://lore.kernel.org/xen-devel/20220719174253.541965-2-olekstysh@gmail.com/
> 
> Copy below for the convenience:
> 
> 
> +bool is_memory_hole(mfn_t start, mfn_t end)
> +{
> +    /* TODO: this needs to be properly implemented. */
> +    return true;
> +}
> 
> 
> 
> 
> Patch looks good, just a couple of minor comments/nits.

Ok. I will remove “From: … “ in next version.
> 
>> 
>> is_memory_hole was implemented for x86 and not for ARM when introduced.
>> Replace is_memory_hole call to pci_check_bar as function should check
>> if device BAR is in defined memory range. Also, add an implementation
>> for ARM which is required for PCI passthrough.
>> 
>> On x86, pci_check_bar will call is_memory_hole which will check if BAR
>> is not overlapping with any memory region defined in the memory map.
>> 
>> On ARM, pci_check_bar will go through the host bridge ranges and check
>> if the BAR is in the range of defined ranges.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>>  xen/arch/arm/include/asm/pci.h     | 12 ++++++++++
>>  xen/arch/arm/pci/pci-host-common.c | 35 ++++++++++++++++++++++++++++++
>>  xen/arch/x86/include/asm/pci.h     | 10 +++++++++
>>  xen/drivers/passthrough/pci.c      |  8 +++----
>>  4 files changed, 61 insertions(+), 4 deletions(-)
>> 
>> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
>> index 7c7449d64f..5c4ab2c4dc 100644
>> --- a/xen/arch/arm/include/asm/pci.h
>> +++ b/xen/arch/arm/include/asm/pci.h
>> @@ -91,6 +91,16 @@ struct pci_ecam_ops {
>>      int (*init)(struct pci_config_window *);
>>  };
>>  +/*
>> + * struct to hold pci device bar.
>> + */
>> +struct pdev_bar
>> +{
>> +    mfn_t start;
>> +    mfn_t end;
>> +    bool is_valid;
>> +};
> 
> 
> Nit: This is only used by pci-host-common.c, so I think it could be declared there.

Ack.
> 
> 
> 
>> +
>>  /* Default ECAM ops */
>>  extern const struct pci_ecam_ops pci_generic_ecam_ops;
>>  @@ -125,6 +135,8 @@ int pci_host_iterate_bridges_and_count(struct domain *d,
>>    int pci_host_bridge_mappings(struct domain *d);
>>  +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
>> +
>>  #else   /*!CONFIG_HAS_PCI*/
>>    struct arch_pci_dev { };
>> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
>> index fd8c0f837a..8ea1aaeece 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -363,6 +363,41 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>      return 0;
>>  }
>>  +static int is_bar_valid(const struct dt_device_node *dev,
>> +                        u64 addr, u64 len, void *data)
>> +{
>> +    struct pdev_bar *bar_data = data;
>> +    unsigned long s = mfn_x(bar_data->start);
>> +    unsigned long e = mfn_x(bar_data->end);
>> +
>> +    if ( (s < e) && (s >= PFN_DOWN(addr)) && (e<= PFN_DOWN(addr + len - 1)) )
> 
> 
> Nit: white space after 'e' is missed in the last part of the check

Ack.

> 
> 
>> +        bar_data->is_valid =  true;
>> +
>> +    return 0;
>> +}
>> +
>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>> +{
>> +    int ret;
>> +    struct dt_device_node *dt_node;
>> +    struct device *dev = (struct device *)pci_to_dev(pdev);
> 
> 
> The cast is present here because of the const?

Yes you are right, cast is because of the const.
> 
> I would consider passing "const struct pci_dev *pdev" instead of "struct device *dev" to pci_find_host_bridge_node() and dropping conversion (pci<->dev) in both functions.

Yes make sense. I will do that in next version.
> 
> 
> Something like below (not tested):
> 
> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
> index 5c4ab2c4dc..a17ef32252 100644
> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -116,7 +116,7 @@ bool pci_ecam_need_p2m_hwdom_mapping(struct domain *d,
>                                       struct pci_host_bridge *bridge,
>                                       uint64_t addr);
>  struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus);
> -struct dt_device_node *pci_find_host_bridge_node(struct device *dev);
> +struct dt_device_node *pci_find_host_bridge_node(const struct pci_dev *pdev);
>  int pci_get_host_bridge_segment(const struct dt_device_node *node,
>                                  uint16_t *segment);
> 
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index 8ea1aaeece..3a64a7350f 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -243,10 +243,9 @@ err_exit:
>  /*
>   * Get host bridge node given a device attached to it.
>   */
> -struct dt_device_node *pci_find_host_bridge_node(struct device *dev)
> +struct dt_device_node *pci_find_host_bridge_node(const struct pci_dev *pdev)
>  {
>      struct pci_host_bridge *bridge;
> -    struct pci_dev *pdev = dev_to_pci(dev);
> 
>      bridge = pci_find_host_bridge(pdev->seg, pdev->bus);
>      if ( unlikely(!bridge) )
> @@ -380,14 +379,13 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>  {
>      int ret;
>      struct dt_device_node *dt_node;
> -    struct device *dev = (struct device *)pci_to_dev(pdev);
>      struct pdev_bar bar_data =  {
>          .start = start,
>          .end = end,
>          .is_valid = false
>      };
> 
> -    dt_node = pci_find_host_bridge_node(dev);
> +    dt_node = pci_find_host_bridge_node(pdev);
> 
>      ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
>      if ( ret < 0 )
> 
> 
>> +    struct pdev_bar bar_data =  {
>> +        .start = start,
>> +        .end = end,
>> +        .is_valid = false
>> +    };
>> +
>> +    dt_node = pci_find_host_bridge_node(dev);
> 
>     if ( !dt_node )
>         return false;

Ack. 
> 
> 
>> +
>> +    ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
>> +    if ( ret < 0 )
>> +        return ret;
> 
> s/return ret;/return false;

Ack. 
> 
> 
>> +
>> +    if ( !bar_data.is_valid )
>> +        return false;
>> +
>> +    return true;
>> +}
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
>> index c8e1a9ecdb..f4a58c8acf 100644
>> --- a/xen/arch/x86/include/asm/pci.h
>> +++ b/xen/arch/x86/include/asm/pci.h
>> @@ -57,4 +57,14 @@ static always_inline bool is_pci_passthrough_enabled(void)
>>    void arch_pci_init_pdev(struct pci_dev *pdev);
>>  +static inline bool pci_check_bar(const struct pci_dev *pdev,
>> +                                 mfn_t start, mfn_t end)
>> +{
>> +    /*
>> +     * Check if BAR is not overlapping with any memory region defined
>> +     * in the memory map.
>> +     */
>> +    return is_memory_hole(start, end);
>> +}
> 
> 
> Nit: I would use simple #define instead of static inline here
> 
> But I am not 100% sure that x86 maintainers would be happy.
> 

Jan replied to this and I will check what is suggested by Jan.

Regards,
Rahul

Hi Rahul,

This patch seems to have been sent in-reply-to the SMMUv1 patch. Was it 
intended?

On 09/08/2022 16:22, Rahul Singh wrote:
>> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@gmail.com> wrote:
>>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>>> +{
>>> +    int ret;
>>> +    struct dt_device_node *dt_node;
>>> +    struct device *dev = (struct device *)pci_to_dev(pdev);
>>
>>
>> The cast is present here because of the const?
> 
> Yes you are right, cast is because of the const.
>>
>> I would consider passing "const struct pci_dev *pdev" instead of "struct device *dev" to pci_find_host_bridge_node() and dropping conversion (pci<->dev) in both functions.

It looks like this function was added without any callers. The commit 
message claim there will be some. Can you (or Oleksandr) confirm this is 
not going to be problem for future patches?

That said, I agree that the conversion pci -> dev -> pci is pointless. 
So I would say if there are use case where we only have a 'dev' in hand, 
then we could ask the caller to do the conversation or we provide an 
helper if there are too many cases.

> 
> Yes make sense. I will do that in next version.

While you are modifying the prototype for pci_find_host_bridge_node() 
can you consider to also constify the return (it should not be modified)?

In any case, the change suggested by Oleksandr should preferably be 
separate to this patch and added before.

Cheers,

-- 
Julien Grall

Hi Julien,

> On 9 Aug 2022, at 4:46 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> This patch seems to have been sent in-reply-to the SMMUv1 patch. Was it intended?

That was by mistake I want to send all the patches independently but somehow I send it 
from single git "send-email” command because of that I think this patch comes in-reply-to 
SMMUv1 patch.

> 
> On 09/08/2022 16:22, Rahul Singh wrote:
>>> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@gmail.com> wrote:
>>>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>>>> +{
>>>> +    int ret;
>>>> +    struct dt_device_node *dt_node;
>>>> +    struct device *dev = (struct device *)pci_to_dev(pdev);
>>> 
>>> 
>>> The cast is present here because of the const?
>> Yes you are right, cast is because of the const.
>>> 
>>> I would consider passing "const struct pci_dev *pdev" instead of "struct device *dev" to pci_find_host_bridge_node() and dropping conversion (pci<->dev) in both functions.
> 
> It looks like this function was added without any callers. The commit message claim there will be some. Can you (or Oleksandr) confirm this is not going to be problem for future patches?

I checked the whole PCI passthrough feature branch this function will be used when
we add iommu support for PCI device.  

> 
> That said, I agree that the conversion pci -> dev -> pci is pointless. So I would say if there are use case where we only have a 'dev' in hand, then we could ask the caller to do the conversation or we provide an helper if there are too many cases.
> 
>> Yes make sense. I will do that in next version.
> 
> While you are modifying the prototype for pci_find_host_bridge_node() can you consider to also constify the return (it should not be modified)?

Agree, I will constify the retrun also. 

> 
> In any case, the change suggested by Oleksandr should preferably be separate to this patch and added before.

Ack. 

Regards,
Rahul

On Tue, Aug 9, 2022 at 7:26 PM Rahul Singh <Rahul.Singh@arm.com> wrote:

> Hi Julien,
>

Hello Julien, Rahul

[sorry for possible format issues]


[snip]


>
>
> >
> > On 09/08/2022 16:22, Rahul Singh wrote:
> >>> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@gmail.com> wrote:
> >>>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t
> end)
> >>>> +{
> >>>> +    int ret;
> >>>> +    struct dt_device_node *dt_node;
> >>>> +    struct device *dev = (struct device *)pci_to_dev(pdev);
> >>>
> >>>
> >>> The cast is present here because of the const?
> >> Yes you are right, cast is because of the const.
> >>>
> >>> I would consider passing "const struct pci_dev *pdev" instead of
> "struct device *dev" to pci_find_host_bridge_node() and dropping conversion
> (pci<->dev) in both functions.
> >
> > It looks like this function was added without any callers. The commit
> message claim there will be some. Can you (or Oleksandr) confirm this is
> not going to be problem for future patches?
>
> I checked the whole PCI passthrough feature branch this function will be
> used when
> we add iommu support for PCI device.



Can confirm that, it will be called by the iommu code, as I understand
there won't be an issue, the more, the exact place where the
pci_find_host_bridge_node() will be called will have "pdev" in hand.


>
>
> >
> > That said, I agree that the conversion pci -> dev -> pci is pointless.
> So I would say if there are use case where we only have a 'dev' in hand,
> then we could ask the caller to do the conversation or we provide an helper
> if there are too many cases.
> >
> >> Yes make sense. I will do that in next version.
> >
> > While you are modifying the prototype for pci_find_host_bridge_node()
> can you consider to also constify the return (it should not be modified)?
>
> Agree, I will constify the retrun also.
>
> >
> > In any case, the change suggested by Oleksandr should preferably be
> separate to this patch and added before.
>
> Ack.
>
> Regards,
> Rahul
>
>

-- 
Regards,

Oleksandr Tyshchenko

On 08.08.2022 17:30, Oleksandr wrote:
> On 05.08.22 18:43, Rahul Singh wrote:
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -363,6 +363,41 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>       return 0;
>>   }
>>   
>> +static int is_bar_valid(const struct dt_device_node *dev,
>> +                        u64 addr, u64 len, void *data)

Nit: No new uses of u64 please use uint64_t instead.

>> --- a/xen/arch/x86/include/asm/pci.h
>> +++ b/xen/arch/x86/include/asm/pci.h
>> @@ -57,4 +57,14 @@ static always_inline bool is_pci_passthrough_enabled(void)
>>   
>>   void arch_pci_init_pdev(struct pci_dev *pdev);
>>   
>> +static inline bool pci_check_bar(const struct pci_dev *pdev,
>> +                                 mfn_t start, mfn_t end)
>> +{
>> +    /*
>> +     * Check if BAR is not overlapping with any memory region defined
>> +     * in the memory map.
>> +     */
>> +    return is_memory_hole(start, end);
>> +}
> 
> 
> Nit: I would use simple #define instead of static inline here
> 
> But I am not 100% sure that x86 maintainers would be happy.

Quite the other way around - when possible we prefer inline functions.
And note that the two functions are strictly aliases of one another
(in which case a simplified

#define pci_check_bar is_memory_hole

might indeed have been worth a consideration, as there's no type
safety to be lost in such cases).

Jan