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

[Julien Grall <julien@xxxxxxx>]

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