[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.