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=-2.4 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 6ACACC432C0 for ; Mon, 25 Nov 2019 17:48:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3D92A20835 for ; Mon, 25 Nov 2019 17:48:24 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="avnCZcmn" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725851AbfKYRsX (ORCPT ); Mon, 25 Nov 2019 12:48:23 -0500 Received: from mail-wr1-f66.google.com ([209.85.221.66]:37154 "EHLO mail-wr1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727464AbfKYRsX (ORCPT ); Mon, 25 Nov 2019 12:48:23 -0500 Received: by mail-wr1-f66.google.com with SMTP id t1so19213203wrv.4 for ; Mon, 25 Nov 2019 09:48:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=BgkOCn6dEkaCecb5ZyLmsM0vtVrRc/X2TTNPZzp0BGs=; b=avnCZcmnWQYRn2ejXtbreBr+2fzx+QirQ6jMfKAMwEDo3jyctcQvSS6SM6aBRJCAeP kfDeyJNKwf8Ww1fdC45IB/7jKdswYHhGPaF1CAK1k/eHulj1Z8NDM4NLugYgnTl5DEOo YefFZFkbA6n3NhXip9hDUOr85MJpTQj4qNSE9VQlJPLdQNIY7YvpyPpEkKFXnyOnZTKk zxPRxvEFW8WTSxxiqEYBqSc/EW+pCq8OXdDzqUhlOxxL17HZqZAjCdkw8lFuH7Nliyrm MDDF0iP1wW7sCg4O3wkJdWYO3+EHez48clfhcdiT/8bJ80T/RuVV1xpWQm7lYwPafspo 67oQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=BgkOCn6dEkaCecb5ZyLmsM0vtVrRc/X2TTNPZzp0BGs=; b=KITyoZVSj92s3v6jsdTXEKcUXEwqMK8Z9vxwxmNKCKomAszdOteXMLk/VaNuWXldlI VMz6wmqeLeT4nJd1iw0lAxIU6Y0Gd/b54gKD9SeNFFAgYuv1VU/wT4QURvL0Q6SWSxO0 /N8WvU5aA7vm5PGoLw9p5Qpzwabzh/P6AL7K49lMdwbr8FqcbzlzuqdexJY6fhkyT1UL XT7iPxzPjV3cGc1o+yL/I6snxHaWpInenNE7NvWO41F7ciuN82DGBH3RSUCoMDl+vkUp zxN2oXqgsiAVvhT014yaCntYgCkQw3An30J7XPahtvXBYygBtuT8USnJoTopx85BgDH8 5JbQ== X-Gm-Message-State: APjAAAUvtYnZcRAVNatYD8BFmqmWaAxbRgfPY6bBWxoCfiQyc5+TtHN7 CkYACRAAJ4TzAAdHCV1e//Mq6g== X-Google-Smtp-Source: APXvYqzTUYJqRjRrvNHNtPJwWI/KyemjW5baiqCvdsWHNGy9hgofkwoCe9NS+y/KmUwV3VIZ5E9NFQ== X-Received: by 2002:a5d:4acb:: with SMTP id y11mr12150855wrs.106.1574704100857; Mon, 25 Nov 2019 09:48:20 -0800 (PST) Received: from lophozonia (xdsl-188-155-204-106.adslplus.ch. [188.155.204.106]) by smtp.gmail.com with ESMTPSA id x7sm11127238wrq.41.2019.11.25.09.48.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Nov 2019 09:48:20 -0800 (PST) Date: Mon, 25 Nov 2019 18:48:17 +0100 From: Jean-Philippe Brucker To: "Michael S. Tsirkin" Cc: linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, iommu@lists.linux-foundation.org, virtualization@lists.linux-foundation.org, linux-pci@vger.kernel.org, virtio-dev@lists.oasis-open.org, rjw@rjwysocki.net, lenb@kernel.org, lorenzo.pieralisi@arm.com, guohanjun@huawei.com, sudeep.holla@arm.com, gregkh@linuxfoundation.org, joro@8bytes.org, bhelgaas@google.com, jasowang@redhat.com, jacob.jun.pan@intel.com, eric.auger@redhat.com, sebastien.boeuf@intel.com, kevin.tian@intel.com Subject: Re: [RFC 13/13] iommu/virtio: Add topology description to Message-ID: <20191125174817.GB945122@lophozonia> References: <20191122105000.800410-1-jean-philippe@linaro.org> <20191122105000.800410-14-jean-philippe@linaro.org> <20191122072753-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191122072753-mutt-send-email-mst@kernel.org> User-Agent: Mutt/1.12.2 (2019-09-21) Sender: linux-acpi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org On Fri, Nov 22, 2019 at 07:53:19AM -0500, Michael S. Tsirkin wrote: > Overall this looks good to me. The only point is that > I think the way the interface is designed makes writing > the driver a bit too difficult. Idea: if instead we just > have a length field and then an array of records > (preferably unions so we don't need to work hard), > we can shadow that into memory, then iterate over > the unions. > > Maybe add a uniform record length + number of records field. > Then just skip types you do not know how to handle. > This will also help make sure it's within bounds. > > What do you think? Sounds good, that should simplify the implementation a bit. > You will need to do something to address the TODO I think. Yes, I'll try to figure out a way to test platform devices. > > +static void viommu_cwrite(struct pci_dev *dev, int cfg, > > + struct viommu_cap_config *cap, u32 length, u32 offset, > > + u32 val) > > A single user with 4 byte parameter. Just open-code? Ok > > + cap.head.type = viommu_cread(dev, pci_cfg, dev_cfg, 2, offset); > > + cap.head.next = viommu_cread(dev, pci_cfg, dev_cfg, 2, offset + 2); > > All of this doesn't seem to be endian-clean. Try running sparse I think > it will complain. It does, I'll fix this > > @@ -36,6 +37,31 @@ struct virtio_iommu_config { > > struct virtio_iommu_range_32 domain_range; > > /* Probe buffer size */ > > __le32 probe_size; > > + /* Offset to the beginning of the topology table */ > > + __le16 topo_offset; > > why do we need an offset? I find it awkward to put a variable-size array in the middle of the config. The virtio_iommu_config struct would be easier to extend later if we keep the array at the end and only define small static fields here. > > > +}; > > + > > +struct virtio_iommu_topo_head { > > + __le16 type; > > + __le16 next; > > +}; > > So this linked list makes things harder than necessary imho. > It will be easier to just have a counter with # of records. > Then make all records the same size. > Then just read each record out into a buffer, and > handle it there. Yes, that should simplify things. Thanks, Jean 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=-2.0 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no 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 6A269C432C0 for ; Mon, 25 Nov 2019 17:48:27 +0000 (UTC) Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2C7FE20679 for ; Mon, 25 Nov 2019 17:48:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="avnCZcmn" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2C7FE20679 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=iommu-bounces@lists.linux-foundation.org Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id EA25120033; Mon, 25 Nov 2019 17:48:26 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id moRyZzea9pUY; Mon, 25 Nov 2019 17:48:26 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id 27C4D203D4; Mon, 25 Nov 2019 17:48:26 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id E94FDC0881; Mon, 25 Nov 2019 17:48:25 +0000 (UTC) Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id E8A7BC0878 for ; Mon, 25 Nov 2019 17:48:24 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id C71908797A for ; Mon, 25 Nov 2019 17:48:24 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id HyHtnDUvaU0z for ; Mon, 25 Nov 2019 17:48:22 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by hemlock.osuosl.org (Postfix) with ESMTPS id 7E7588796C for ; Mon, 25 Nov 2019 17:48:22 +0000 (UTC) Received: by mail-wr1-f68.google.com with SMTP id z10so19152381wrs.12 for ; Mon, 25 Nov 2019 09:48:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=BgkOCn6dEkaCecb5ZyLmsM0vtVrRc/X2TTNPZzp0BGs=; b=avnCZcmnWQYRn2ejXtbreBr+2fzx+QirQ6jMfKAMwEDo3jyctcQvSS6SM6aBRJCAeP kfDeyJNKwf8Ww1fdC45IB/7jKdswYHhGPaF1CAK1k/eHulj1Z8NDM4NLugYgnTl5DEOo YefFZFkbA6n3NhXip9hDUOr85MJpTQj4qNSE9VQlJPLdQNIY7YvpyPpEkKFXnyOnZTKk zxPRxvEFW8WTSxxiqEYBqSc/EW+pCq8OXdDzqUhlOxxL17HZqZAjCdkw8lFuH7Nliyrm MDDF0iP1wW7sCg4O3wkJdWYO3+EHez48clfhcdiT/8bJ80T/RuVV1xpWQm7lYwPafspo 67oQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=BgkOCn6dEkaCecb5ZyLmsM0vtVrRc/X2TTNPZzp0BGs=; b=NHklXT1uKDspql21Q174cWmuuGkt4Cp8102ATAB+iKCris7uiS0ATwYecUjpP2xb90 Wo3WnQBdUhSwq6xXaFiD4soUUb+KBNUbwlE8hIffVlKbc6glptkXDsT12vP3ZBcRZHwD PtqVACvCx3p4+qKB61ipkSk18D0YUbPyPBVunqb548/dBJEHUJnYoIAk05QPL6oCvGTQ yAsEsWJhi+FvuWF9/leZjtU/XTIMwOh0rwk1+6jCVy3VqCr12Mzp27T79/l8WMLviUnA gVSv6HwGkpBqzi60CtJ7/c/9g8Owq/K7s2s3iZ5qeZu0H9Ad072ccWWkwrFavNLvQNyi YIsQ== X-Gm-Message-State: APjAAAWZ1zbp2+C8eB4RosqDYLmxP3TNzaZcEjcNmqCT0cy3Na4kIaR8 DBP2j20P28cksXt+WvqEvKlNLw== X-Google-Smtp-Source: APXvYqzTUYJqRjRrvNHNtPJwWI/KyemjW5baiqCvdsWHNGy9hgofkwoCe9NS+y/KmUwV3VIZ5E9NFQ== X-Received: by 2002:a5d:4acb:: with SMTP id y11mr12150855wrs.106.1574704100857; Mon, 25 Nov 2019 09:48:20 -0800 (PST) Received: from lophozonia (xdsl-188-155-204-106.adslplus.ch. [188.155.204.106]) by smtp.gmail.com with ESMTPSA id x7sm11127238wrq.41.2019.11.25.09.48.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Nov 2019 09:48:20 -0800 (PST) Date: Mon, 25 Nov 2019 18:48:17 +0100 From: Jean-Philippe Brucker To: "Michael S. Tsirkin" Subject: Re: [RFC 13/13] iommu/virtio: Add topology description to Message-ID: <20191125174817.GB945122@lophozonia> References: <20191122105000.800410-1-jean-philippe@linaro.org> <20191122105000.800410-14-jean-philippe@linaro.org> <20191122072753-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20191122072753-mutt-send-email-mst@kernel.org> User-Agent: Mutt/1.12.2 (2019-09-21) Cc: virtio-dev@lists.oasis-open.org, kevin.tian@intel.com, gregkh@linuxfoundation.org, linux-pci@vger.kernel.org, sudeep.holla@arm.com, rjw@rjwysocki.net, virtualization@lists.linux-foundation.org, linux-acpi@vger.kernel.org, iommu@lists.linux-foundation.org, sebastien.boeuf@intel.com, jacob.jun.pan@intel.com, guohanjun@huawei.com, bhelgaas@google.com, jasowang@redhat.com, linux-arm-kernel@lists.infradead.org, lenb@kernel.org X-BeenThere: iommu@lists.linux-foundation.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Development issues for Linux IOMMU support List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: iommu-bounces@lists.linux-foundation.org Sender: "iommu" On Fri, Nov 22, 2019 at 07:53:19AM -0500, Michael S. Tsirkin wrote: > Overall this looks good to me. The only point is that > I think the way the interface is designed makes writing > the driver a bit too difficult. Idea: if instead we just > have a length field and then an array of records > (preferably unions so we don't need to work hard), > we can shadow that into memory, then iterate over > the unions. > > Maybe add a uniform record length + number of records field. > Then just skip types you do not know how to handle. > This will also help make sure it's within bounds. > > What do you think? Sounds good, that should simplify the implementation a bit. > You will need to do something to address the TODO I think. Yes, I'll try to figure out a way to test platform devices. > > +static void viommu_cwrite(struct pci_dev *dev, int cfg, > > + struct viommu_cap_config *cap, u32 length, u32 offset, > > + u32 val) > > A single user with 4 byte parameter. Just open-code? Ok > > + cap.head.type = viommu_cread(dev, pci_cfg, dev_cfg, 2, offset); > > + cap.head.next = viommu_cread(dev, pci_cfg, dev_cfg, 2, offset + 2); > > All of this doesn't seem to be endian-clean. Try running sparse I think > it will complain. It does, I'll fix this > > @@ -36,6 +37,31 @@ struct virtio_iommu_config { > > struct virtio_iommu_range_32 domain_range; > > /* Probe buffer size */ > > __le32 probe_size; > > + /* Offset to the beginning of the topology table */ > > + __le16 topo_offset; > > why do we need an offset? I find it awkward to put a variable-size array in the middle of the config. The virtio_iommu_config struct would be easier to extend later if we keep the array at the end and only define small static fields here. > > > +}; > > + > > +struct virtio_iommu_topo_head { > > + __le16 type; > > + __le16 next; > > +}; > > So this linked list makes things harder than necessary imho. > It will be easier to just have a counter with # of records. > Then make all records the same size. > Then just read each record out into a buffer, and > handle it there. Yes, that should simplify things. Thanks, Jean _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu 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=-2.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 53CD7C432C0 for ; Mon, 25 Nov 2019 17:48:29 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2A7EC20679 for ; Mon, 25 Nov 2019 17:48:29 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="AfQFjd9d"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="avnCZcmn" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2A7EC20679 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Sgyq2XBnFAmO5buNx9MTjdGGnWlX61gHYkrdOskg9V8=; b=AfQFjd9du/T+7i FiHvf0xfh0V6HZpyFKDTC9lLnr+EdDVpQmYtbyMoJ8L+nEZ83Co525HqKISS0SVVYJHNNe7hpFHLP O1xJpdeEpVr/rRDAc2AXKPJ1rr1QKBUi/v9IhkWGM9Q2P0gONifxmPMd2sM70F5JzE86EJVUxEzFo 5uCmpuNsX9TAypPU6n62CRvBUr0VysrDX+biWDKtG5/mDbEqn1DURu56VE75Q+Br2rxjLBKp5Ca3N OLY5UJm45EYWRcAYsHhPs081lrWnzJEYxBhZOAdCJ+gzav44xBvi1c7zNyvcJuI5t8Z9xXmin4ubf ZyOQZ2O6CIoEsdt6RhDQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1iZITM-0003Z0-EN; Mon, 25 Nov 2019 17:48:28 +0000 Received: from mail-wr1-x442.google.com ([2a00:1450:4864:20::442]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1iZITI-0003Y7-HL for linux-arm-kernel@lists.infradead.org; Mon, 25 Nov 2019 17:48:26 +0000 Received: by mail-wr1-x442.google.com with SMTP id z3so19219982wru.3 for ; Mon, 25 Nov 2019 09:48:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=BgkOCn6dEkaCecb5ZyLmsM0vtVrRc/X2TTNPZzp0BGs=; b=avnCZcmnWQYRn2ejXtbreBr+2fzx+QirQ6jMfKAMwEDo3jyctcQvSS6SM6aBRJCAeP kfDeyJNKwf8Ww1fdC45IB/7jKdswYHhGPaF1CAK1k/eHulj1Z8NDM4NLugYgnTl5DEOo YefFZFkbA6n3NhXip9hDUOr85MJpTQj4qNSE9VQlJPLdQNIY7YvpyPpEkKFXnyOnZTKk zxPRxvEFW8WTSxxiqEYBqSc/EW+pCq8OXdDzqUhlOxxL17HZqZAjCdkw8lFuH7Nliyrm MDDF0iP1wW7sCg4O3wkJdWYO3+EHez48clfhcdiT/8bJ80T/RuVV1xpWQm7lYwPafspo 67oQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=BgkOCn6dEkaCecb5ZyLmsM0vtVrRc/X2TTNPZzp0BGs=; b=men2W8C+fZNP9+mmjY/vmlMgfTuGxSC4h83PSqcfEaSy5eGDVxHmqrUnrmuH73o1JV clzYnloOca0DyVjNu8pULnwWjOrp2VPnomGCXxd/4AAy6DIcboHuPhg1Aryq4LnKG1ZM is5/t+0Pwe86yiZGzJG6iChGhCsdudOHhJ59nlD2oWHaEzVqqM+rZo7uwPqJo3N1/jPu tSV9VXIDgDGKr65Eu9Fz+Yxr4MijymtAlliyE7lZQPHeoXDbgACPOo9lIqtK9Yh/5aeO X2kPrcmXNzkF3SqexWace1CT2j8tFBIXg/c40tFZHv76ws/h6sYjmZEcEVhFe7Ws7cLq Dq9A== X-Gm-Message-State: APjAAAW0gIUVDqXocKegTicQ5MobGYoBHilt2l2iHGreOKFP9OukWmzT uFhepzGQ/CyyLS7T4+3j3Y8gcQ== X-Google-Smtp-Source: APXvYqzTUYJqRjRrvNHNtPJwWI/KyemjW5baiqCvdsWHNGy9hgofkwoCe9NS+y/KmUwV3VIZ5E9NFQ== X-Received: by 2002:a5d:4acb:: with SMTP id y11mr12150855wrs.106.1574704100857; Mon, 25 Nov 2019 09:48:20 -0800 (PST) Received: from lophozonia (xdsl-188-155-204-106.adslplus.ch. [188.155.204.106]) by smtp.gmail.com with ESMTPSA id x7sm11127238wrq.41.2019.11.25.09.48.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 25 Nov 2019 09:48:20 -0800 (PST) Date: Mon, 25 Nov 2019 18:48:17 +0100 From: Jean-Philippe Brucker To: "Michael S. Tsirkin" Subject: Re: [RFC 13/13] iommu/virtio: Add topology description to Message-ID: <20191125174817.GB945122@lophozonia> References: <20191122105000.800410-1-jean-philippe@linaro.org> <20191122105000.800410-14-jean-philippe@linaro.org> <20191122072753-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20191122072753-mutt-send-email-mst@kernel.org> User-Agent: Mutt/1.12.2 (2019-09-21) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20191125_094824_706450_68F5ECF0 X-CRM114-Status: GOOD ( 19.03 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: virtio-dev@lists.oasis-open.org, kevin.tian@intel.com, lorenzo.pieralisi@arm.com, gregkh@linuxfoundation.org, linux-pci@vger.kernel.org, joro@8bytes.org, sudeep.holla@arm.com, rjw@rjwysocki.net, virtualization@lists.linux-foundation.org, linux-acpi@vger.kernel.org, iommu@lists.linux-foundation.org, sebastien.boeuf@intel.com, jacob.jun.pan@intel.com, eric.auger@redhat.com, guohanjun@huawei.com, bhelgaas@google.com, jasowang@redhat.com, linux-arm-kernel@lists.infradead.org, lenb@kernel.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Nov 22, 2019 at 07:53:19AM -0500, Michael S. Tsirkin wrote: > Overall this looks good to me. The only point is that > I think the way the interface is designed makes writing > the driver a bit too difficult. Idea: if instead we just > have a length field and then an array of records > (preferably unions so we don't need to work hard), > we can shadow that into memory, then iterate over > the unions. > > Maybe add a uniform record length + number of records field. > Then just skip types you do not know how to handle. > This will also help make sure it's within bounds. > > What do you think? Sounds good, that should simplify the implementation a bit. > You will need to do something to address the TODO I think. Yes, I'll try to figure out a way to test platform devices. > > +static void viommu_cwrite(struct pci_dev *dev, int cfg, > > + struct viommu_cap_config *cap, u32 length, u32 offset, > > + u32 val) > > A single user with 4 byte parameter. Just open-code? Ok > > + cap.head.type = viommu_cread(dev, pci_cfg, dev_cfg, 2, offset); > > + cap.head.next = viommu_cread(dev, pci_cfg, dev_cfg, 2, offset + 2); > > All of this doesn't seem to be endian-clean. Try running sparse I think > it will complain. It does, I'll fix this > > @@ -36,6 +37,31 @@ struct virtio_iommu_config { > > struct virtio_iommu_range_32 domain_range; > > /* Probe buffer size */ > > __le32 probe_size; > > + /* Offset to the beginning of the topology table */ > > + __le16 topo_offset; > > why do we need an offset? I find it awkward to put a variable-size array in the middle of the config. The virtio_iommu_config struct would be easier to extend later if we keep the array at the end and only define small static fields here. > > > +}; > > + > > +struct virtio_iommu_topo_head { > > + __le16 type; > > + __le16 next; > > +}; > > So this linked list makes things harder than necessary imho. > It will be easier to just have a counter with # of records. > Then make all records the same size. > Then just read each record out into a buffer, and > handle it there. Yes, that should simplify things. Thanks, Jean _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: virtio-dev-return-6403-cohuck=redhat.com@lists.oasis-open.org Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Received: from lists.oasis-open.org (oasis-open.org [10.110.1.242]) by lists.oasis-open.org (Postfix) with ESMTP id 5B14E985DB8 for ; Mon, 25 Nov 2019 17:48:22 +0000 (UTC) Date: Mon, 25 Nov 2019 18:48:17 +0100 From: Jean-Philippe Brucker Message-ID: <20191125174817.GB945122@lophozonia> References: <20191122105000.800410-1-jean-philippe@linaro.org> <20191122105000.800410-14-jean-philippe@linaro.org> <20191122072753-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 In-Reply-To: <20191122072753-mutt-send-email-mst@kernel.org> Subject: [virtio-dev] Re: [RFC 13/13] iommu/virtio: Add topology description to Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline To: "Michael S. Tsirkin" Cc: linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, iommu@lists.linux-foundation.org, virtualization@lists.linux-foundation.org, linux-pci@vger.kernel.org, virtio-dev@lists.oasis-open.org, rjw@rjwysocki.net, lenb@kernel.org, lorenzo.pieralisi@arm.com, guohanjun@huawei.com, sudeep.holla@arm.com, gregkh@linuxfoundation.org, joro@8bytes.org, bhelgaas@google.com, jasowang@redhat.com, jacob.jun.pan@intel.com, eric.auger@redhat.com, sebastien.boeuf@intel.com, kevin.tian@intel.com List-ID: On Fri, Nov 22, 2019 at 07:53:19AM -0500, Michael S. Tsirkin wrote: > Overall this looks good to me. The only point is that > I think the way the interface is designed makes writing > the driver a bit too difficult. Idea: if instead we just > have a length field and then an array of records > (preferably unions so we don't need to work hard), > we can shadow that into memory, then iterate over > the unions. >=20 > Maybe add a uniform record length + number of records field. > Then just skip types you do not know how to handle. > This will also help make sure it's within bounds. >=20 > What do you think? Sounds good, that should simplify the implementation a bit. > You will need to do something to address the TODO I think. Yes, I'll try to figure out a way to test platform devices. > > +static void viommu_cwrite(struct pci_dev *dev, int cfg, > > +=09=09=09 struct viommu_cap_config *cap, u32 length, u32 offset, > > +=09=09=09 u32 val) >=20 > A single user with 4 byte parameter. Just open-code? Ok > > +=09=09cap.head.type =3D viommu_cread(dev, pci_cfg, dev_cfg, 2, offset)= ; > > +=09=09cap.head.next =3D viommu_cread(dev, pci_cfg, dev_cfg, 2, offset = + 2); >=20 > All of this doesn't seem to be endian-clean. Try running sparse I think > it will complain. It does, I'll fix this > > @@ -36,6 +37,31 @@ struct virtio_iommu_config { > > =09struct virtio_iommu_range_32=09=09domain_range; > > =09/* Probe buffer size */ > > =09__le32=09=09=09=09=09probe_size; > > +=09/* Offset to the beginning of the topology table */ > > +=09__le16=09=09=09=09=09topo_offset; >=20 > why do we need an offset? I find it awkward to put a variable-size array in the middle of the config. The virtio_iommu_config struct would be easier to extend later if we keep the array at the end and only define small static fields here. >=20 > > +}; > > + > > +struct virtio_iommu_topo_head { > > +=09__le16=09=09=09=09=09type; > > +=09__le16=09=09=09=09=09next; > > +}; >=20 > So this linked list makes things harder than necessary imho. > It will be easier to just have a counter with # of records. > Then make all records the same size. > Then just read each record out into a buffer, and > handle it there. Yes, that should simplify things. Thanks, Jean --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org