From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0F712C4338F for ; Fri, 20 Aug 2021 06:49:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E287E60F4A for ; Fri, 20 Aug 2021 06:49:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238575AbhHTGua (ORCPT ); Fri, 20 Aug 2021 02:50:30 -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-Google-Smtp-Source: ABdhPJxWz/9TmQnG6a6Du0TX4lp94P5/KwicmwvaBeBO644dQYvRpxIV+9458y43F5RM9soOs6IFHQ== 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