Messages in this thread | | | Subject | Re: [PATCH 3/3] VFIO V4: VFIO driver: Non-privileged user level PCI drivers | From | Alex Williamson <> | Date | Mon, 27 Sep 2010 14:57:49 -0600 |
| |
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
| |