Received: by 2002:a05:6a10:1d13:0:0:0:0 with SMTP id pp19csp1206542pxb; Thu, 19 Aug 2021 23:51:56 -0700 (PDT) X-Google-Smtp-Source: ABdhPJy4RjGHihiaP5SOpE8/4PPanIYprqm7DIVYje4cfLbBUcgFNKAXvrcCOTK2nN9LWWAaMPbF X-Received: by 2002:aa7:d54f:: with SMTP id u15mr18101304edr.178.1629442316556; Thu, 19 Aug 2021 23:51:56 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1629442316; cv=none; d=google.com; s=arc-20160816; b=xVnJuDOiwsSVCAqpNT7iHEfeW3A6ooLzQaNiHJjC0wsQYakeqvVW8umvuj670EWJMQ s3L81Lm9QCesS3yC+YEnyQUkW7vTpxA5oLVixUBGXDlu2cj3RzWo6J6aw3/jlIQsgqxJ nFrLm5QMoNdCIjn9TJ1d6Bkjwbri0Jgg5jKotwOQ8Xx0NIWYCBMPZ9LRfXmKN72ATmFy +Uvr0gNY+ZCQtirN1uRFbaxLXhN4IOnvliTahp48oE4/FYlyJAkkFt1ZHNPE8B6sd5wt fYubS6nOtX8ZSAnZLl913jxFbZQl3QgM45M0KLj12JAbIuT9iY6CJmCdAfI7YEIn9HQ1 QQcA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:mime-version:message-id:date:references :in-reply-to:subject:cc:to:from:dkim-signature; bh=gouExDkcyZl3ijmObUC+G2ORUVV3VspK7d7Fu2DyAHg=; b=SGw2QEQyDyy7TOG4GwetFnXO6tz3EtqJOS2xpD14Ueak0cHN+8EPMq6PytoEuy1bqo aaxIj5fB42xW6UYDAnkrpkD0KicJ2ZZvY9qnmcgat5WenJMy1wFlFDJQmx9GqvRm/tiG 5Qo/PWxSu/Eu3fqWcRuFOed/s7wyXYg55APqJTH0B2P9FLTcIl0IUG49hi2Q9a3IffmD BxIslp+EHZR0OCUAU4LJr28YtItyqoMoVMvtoKFDmcaG9SUkdS4xrZ2I1h7T5PqSdrWF qqT/HUaZ9xcwwJdSwJKZhGGY/haXHUsSR+230r8sgYPftKp1CnH1hcQn/N1ji7E0qOUE cWFA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@axtens.net header.s=google header.b=nTPXXxyl; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id j14si6079231edk.558.2021.08.19.23.51.34; Thu, 19 Aug 2021 23:51:56 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@axtens.net header.s=google header.b=nTPXXxyl; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238506AbhHTGu3 (ORCPT + 99 others); Fri, 20 Aug 2021 02:50:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41498 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231998AbhHTGu2 (ORCPT ); Fri, 20 Aug 2021 02:50:28 -0400 Received: from mail-pg1-x52f.google.com (mail-pg1-x52f.google.com [IPv6:2607:f8b0:4864:20::52f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B32E4C061575 for ; Thu, 19 Aug 2021 23:49:51 -0700 (PDT) Received: by mail-pg1-x52f.google.com with SMTP id t1so8221536pgv.3 for ; Thu, 19 Aug 2021 23:49:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google; h=from:to:cc:subject:in-reply-to:references:date:message-id :mime-version; bh=gouExDkcyZl3ijmObUC+G2ORUVV3VspK7d7Fu2DyAHg=; b=nTPXXxylyEXDYBekIaAIzp+BtlWfWK9uk9zQ2AZQeRrD58IjWAI3Xax0m2/WDtnBA4 HZXtXcI8R8qZ1MFS7Th4L0ULWT98saZd4j8GUX7yb4Jj7ven42yOiobysolY2N2Pdsqz rTgPY50k0Ct1FbBLrkGUtgBJMg9FdJMBML7aQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date :message-id:mime-version; bh=gouExDkcyZl3ijmObUC+G2ORUVV3VspK7d7Fu2DyAHg=; b=chgT5DRuY2yxMyvit9QNs1eIROC2Z4H2YSxm4dPIi6RV7Vsvjerw2V5F4a8+HXpJo1 PIEllSXz35CRSCLoMvyxhWYAMOoFHtS1pv7N8lzedl//xmQNhECqeElapUhc/khQupPn 5IdMgr0aPpOxraaUhA3nPGmgjTayPGWLw4uUWxEzuKu2Y9b2HBBC0yim4hJN+g+Vnreo AufjMCxpeiQzdSTrwujnuLcFDxSy/MDGVInJqwLo0DVGoy/Pg+mAxJ2w54udZdyRabkV 0Aw55He3XVjBdsqYUkrkLUktVHvYDjwyc4/qxbXrP/bSxpeTLin25EUYWeBfaksxgUsI Pp0A== X-Gm-Message-State: AOAM530lsXk8Xc1BpYB18Sc4W+ZYBsCuehtLk0vp0vRzl5aLOGlP3Mqb 0gK0VRsoQG14CbmeB2siM5rX9g== X-Received: by 2002:a63:3245:: with SMTP id y66mr17150880pgy.443.1629442191180; Thu, 19 Aug 2021 23:49:51 -0700 (PDT) Received: from localhost ([2001:4479:e000:e400:a926:b5e4:f61:cefa]) by smtp.gmail.com with ESMTPSA id 22sm6475251pgn.88.2021.08.19.23.49.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 19 Aug 2021 23:49:50 -0700 (PDT) From: Daniel Axtens To: Xianting Tian , gregkh@linuxfoundation.org, jirislaby@kernel.org, amit@kernel.org, arnd@arndb.de, osandov@fb.com Cc: Xianting Tian , shile.zhang@linux.alibaba.com, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org Subject: Re: [PATCH v8 2/3] tty: hvc: pass DMA capable memory to put_chars() In-Reply-To: <20210818082122.166881-3-xianting.tian@linux.alibaba.com> References: <20210818082122.166881-1-xianting.tian@linux.alibaba.com> <20210818082122.166881-3-xianting.tian@linux.alibaba.com> Date: Fri, 20 Aug 2021 16:49:47 +1000 Message-ID: <87pmu8ehkk.fsf@linkitivity.dja.id.au> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Xianting Tian writes: > As well known, hvc backend driver(eg, virtio-console) can register its > operations to hvc framework. The operations can contain put_chars(), > get_chars() and so on. > > Some hvc backend may do dma in its operations. eg, put_chars() of > virtio-console. But in the code of hvc framework, it may pass DMA > incapable memory to put_chars() under a specific configuration, which > is explained in commit c4baad5029(virtio-console: avoid DMA from stack): We could also run into issues on powerpc where Andrew is working on adding vmap-stack but the opal hvc driver assumes that it is passed a buffer which is not in vmalloc space but in the linear mapping. So it would be good to fix this (or more clearly document what drivers can expect). > 1, c[] is on stack, > hvc_console_print(): > char c[N_OUTBUF] __ALIGNED__; > cons_ops[index]->put_chars(vtermnos[index], c, i); > 2, ch is on stack, > static void hvc_poll_put_char(,,char ch) > { > struct tty_struct *tty = driver->ttys[0]; > struct hvc_struct *hp = tty->driver_data; > int n; > > do { > n = hp->ops->put_chars(hp->vtermno, &ch, 1); > } while (n <= 0); > } > > Commit c4baad5029 is just the fix to avoid DMA from stack memory, which > is passed to virtio-console by hvc framework in above code. But I think > the fix is aggressive, it directly uses kmemdup() to alloc new buffer > from kmalloc area and do memcpy no matter the memory is in kmalloc area > or not. But most importantly, it should better be fixed in the hvc > framework, by changing it to never pass stack memory to the put_chars() > function in the first place. Otherwise, we still face the same issue if > a new hvc backend using dma added in the future. > > In this patch, we make 'char out_buf[N_OUTBUF]' and 'chat out_ch' part > of 'struct hvc_struct', so both two buf are no longer the stack memory. > we can use it in above two cases separately. > > Introduce another array(cons_outbufs[]) for buffer pointers next to > the cons_ops[] and vtermnos[] arrays. With the array, we can easily find > the buffer, instead of traversing hp list. > > With the patch, we can remove the fix c4baad5029. > > Signed-off-by: Xianting Tian > Reviewed-by: Shile Zhang > struct hvc_struct { > struct tty_port port; > spinlock_t lock; > int index; > int do_wakeup; > - char *outbuf; > - int outbuf_size; > int n_outbuf; > uint32_t vtermno; > const struct hv_ops *ops; > @@ -48,6 +56,10 @@ struct hvc_struct { > struct work_struct tty_resize; > struct list_head next; > unsigned long flags; > + char out_ch; > + char out_buf[N_OUTBUF] __ALIGNED__; > + int outbuf_size; > + char outbuf[0] __ALIGNED__; I'm trying to understand this patch but I am finding it very difficult to understand what the difference between `out_buf` and `outbuf` (without the underscore) is supposed to be. `out_buf` is statically sized and the size of `outbuf` is supposed to depend on the arguments to hvc_alloc(), but I can't quite figure out what the roles of each one are and their names are confusingly similiar! I looked briefly at the older revisions of the series but it didn't make things much clearer. Could you give them clearer names? Also, looking at Documentation/process/deprecated.rst, it looks like maybe we want to use a 'flexible array member' instead: .. note:: If you are using struct_size() on a structure containing a zero-length or a one-element array as a trailing array member, please refactor such array usage and switch to a `flexible array member <#zero-length-and-one-element-arrays>`_ instead. I think we want: > + char outbuf[] __ALIGNED__; Kind regards, Daniel