Re: [PATCH v3 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.09.2022 02:24, Stefano Stabellini wrote:
> On Thu, 1 Sep 2022, Rahul Singh wrote:
>> @@ -363,6 +373,39 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>      return 0;
>>  }
>>  
>> +static int is_bar_valid(const struct dt_device_node *dev,
>> +                        uint64_t addr, uint64_t 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_UP(addr + len - 1)) 
>> )
>> +        bar_data->is_valid =  true;
> 
> 
> This patch looks good and you addressed all Jan's comment well. Before I
> ack it, one question.
> 
> I know that you made this change to address Jan's comment but using
> PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <=
> PFN_UP(addr + len - 1)) check means that we are relaxing the
> requirements, aren't we?
> 
> I know that this discussion is a bit pointless because addr and len should
> always be page aligned, and if they weren't it would be a mistake. But
> assuming that they are not page aligned, wouldn't we want this check to
> be a strict as possible?
> 
> Wouldn't we want to ensure that the [s,e] range is a strict subset of
> [addr,addr+len-1] ? If so we would need to do the following instead:
> 
>     if ( (s <= e) && (s >= PFN_UP(addr)) && (e <= PFN_DOWN(addr + len - 1)) )
>         bar_data->is_valid =  true;

But that might mean (in theory at least) a partial overlap, which has
to be avoided. The only alternative that I see to Rahul's original
code is to omit use of PFN_DOWN() and PFN_UP() in this construct
altogether. Assuming that's correct for the passed in (addr,len)
tuple.

Jan