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

[Julien Grall <julien@xxxxxxx>]

On 07/09/2022 13:01, Jan Beulich wrote:
> On 07.09.2022 12:00, Julien Grall wrote:
> > On 07/09/2022 10:07, Jan Beulich wrote:
> > > On 07.09.2022 10:58, Julien Grall wrote:
> > > > 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 am well aware about that and I am not sure how you implied this is
> > what I was referring to from what I wrote (in particular if you read the
> > next sentence).
> >
> > Stefano wrote that it would be a mistake if the address/length is not
> > page-aligned. However, I am not aware from such requirement written
> > down. It seems to be more an expected common sense that was IIRC not
> > always respected on HW supporting multiple page-granularity.
> I guess the question was then solely directed at Stefano? 
This question yes. The rest was a reply to your suggestion. I will wait 
for Stefano to answer.
Cheers,

--
Julien Grall