lkml.org 
[lkml]   [2010]   [Sep]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 3/3] VFIO V4: VFIO driver: Non-privileged user level PCI drivers
    From
    Date
    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



    \
     
     \ /
      Last update: 2010-09-27 23:01    [W:2.268 / U:0.148 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site