Re: [PULL 7/7] vfio: fix mapping of MSIX bar

Alex Williamson
2014-01-19T16:59:11Z

On Sun, 2014-01-19 at 23:46 +0800, Kai Huang wrote:
> On Sun, Jan 19, 2014 at 10:11 PM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Sun, 2014-01-19 at 22:03 +0800, Kai Huang wrote:
> >> On Sat, Jan 18, 2014 at 3:25 AM, Alex Williamson
> >> <alex.williamson@redhat.com> wrote:
> >> > From: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> >
> >> > VFIO virtualizes MSIX table for the guest but not mapping the part of
> >> > a BAR which contains an MSIX table. Since vfio_mmap_bar() mmaps chunks
> >> > before and after the MSIX table, they have to be aligned to the host
> >> > page size which may be TARGET_PAGE_MASK (4K) or 64K in case of PPC64.
> >> >
> >> > This fixes boundaries calculations to use the real host page size.
> >> >
> >> > Without the patch, the chunk before MSIX table may overlap with the MSIX
> >> > table and mmap will fail in the host kernel. The result will be serious
> >> > slowdown as the whole BAR will be emulated by QEMU.
> >> >
> >> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >> > ---
> >> > hw/misc/vfio.c | 6 +++---
> >> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> >> > index 432547c..8a1f1a1 100644
> >> > --- a/hw/misc/vfio.c
> >> > +++ b/hw/misc/vfio.c
> >> > @@ -2544,7 +2544,7 @@ static void vfio_map_bar(VFIODevice *vdev, int nr)
> >> > * potentially insert a direct-mapped subregion before and after it.
> >> > */
> >> > if (vdev->msix && vdev->msix->table_bar == nr) {
> >> > - size = vdev->msix->table_offset & TARGET_PAGE_MASK;
> >> > + size = vdev->msix->table_offset & qemu_host_page_mask;
> >> > }
> >> >
> >> > strncat(name, " mmap", sizeof(name) - strlen(name) - 1);
> >> > @@ -2556,8 +2556,8 @@ static void vfio_map_bar(VFIODevice *vdev, int nr)
> >> > if (vdev->msix && vdev->msix->table_bar == nr) {
> >> > unsigned start;
> >> >
> >> > - start = TARGET_PAGE_ALIGN(vdev->msix->table_offset +
> >> > - (vdev->msix->entries * PCI_MSIX_ENTRY_SIZE));
> >> > + start = HOST_PAGE_ALIGN(vdev->msix->table_offset +
> >> > + (vdev->msix->entries * PCI_MSIX_ENTRY_SIZE));
> >> >
> >> Hi Alex,
> >>
> >> I am new to vfio and qemu, and have some questions. Does MSIX have one
> >> dedicated bar when qemu emulating the device? Looks your code maps
> >> both the content before and after the MSIX table? If MSIX has
> >> dedicated bar, I think we can just skip the MSIX bar, why do we need
> >> to map the context before and after the MSIX table?
> >
> > vfio is used to pass through existing physical devices. We don't get to
> > define the MSI-X layout of those devices. Therefore we must be prepared
> > to handle any possible layout. The BAR may be dedicated to the MSI-X
> > table or it may also include memory mapped register space for the
> > device. Thanks,
> >
> > Alex
> >
>
> This sounds reasonable. So if there's bar contains both MSIX table and
> register, the register access will be trapped and emulated?

We attempt to directly map any portions of the BAR that are sufficiently
aligned, that's exactly what this patch is fixing. For target page size
equal to host page size it already works. Anything we can't directly
map is trapped in qemu and emulated if it's part of the MSI-X table or
passed through if it's outside the table.

> Btw, did vfio_mmap_bar consider the pedding bit array table? I don't
> think they can be accessed directly by userspace either.

The PBA is also emulated, see msix_init(). MemoryRegions are created
both for the table and the PBA. Unlike the MSI-X table, the PBA is
read-only, so we don't need to go to the same lengths to protect it as
we do the physical MSI-X table. Thanks,

Alex