Re: [kvm-unit-tests PATCH v7 06/13] pci: Rework pci_bar_addr()

[Peter Xu <peterx@redhat.com>]

On Fri, Oct 14, 2016 at 08:55:20AM +0200, Andrew Jones wrote:
> Hi Peter,
> 
> On Fri, Oct 14, 2016 at 02:23:55PM +0800, Peter Xu wrote:
> > On Thu, Oct 13, 2016 at 04:16:03PM +0200, Alexander Gordeev wrote:
> > > On Thu, Oct 13, 2016 at 02:40:35PM +0800, Peter Xu wrote:
> > > > > > > +	return pci_translate_addr(dev, addr);
> > > > > > 
> > > > > > Raw question: do we need to translate bar addresses as well?
> > > > > 
> > > > > I believe, yes.
> > > > > 
> > > > > Unless we always have identity mapping between PCI address space and
> > > > > CPU physical address space I can not realize how could it be done
> > > > > otherwise. But even if we were, I would leave the translation routine
> > > > > for clarity.
> > > > 
> > > > Sorry I didn't quite catch your point. Are we talking about IOMMU
> > > > address remapping here? IMHO BAR addresses are from CPU's point of
> > > > view. It's only used by CPU, not device. In that case, BAR address
> > > > should not be translated at least by IOMMU (no matter for x86/arm or
> > > > whatever).
> > > > 
> > > > Take Linux as example: pci_ioremap_bar() is responsible to be called
> > > > for any PCI drivers to map device memory bars into kernel virtual
> > > > address space. Basically it does:
> > > > 
> > > > void __iomem *pci_ioremap_bar(struct pci_dev *pdev, int bar)
> > > > {
> > > > 	struct resource *res = &pdev->resource[bar];
> > > > 	return ioremap_nocache(res->start, resource_size(res));
> > > > }
> > > > 
> > > > So as it is written: I believe we don't translate the bar address
> > > > (which should be res->start). We use it as physical address.
> > > > 
> > > > Or, do you mean other kinds of translation that I don't aware of?
> > > 
> > > Yes, I mean translation from PCI bus address space to CPU physical
> > > address space. These two busses are different and hence need a
> > > translation. I assume Linux pci_dev::resource[] have translated
> > > address, but it is not what PCI devices see. Unless I do not terribly
> > > missing somethig, BAR addresses is what a device sees on its AD[0..31]
> > > pins.
> > 
> > I believe pci_dev::resource[] should be assigned by BIOS or something
> > before Linux. At that time, IOMMU is possibly even not inited. So no
> > chance for a translation at all.
> 
> kvm-unit-tests != Linux
> 
> kvm-unit-tests/arm doesn't have a bootloader at all (not counting QEMU
> initializing registers, and a handful of QEMU generated instructions
> that gives us a kick)
> 
> Anyway, I'm glad you're reviewing this series (my PCI skills are
> minimal), but you'll have to review it in the right context. In
> this case, more of a seabios context.

Thank you for pointing out. :-)

I know little about PCI as well, just want to know the fact on how we
should treat BAR addresses. So I'd say my comments are more like
questions rather than I disagree with the changes. E.g., even if we
don't have BIOS, do we really need to translate BAR addresses on ARM?

Sorry if I brought too much noise to this thread. Looking forward to
Alex's further works.

Thanks!

-- peterx