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? 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,
--
Julien Grall
