Re: [Qemu-devel] [PATCH qemu v6 07/15] vfio: spapr: Add SPAPR IOMMU v2 support (DMA memory preregistering)

[Alexey Kardashevskiy]

On 04/16/2015 08:07 PM, Thomas Huth wrote:
> Am Sat, 11 Apr 2015 01:24:36 +1000
> schrieb Alexey Kardashevskiy <address@hidden>:
>
> > This makes use of the new "memory registering" feature. The idea is
> > to provide the userspace ability to notify the host kernel about pages
> > which are going to be used for DMA. Having this information, the host
> > kernel can pin them all once per user process, do locked pages
> > accounting (once) and not spent time on doing that in real time with
> > possible failures which cannot be handled nicely in some cases.
> >
> > This adds a guest RAM memory listener which notifies a VFIO container
> > about memory which needs to be pinned/unpinned. VFIO MMIO regions
> > (i.e. "skip dump" regions) are skipped.
> >
> > The feature is only enabled for SPAPR IOMMU v2. The host kernel changes
> > are required. Since v2 does not need/support VFIO_IOMMU_ENABLE, this does
> > not call it when v2 is detected and enabled.
> >
> > This does not change the guest visible interface.
> >
> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
> > ---
> > Changes:
> > v6:
> > * fixed commit log (s/guest/userspace/), added note about no guest visible
> > change
> > * fixed error checking if ram registration failed
> > * added alignment check for section->offset_within_region
> >
> > v5:
> > * simplified the patch
> > * added trace points
> > * added round_up() for the size
> > * SPAPR IOMMU v2 used
> > ---
> >   hw/vfio/common.c              | 26 +++++++++----
> >   hw/vfio/spapr.c               | 88 ++++++++++++++++++++++++++++++++++++++++++-
> >   include/hw/vfio/vfio-common.h |  5 ++-
> >   trace-events                  |  1 +
> >   4 files changed, 109 insertions(+), 11 deletions(-)
> [...]
> > diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> > index 5f79194..31353f1 100644
> > --- a/hw/vfio/spapr.c
> > +++ b/hw/vfio/spapr.c
> > @@ -17,6 +17,9 @@
> >    *  along with this program; if not, see <http://www.gnu.org/licenses/>.
> >    */
> >
> > +#include <sys/ioctl.h>
> > +#include <linux/vfio.h>
> > +
> >   #include "hw/vfio/vfio-common.h"
> >   #include "qemu/error-report.h"
> >   #include "trace.h"
> > @@ -211,16 +214,97 @@ static const MemoryListener vfio_spapr_memory_listener = {
> >       .region_del = vfio_spapr_listener_region_del,
> >   };
> >
> > +static void vfio_ram_do_region(VFIOContainer *container,
> > +                              MemoryRegionSection *section, unsigned long req)
> > +{
> > +    int ret;
> > +    struct vfio_iommu_spapr_register_memory reg = { .argsz = sizeof(reg) };
> > +
> > +    if (!memory_region_is_ram(section->mr) ||
> > +        memory_region_is_skip_dump(section->mr)) {
> > +        return;
> > +    }
> > +
> > +    if (unlikely((section->offset_within_region & (getpagesize() - 1)))) {
> > +        error_report("%s received unaligned region", __func__);
> > +        return;
> > +    }
> > +
> > +    reg.vaddr = (__u64) memory_region_get_ram_ptr(section->mr) +
> > +        section->offset_within_region;
> > +    reg.size = ROUND_UP(int128_get64(section->size), TARGET_PAGE_SIZE);
> > +
> > +    ret = ioctl(container->fd, req, &reg);
> > +    trace_vfio_ram_register(_IOC_NR(req) - VFIO_BASE, reg.vaddr, reg.size,
> > +            ret ? -errno : 0);
> > +    if (!ret) {
>
> Since this function does not return an error code (maybe it should?),


It is called from listener callbacks which do not return codes.


> would it
> make sense to print out at least an error message here that something went 
> wrong?
> (most people won't have the tracing enabled, so the error might go unnoticed 
> otherwise)


!ret means ret==0 which means "good". The error case handling is few lines 
below and it won't go unnoticed.



> > +        return;
> > +    }
> > +
> > +    /*
> > +     * On the initfn path, store the first error in the container so we
> > +     * can gracefully fail.  Runtime, there's not much we can do other
> > +     * than throw a hardware error.
> > +     */
> > +    if (!container->iommu_data.spapr.ram_reg_initialized) {
> > +        if (!container->iommu_data.spapr.ram_reg_error) {
> > +            container->iommu_data.spapr.ram_reg_error = -errno;
> > +        }
> > +    } else {
> > +        hw_error("vfio: RAM registering failed, unable to continue");
> > +    }
> > +}
>
>   Thomas


--
Alexey