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

[Thomas Huth <thuth@redhat.com>]

Am Sat, 11 Apr 2015 01:24:36 +1000
schrieb Alexey Kardashevskiy <aik@ozlabs.ru>:

> 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 <aik@ozlabs.ru>
> ---
> 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?), 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)

> +        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