Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Thiago Jung Bauermann <bauerman@linux.ibm.com>
To: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
Cc: Joerg Roedel <joro@8bytes.org>,
	Mark Rutland <Mark.Rutland@arm.com>,
	"virtio-dev\@lists.oasis-open.org"
	<virtio-dev@lists.oasis-open.org>,
	"kevin.tian\@intel.com" <kevin.tian@intel.com>,
	"tnowicki\@caviumnetworks.com" <tnowicki@caviumnetworks.com>,
	"mst\@redhat.com" <mst@redhat.com>,
	Marc Zyngier <Marc.Zyngier@arm.com>,
	"linux-pci\@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"jasowang\@redhat.com" <jasowang@redhat.com>,
	Will Deacon <Will.Deacon@arm.com>,
	Robin Murphy <Robin.Murphy@arm.com>,
	"virtualization\@lists.linux-foundation.org" 
	<virtualization@lists.linux-foundation.org>,
	"iommu\@lists.linux-foundation.org" 
	<iommu@lists.linux-foundation.org>,
	"robh+dt\@kernel.org" <robh+dt@kernel.org>,
	"bhelgaas\@google.com" <bhelgaas@google.com>,
	"frowand.list\@gmail.com" <frowand.list@gmail.com>,
	"kvmarm\@lists.cs.columbia.edu" <kvmarm@lists.cs.columbia.edu>,
	"devicetree\@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [PATCH v7 0/7] Add virtio-iommu driver
Date: Thu, 21 Feb 2019 19:18:59 -0300
Message-ID: <878sy8bx9o.fsf@morokweng.localdomain> (raw)
In-Reply-To: <a6315ab3-bffb-7470-365d-b26df6524bda@arm.com>


Hello Jean-Philippe,

Jean-Philippe Brucker <jean-philippe.brucker@arm.com> writes:
> Makes sense, though I think other virtio devices have been developed a
> little more organically: device and driver code got upstreamed first,
> and then the specification describing their interface got merged into
> the standard. For example I believe that code for crypto, input and GPU
> devices were upstreamed long before the specification was merged. Once
> an implementation is upstream, the interface is expected to be
> backward-compatible (all subsequent changes are introduced using feature
> bits).
>
> So I've been working with this process in mind, also described by Jens
> at KVM forum 2017 [3]:
> (1) Reserve a device ID, and get that merged into virtio (ID 23 for
> virtio-iommu was reserved last year)
> (2) Open-source an implementation (this driver and Eric's device)
> (3) Formalize and upstream the device specification
>
> But I get that some overlap between (2) and (3) would have been better.
> So far the spec document has been reviewed mainly from the IOMMU point
> of view, and might require more changes to be in line with the other
> virtio devices -- hopefully just wording changes. I'll kick off step
> (3), but I think the virtio folks are a bit busy with finalizing the 1.1
> spec so I expect it to take a while.

I read v0.9 of the spec and have some minor comments, hope this is a
good place to send them:

1. In section 2.6.2, one reads

    If the VIRTIO_IOMMU_F_INPUT_RANGE feature is offered and the range
    described by fields virt_start and virt_end doesn’t fit in the range
    described by input_range, the device MAY set status to VIRTIO_-
    IOMMU_S_RANGE and ignore the request.

Shouldn't int say "If the VIRTIO_IOMMU_F_INPUT_RANGE feature is
negotiated" instead?

2. There's a typo at the end of section 2.6.5:

    The VIRTIO_IOMMU_MAP_F_MMIO flag is a memory type rather than a
    protection lag.

s/lag/flag/

3. In section 3.1.2.1.1, the viommu compatible field says "virtio,mmio".
Shouldn't it say "virtio,mmio-iommu" instead, to be consistent with
"virtio,pci-iommu"?

4. There's a typo in section 3.3:

    A host bridge may limit the input address space – transaction
    accessing some addresses won’t reach the physical IOMMU.

s/transaction/transactions/

I also have one last comment which you may freely ignore, considering
it's clearly just personal opinion and also considering that the
specification is mature at this point: it specifies memory ranges by
specifying start and end addresses. My experience has been that this is
error prone, leading to confusion and bugs regarding whether the end
address is inclusive or exclusive. I tend to prefer expressing memory
ranges by specifying a start address and a length, which eliminates
ambiguity.

--
Thiago Jung Bauermann
IBM Linux Technology Center


  reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-15 12:19 Jean-Philippe Brucker
2019-01-15 12:19 ` [PATCH v7 1/7] dt-bindings: virtio-mmio: Add IOMMU description Jean-Philippe Brucker
2019-01-15 12:19 ` [PATCH v7 2/7] dt-bindings: virtio: Add virtio-pci-iommu node Jean-Philippe Brucker
2019-01-15 12:19 ` [PATCH v7 3/7] of: Allow the iommu-map property to omit untranslated devices Jean-Philippe Brucker
2019-01-15 12:19 ` [PATCH v7 4/7] PCI: OF: Initialize dev->fwnode appropriately Jean-Philippe Brucker
2019-01-15 12:19 ` [PATCH v7 5/7] iommu: Add virtio-iommu driver Jean-Philippe Brucker
2019-01-15 12:19 ` [PATCH v7 6/7] iommu/virtio: Add probe request Jean-Philippe Brucker
2019-01-15 12:19 ` [PATCH v7 7/7] iommu/virtio: Add event queue Jean-Philippe Brucker
2019-01-18 15:51 ` [PATCH v7 0/7] Add virtio-iommu driver Michael S. Tsirkin
2019-01-21 11:29   ` Jean-Philippe Brucker
2019-01-29 18:54     ` Michael S. Tsirkin
2019-02-21 21:57       ` Thiago Jung Bauermann
2019-01-23  8:34 ` Joerg Roedel
2019-01-24 16:03   ` Jean-Philippe Brucker
2019-02-21 22:18     ` Thiago Jung Bauermann [this message]
2019-02-22 12:18       ` Jean-Philippe Brucker
2019-02-25 13:20 ` Tomasz Nowicki
2019-05-12 16:31 ` Michael S. Tsirkin
2019-05-27  9:26   ` Joerg Roedel
2019-05-27 15:15     ` Michael S. Tsirkin
2019-05-28  9:18       ` Jean-Philippe Brucker
2019-05-12 17:15 ` Michael S. Tsirkin

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=878sy8bx9o.fsf@morokweng.localdomain \
    --to=bauerman@linux.ibm.com \
    --cc=Marc.Zyngier@arm.com \
    --cc=Mark.Rutland@arm.com \
    --cc=Robin.Murphy@arm.com \
    --cc=Will.Deacon@arm.com \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=frowand.list@gmail.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jasowang@redhat.com \
    --cc=jean-philippe.brucker@arm.com \
    --cc=joro@8bytes.org \
    --cc=kevin.tian@intel.com \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-pci@vger.kernel.org \
    --cc=mst@redhat.com \
    --cc=robh+dt@kernel.org \
    --cc=tnowicki@caviumnetworks.com \
    --cc=virtio-dev@lists.oasis-open.org \
    --cc=virtualization@lists.linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git