Re: [PATCH 3/3] VFIO V4: VFIO driver: Non-privileged user level PCI drivers

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 2010-09-27 at 13:54 +0200, Michael S. Tsirkin wrote:
> On Wed, Sep 22, 2010 at 02:18:24PM -0700, Tom Lyon wrote:
> > 
> > Signed-off-by: Tom Lyon <pugs@cisco.com>
> 
> Some comments on the pci bits:
> 
> After going over them for the Nth time - something needs to be done
> with the rvirt/write tables. I doubt anyone besides me and you
> has gone over them: 

/me bites tongue...

> > +static void vfio_bar_fixup(struct vfio_dev *vdev)
> > +{
> 
> So you do this on each read?
> Why don't you mask the appropriate bits on write?
> This is what real hardware does, after all.
> Then you won't need the bardirty field.
> 
> 
> > +	struct pci_dev *pdev = vdev->pdev;
> > +	int bar;
> > +	u32 *lp;
> > +	u64 mask;
> > +
> > +	for (bar = 0; bar <= 5; bar++) {
> > +		if (pci_resource_start(pdev, bar))
> > +			mask = ~(pci_resource_len(pdev, bar) - 1);
> > +		else
> > +			mask = 0;
> > +		lp = (u32 *)vdev->vconfig + PCI_BASE_ADDRESS_0 + 4*bar;
> > +		*lp &= (u32)mask;
> > +
> > +		if (pci_resource_flags(pdev, bar) & IORESOURCE_IO)
> > +			*lp |= PCI_BASE_ADDRESS_SPACE_IO;
> > +		else if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) {
> > +			*lp |= PCI_BASE_ADDRESS_SPACE_MEMORY;
> > +			if (pci_resource_flags(pdev, bar) & IORESOURCE_PREFETCH)
> > +				*lp |= PCI_BASE_ADDRESS_MEM_PREFETCH;
> > +			if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM_64) {
> > +				*lp |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> > +				lp++;
> > +				*lp &= (u32)(mask >> 32);
> > +				bar++;
> > +			}
> > +		}
> > +	}
> > +
> > +	if (pci_resource_start(pdev, PCI_ROM_RESOURCE))
> 
> Not if (pci_resource_len)?

Using this test covers both zero length/unimplemented BARs and BARs that
aren't mapped by the host.  Unmapped BARs on the host are exposed as
unimplemented BARs to the vfio user.  I wonder if this is still
necessary if we do the iomap/rom_map on open as I propose.

> > +		mask = ~(pci_resource_len(pdev, PCI_ROM_RESOURCE) - 1);
> > +	else
> > +		mask = 0;
> > +	lp = (u32 *)vdev->vconfig + PCI_ROM_ADDRESS;
> > +	*lp &= (u32)mask;
> > +
> > +	vdev->bardirty = 0;
> 
> Aren't the pci values in little endian format?
> If so doing operations on them in native endianness is wrong.
> sparse generally is good at catching these, but you will
> have to avoid so many type casts and annotate endian-ness
> for it to be of any use.

Yep, I expect there are a number of endian issues here, good call
pointing them out.

Alex