Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1754011AbeAOLWv convert rfc822-to-8bit (ORCPT
        <rfc822;ralf@linux-mips.org> + 1 other);
        Mon, 15 Jan 2018 06:22:51 -0500
Received: from mail-qt0-f194.google.com ([209.85.216.194]:35200 "EHLO
        mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751636AbeAOLWt (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 15 Jan 2018 06:22:49 -0500
X-Google-Smtp-Source: ACJfBovbsifKJRJpl+QPS6IafGCckX/GLvw9tL6WBZoUpQ5KMgskaj/Sf/zvNyQSII/HjGbMZ+cJ5g6nqRJ2a6mbtKo=
MIME-Version: 1.0
In-Reply-To: <20180115085528.32663-1-peterx@redhat.com>
References: <20180115085528.32663-1-peterx@redhat.com>
From: Marc-Andre Lureau <mlureau@redhat.com>
Date: Mon, 15 Jan 2018 12:22:48 +0100
Message-ID: <CAMxuvax0N5qf5DN5tkmvSEkVcGXMQzfBAY+MxLqJGEttYQnuKQ@mail.gmail.com>
Subject: Re: [PATCH] fw_cfg: don't use DMA mapping for fw_cfg device
To: Peter Xu <peterx@redhat.com>
Cc: qemu-devel <qemu-devel@nongnu.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "Michael S . Tsirkin" <mst@redhat.com>,
        =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= <marcandre.lureau@redhat.com>,
        Baoquan He <bhe@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Return-Path: <linux-kernel-owner@vger.kernel.org>

Hi

On Mon, Jan 15, 2018 at 9:55 AM, Peter Xu <peterx@redhat.com> wrote:
> fw_cfg device does not need IOMMU protection, so use physical addresses
> always.  That's how QEMU implements fw_cfg.  Otherwise we'll see call
> traces during boot when vIOMMU is enabled in guest:
>
> [    1.018306] ------------[ cut here ]------------
> [    1.018314] WARNING: CPU: 1 PID: 1 at drivers/firmware/qemu_fw_cfg.c:152 fw_cfg_dma_transfer+0x399/0x500
> [    1.018315] fw_cfg_dma_transfer: failed to map fw_cfg_dma
> [    1.018316] Modules linked in:
> [    1.018320] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 3.10.0-827.el7.x86_64 #1
> [    1.018321] Hardware name: Red Hat KVM, BIOS 1.11.0-1.el7 04/01/2014
> [    1.018322] Call Trace:
> [    1.018330]  [<ffffffffafcf6b3a>] dump_stack+0x19/0x1b
> [    1.018334]  [<ffffffffaf68fff8>] __warn+0xd8/0x100
> [    1.018336]  [<ffffffffaf69007f>] warn_slowpath_fmt+0x5f/0x80
> [    1.018338]  [<ffffffffafb67329>] fw_cfg_dma_transfer+0x399/0x500
> [    1.018340]  [<ffffffffafb6753c>] fw_cfg_read_blob+0xac/0x1c0
> [    1.018342]  [<ffffffffafb67720>] fw_cfg_register_dir_entries+0x80/0x450
> [    1.018344]  [<ffffffffafb67d02>] fw_cfg_sysfs_probe+0x212/0x3f0
> [    1.018347]  [<ffffffffafa70352>] platform_drv_probe+0x42/0x110
> [    1.018350]  [<ffffffffafa6e002>] driver_probe_device+0xc2/0x3e0
> [    1.018352]  [<ffffffffafa6e3f3>] __driver_attach+0x93/0xa0
> [    1.018354]  [<ffffffffafa6e360>] ? __device_attach+0x40/0x40
> [    1.018359]  [<ffffffffafa6bbd3>] bus_for_each_dev+0x73/0xc0
> [    1.018362]  [<ffffffffafa6d97e>] driver_attach+0x1e/0x20
> [    1.018364]  [<ffffffffafa6d420>] bus_add_driver+0x200/0x2d0
> [    1.018366]  [<ffffffffb03c2041>] ? firmware_map_add_early+0x58/0x58
> [    1.018368]  [<ffffffffafa6ea84>] driver_register+0x64/0xf0
> [    1.018370]  [<ffffffffafa7013a>] __platform_driver_register+0x4a/0x50
> [    1.018372]  [<ffffffffb03c2075>] fw_cfg_sysfs_init+0x34/0x61
> [    1.018376]  [<ffffffffaf602108>] do_one_initcall+0xb8/0x230
> [    1.018379]  [<ffffffffb036b34a>] kernel_init_freeable+0x17a/0x219
> [    1.018381]  [<ffffffffb036ab19>] ? initcall_blacklist+0xb0/0xb0
> [    1.018383]  [<ffffffffafce5b30>] ? rest_init+0x80/0x80
> [    1.018385]  [<ffffffffafce5b3e>] kernel_init+0xe/0xf0
> [    1.018388]  [<ffffffffafd08898>] ret_from_fork+0x58/0x90
> [    1.018390]  [<ffffffffafce5b30>] ? rest_init+0x80/0x80
> [    1.018392] ---[ end trace d00a5b71608a8f59 ]---
>
> Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1533367
> Fixes: e90cb816599b ("fw_cfg: do DMA read operation", 2017-11-28)
> CC: Marc-André Lureau <marcandre.lureau@redhat.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> --
>
> This is based on tree:
>   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git/log/?h=vhost
>
> Please review, thanks.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

The DMA business is confusing, sadly I didn't get much clue what I was
supposed to do. What I can say:

Tested-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Should the series be removed from Michael tree and I squash your fix &
send a v10?

Fwiw, "fw_cfg: write vmcoreinfo details" should also be fixed to
allocate memory (unless your approach fixes that?)

thanks

> ---
>  drivers/firmware/qemu_fw_cfg.c | 37 ++++++++-----------------------------
>  1 file changed, 8 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
> index 6eb5d8f43c3e..62a44bc81a88 100644
> --- a/drivers/firmware/qemu_fw_cfg.c
> +++ b/drivers/firmware/qemu_fw_cfg.c
> @@ -106,11 +106,12 @@ static inline bool fw_cfg_dma_enabled(void)
>  }
>
>  /* qemu fw_cfg device is sync today, but spec says it may become async */
> -static void fw_cfg_wait_for_control(struct fw_cfg_dma *d, dma_addr_t dma)
> +static void fw_cfg_wait_for_control(struct fw_cfg_dma *d)
>  {
>         do {
> -               dma_sync_single_for_cpu(dev, dma, sizeof(*d), DMA_FROM_DEVICE);
> -               if ((be32_to_cpu(d->control) & ~FW_CFG_DMA_CTL_ERROR) == 0)
> +               u32 ctrl = be32_to_cpu(READ_ONCE(d->control));
> +
> +               if ((ctrl & ~FW_CFG_DMA_CTL_ERROR) == 0)
>                         return;
>
>                 usleep_range(50, 100);
> @@ -119,21 +120,9 @@ static void fw_cfg_wait_for_control(struct fw_cfg_dma *d, dma_addr_t dma)
>
>  static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
>  {
> -       dma_addr_t dma_addr = 0;
>         struct fw_cfg_dma *d = NULL;
> -       dma_addr_t dma;
>         ssize_t ret = length;
> -       enum dma_data_direction dir =
> -               (control & FW_CFG_DMA_CTL_READ ? DMA_FROM_DEVICE : 0) |
> -               (control & FW_CFG_DMA_CTL_WRITE ? DMA_TO_DEVICE : 0);
> -
> -       if (address && length) {
> -               dma_addr = dma_map_single(dev, address, length, dir);
> -               if (dma_mapping_error(NULL, dma_addr)) {
> -                       WARN(1, "%s: failed to map address\n", __func__);
> -                       return -EFAULT;
> -               }
> -       }
> +       phys_addr_t dma;
>
>         d = kmalloc(sizeof(*d), GFP_KERNEL);
>         if (!d) {
> @@ -142,34 +131,24 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
>         }
>
>         *d = (struct fw_cfg_dma) {
> -               .address = cpu_to_be64(dma_addr),
> +               .address = cpu_to_be64(virt_to_phys(address)),
>                 .length = cpu_to_be32(length),
>                 .control = cpu_to_be32(control)
>         };
>
> -       dma = dma_map_single(dev, d, sizeof(*d), DMA_BIDIRECTIONAL);
> -       if (dma_mapping_error(NULL, dma)) {
> -               WARN(1, "%s: failed to map fw_cfg_dma\n", __func__);
> -               ret = -EFAULT;
> -               goto end;
> -       }
> +       dma = virt_to_phys(d);
>
>         iowrite32be((u64)dma >> 32, fw_cfg_reg_dma);
>         iowrite32be(dma, fw_cfg_reg_dma + 4);
>
> -       fw_cfg_wait_for_control(d, dma);
> +       fw_cfg_wait_for_control(d);
>
>         if (be32_to_cpu(d->control) & FW_CFG_DMA_CTL_ERROR) {
>                 ret = -EIO;
>         }
>
> -       dma_unmap_single(dev, dma, sizeof(*d), DMA_BIDIRECTIONAL);
> -
>  end:
>         kfree(d);
> -       if (dma_addr)
> -               dma_unmap_single(dev, dma_addr, length, dir);
> -
>         return ret;
>  }
>
> --
> 2.14.3
>