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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.09.2022 10:58, Julien Grall wrote:
> Hi Jan & Stefano,
> 
> On 06/09/2022 09:53, Jan Beulich wrote:
>> 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.
> 
> Hmmm.... Is that requirement written down somewhere?

What do you mean here? Isn't it quite obvious that every byte in the
address space may only be used for a single purpose? I.e. if a byte
is covered by a BAR, it cannot also be covered by a RAM region or
yet something else (e.g. MMIO beyond BARs of PCI devices). What
happens if BAR and RAM indeed overlap depends on fabric and chipset,
but it'll either result in chaos if two parties respond to a single
request on the bus, or it'll be (hopefully) deterministic (for any
individual system) which of the two takes "precedence".

I think we've had a similar discussion a little while ago already in
the context of vPCI with guest address space in mind. The same (imo
obvious) "rule" spelled out above applies there and here.

Jan

> The reason I am 
> asking is "page-aligned" will vary depending on the software. In the 
> past we had a couple of cases where the region would be 4KB-aligned but 
> not necessarily 64KB-aligned.
> 
> If the answer is no to my question then...
> 
>> 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.
> 
> ... I think we would want to drop PFN_DOWN/PFN_UP here.
> 
> Cheers,
>