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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1285621069.4951.61.camel@x201>
Date:	Mon, 27 Sep 2010 14:57:49 -0600
From:	Alex Williamson <alex.williamson@...hat.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
Cc:	Tom Lyon <pugs@...co.com>, linux-pci@...r.kernel.org,
	jbarnes@...tuousgeek.org, linux-kernel@...r.kernel.org,
	kvm@...r.kernel.org, randy.dunlap@...cle.com, arnd@...db.de,
	joro@...tes.org, hjk@...utronix.de, avi@...hat.com, gregkh@...e.de,
	chrisw@...s-sol.org
Subject: Re: [PATCH 3/3] VFIO V4: VFIO driver: Non-privileged user level
 PCI drivers

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@...co.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/