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

[Peter Xu <peterx@redhat.com>]

On Wed, Oct 12, 2016 at 04:37:54PM +0200, Alexander Gordeev wrote:
> On Fri, Sep 23, 2016 at 03:14:04PM +0800, Peter Xu wrote:
> > > -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
> > > +phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
> > >  {
> > >  	uint32_t bar = pci_bar_get(dev, bar_num);
> > > +	uint32_t mask = pci_bar_mask(bar);
> > > +	uint64_t addr = bar & mask;
> > >  
> > > -	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
> > > -		return bar & PCI_BASE_ADDRESS_IO_MASK;
> > > -	else
> > > -		return bar & PCI_BASE_ADDRESS_MEM_MASK;
> > > +	if (pci_bar_is64(dev, bar_num))
> > > +		addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
> > > +
> > > +	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?

Another silly question: I see pci_translate_addr() defined in both
lib/x86/asm/pci.h and lib/asm-generic/pci-host-bridge.h. How's that
working?

> 
> > [...]
> > 
> > > +static inline
> > > +phys_addr_t pci_translate_addr(pcidevaddr_t dev __unused, uint64_t addr)
> > > +{
> > > +    return addr;
> > > +}
> > > +
> > 
> > We are not using dev here (and in following patches), I don't know
> > ARM, but at least x86 will need this to translate IOVA into PA (in
> > other words, each device can have its own IO address space). If this
> > codes are for common, shall we consider that as well?
> 
> Could this function be extended or the prototype blocks the proper
> implementation? (I guess, I will get the answer once I look into your
> works).

Yes, it can be extended.

Thanks,

-- peterx