All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alyssa Rosenzweig <alyssa@collabora.com>
To: Sven Peter <sven@svenpeter.dev>
Cc: Will Deacon <will@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Joerg Roedel <joro@8bytes.org>, Arnd Bergmann <arnd@kernel.org>,
	devicetree@vger.kernel.org, Hector Martin <marcan@marcan.st>,
	linux-kernel@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
	Mohamed Mediouni <mohamed.mediouni@caramail.com>,
	Stan Skowronek <stan@corellium.com>,
	linux-arm-kernel@lists.infradead.org,
	Mark Kettenis <mark.kettenis@xs4all.nl>,
	iommu@lists.linux-foundation.org,
	Alexander Graf <graf@amazon.com>,
	Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
	Rob Herring <robh+dt@kernel.org>,
	r.czerwinski@pengutronix.de
Subject: Re: [PATCH v4 3/3] iommu: dart: Add DART iommu driver
Date: Wed, 30 Jun 2021 09:49:07 -0400	[thread overview]
Message-ID: <YNx2U4GPoKxV3PWd@maud> (raw)
In-Reply-To: <20210627143405.77298-4-sven@svenpeter.dev>

Looks really good! Just a few minor comments. With them addressed,

	Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

> +	  Say Y here if you are using an Apple SoC with a DART IOMMU.

Nit: Do we need to spell out "with a DART IOMMU"? Don't all the apple
socs need DART?

> +/*
> + * This structure is used to identify a single stream attached to a domain.
> + * It's used as a list inside that domain to be able to attach multiple
> + * streams to a single domain. Since multiple devices can use a single stream
> + * it additionally keeps track of how many devices are represented by this
> + * stream. Once that number reaches zero it is detached from the IOMMU domain
> + * and all translations from this stream are disabled.
> + *
> + * @dart: DART instance to which this stream belongs
> + * @sid: stream id within the DART instance
> + * @num_devices: count of devices attached to this stream
> + * @stream_head: list head for the next stream
> + */
> +struct apple_dart_stream {
> +	struct apple_dart *dart;
> +	u32 sid;
> +
> +	u32 num_devices;
> +
> +	struct list_head stream_head;
> +};

It wasn't obvious to me why we can get away without reference counting.
Looking ahead it looks like we assert locks in each case. Maybe add
that to the comment?

```
> +static void apple_dart_hw_set_ttbr(struct apple_dart *dart, u16 sid, u16 idx,
> +				   phys_addr_t paddr)
> +{
> +	writel(DART_TTBR_VALID | (paddr >> DART_TTBR_SHIFT),
> +	       dart->regs + DART_TTBR(sid, idx));
> +}
```

Should we be checking alignment here? Something like

    BUG_ON(paddr & ((1 << DART_TTBR_SHIFT) - 1));

WARNING: multiple messages have this Message-ID (diff)
From: Alyssa Rosenzweig <alyssa@collabora.com>
To: Sven Peter <sven@svenpeter.dev>
Cc: devicetree@vger.kernel.org, r.czerwinski@pengutronix.de,
	Arnd Bergmann <arnd@kernel.org>, Will Deacon <will@kernel.org>,
	Hector Martin <marcan@marcan.st>,
	linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org,
	Rob Herring <robh+dt@kernel.org>,
	Alexander Graf <graf@amazon.com>,
	Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
	Marc Zyngier <maz@kernel.org>,
	Mohamed Mediouni <mohamed.mediouni@caramail.com>,
	Mark Kettenis <mark.kettenis@xs4all.nl>,
	Robin Murphy <robin.murphy@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	Stan Skowronek <stan@corellium.com>
Subject: Re: [PATCH v4 3/3] iommu: dart: Add DART iommu driver
Date: Wed, 30 Jun 2021 09:49:07 -0400	[thread overview]
Message-ID: <YNx2U4GPoKxV3PWd@maud> (raw)
In-Reply-To: <20210627143405.77298-4-sven@svenpeter.dev>

Looks really good! Just a few minor comments. With them addressed,

	Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

> +	  Say Y here if you are using an Apple SoC with a DART IOMMU.

Nit: Do we need to spell out "with a DART IOMMU"? Don't all the apple
socs need DART?

> +/*
> + * This structure is used to identify a single stream attached to a domain.
> + * It's used as a list inside that domain to be able to attach multiple
> + * streams to a single domain. Since multiple devices can use a single stream
> + * it additionally keeps track of how many devices are represented by this
> + * stream. Once that number reaches zero it is detached from the IOMMU domain
> + * and all translations from this stream are disabled.
> + *
> + * @dart: DART instance to which this stream belongs
> + * @sid: stream id within the DART instance
> + * @num_devices: count of devices attached to this stream
> + * @stream_head: list head for the next stream
> + */
> +struct apple_dart_stream {
> +	struct apple_dart *dart;
> +	u32 sid;
> +
> +	u32 num_devices;
> +
> +	struct list_head stream_head;
> +};

It wasn't obvious to me why we can get away without reference counting.
Looking ahead it looks like we assert locks in each case. Maybe add
that to the comment?

```
> +static void apple_dart_hw_set_ttbr(struct apple_dart *dart, u16 sid, u16 idx,
> +				   phys_addr_t paddr)
> +{
> +	writel(DART_TTBR_VALID | (paddr >> DART_TTBR_SHIFT),
> +	       dart->regs + DART_TTBR(sid, idx));
> +}
```

Should we be checking alignment here? Something like

    BUG_ON(paddr & ((1 << DART_TTBR_SHIFT) - 1));
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

WARNING: multiple messages have this Message-ID (diff)
From: Alyssa Rosenzweig <alyssa@collabora.com>
To: Sven Peter <sven@svenpeter.dev>
Cc: Will Deacon <will@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Joerg Roedel <joro@8bytes.org>, Arnd Bergmann <arnd@kernel.org>,
	devicetree@vger.kernel.org, Hector Martin <marcan@marcan.st>,
	linux-kernel@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
	Mohamed Mediouni <mohamed.mediouni@caramail.com>,
	Stan Skowronek <stan@corellium.com>,
	linux-arm-kernel@lists.infradead.org,
	Mark Kettenis <mark.kettenis@xs4all.nl>,
	iommu@lists.linux-foundation.org,
	Alexander Graf <graf@amazon.com>,
	Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
	Rob Herring <robh+dt@kernel.org>,
	r.czerwinski@pengutronix.de
Subject: Re: [PATCH v4 3/3] iommu: dart: Add DART iommu driver
Date: Wed, 30 Jun 2021 09:49:07 -0400	[thread overview]
Message-ID: <YNx2U4GPoKxV3PWd@maud> (raw)
In-Reply-To: <20210627143405.77298-4-sven@svenpeter.dev>

Looks really good! Just a few minor comments. With them addressed,

	Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>

> +	  Say Y here if you are using an Apple SoC with a DART IOMMU.

Nit: Do we need to spell out "with a DART IOMMU"? Don't all the apple
socs need DART?

> +/*
> + * This structure is used to identify a single stream attached to a domain.
> + * It's used as a list inside that domain to be able to attach multiple
> + * streams to a single domain. Since multiple devices can use a single stream
> + * it additionally keeps track of how many devices are represented by this
> + * stream. Once that number reaches zero it is detached from the IOMMU domain
> + * and all translations from this stream are disabled.
> + *
> + * @dart: DART instance to which this stream belongs
> + * @sid: stream id within the DART instance
> + * @num_devices: count of devices attached to this stream
> + * @stream_head: list head for the next stream
> + */
> +struct apple_dart_stream {
> +	struct apple_dart *dart;
> +	u32 sid;
> +
> +	u32 num_devices;
> +
> +	struct list_head stream_head;
> +};

It wasn't obvious to me why we can get away without reference counting.
Looking ahead it looks like we assert locks in each case. Maybe add
that to the comment?

```
> +static void apple_dart_hw_set_ttbr(struct apple_dart *dart, u16 sid, u16 idx,
> +				   phys_addr_t paddr)
> +{
> +	writel(DART_TTBR_VALID | (paddr >> DART_TTBR_SHIFT),
> +	       dart->regs + DART_TTBR(sid, idx));
> +}
```

Should we be checking alignment here? Something like

    BUG_ON(paddr & ((1 << DART_TTBR_SHIFT) - 1));

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-06-30 13:53 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-27 14:34 [PATCH v4 0/3] Apple M1 DART IOMMU driver Sven Peter
2021-06-27 14:34 ` Sven Peter
2021-06-27 14:34 ` Sven Peter via iommu
2021-06-27 14:34 ` [PATCH v4 1/3] iommu: io-pgtable: add DART pagetable format Sven Peter
2021-06-27 14:34   ` Sven Peter
2021-06-27 14:34   ` Sven Peter via iommu
2021-06-28 10:54   ` Alexander Graf
2021-06-28 10:54     ` Alexander Graf
2021-06-28 10:54     ` Alexander Graf via iommu
2021-06-29  7:37     ` Sven Peter
2021-06-29  7:37       ` Sven Peter
2021-06-29  7:37       ` Sven Peter via iommu
2021-06-29 12:04       ` Alexander Graf
2021-06-29 12:04         ` Alexander Graf
2021-06-29 12:04         ` Alexander Graf via iommu
2021-06-30 13:53   ` Alyssa Rosenzweig
2021-06-30 13:53     ` Alyssa Rosenzweig
2021-06-30 13:53     ` Alyssa Rosenzweig
2021-07-13 19:17   ` Robin Murphy
2021-07-13 19:17     ` Robin Murphy
2021-07-13 19:17     ` Robin Murphy
2021-07-14 17:39     ` Sven Peter
2021-07-14 17:39       ` Sven Peter
2021-07-14 17:39       ` Sven Peter via iommu
2021-06-27 14:34 ` [PATCH v4 2/3] dt-bindings: iommu: add DART iommu bindings Sven Peter
2021-06-27 14:34   ` Sven Peter
2021-06-27 14:34   ` Sven Peter via iommu
2021-06-30 13:54   ` Alyssa Rosenzweig
2021-06-30 13:54     ` Alyssa Rosenzweig
2021-06-30 13:54     ` Alyssa Rosenzweig
2021-06-27 14:34 ` [PATCH v4 3/3] iommu: dart: Add DART iommu driver Sven Peter
2021-06-27 14:34   ` Sven Peter
2021-06-27 14:34   ` Sven Peter via iommu
2021-06-30 13:49   ` Alyssa Rosenzweig [this message]
2021-06-30 13:49     ` Alyssa Rosenzweig
2021-06-30 13:49     ` Alyssa Rosenzweig
2021-07-12 11:02     ` Sven Peter
2021-07-12 11:02       ` Sven Peter
2021-07-12 11:02       ` Sven Peter via iommu
2021-07-12 13:53       ` Alyssa Rosenzweig
2021-07-12 13:53         ` Alyssa Rosenzweig
2021-07-12 13:53         ` Alyssa Rosenzweig
2021-07-13 23:23   ` Robin Murphy
2021-07-13 23:23     ` Robin Murphy
2021-07-13 23:23     ` Robin Murphy
2021-07-15 16:41     ` Sven Peter
2021-07-15 16:41       ` Sven Peter
2021-07-15 16:41       ` Sven Peter via iommu
2021-07-19 18:15       ` Robin Murphy
2021-07-19 18:15         ` Robin Murphy
2021-07-19 18:15         ` Robin Murphy
2021-07-25 12:40         ` Sven Peter
2021-07-25 12:40           ` Sven Peter
2021-07-25 12:40           ` Sven Peter via iommu
2021-07-26 13:19           ` Alyssa Rosenzweig
2021-07-26 13:19             ` Alyssa Rosenzweig
2021-07-26 13:19             ` Alyssa Rosenzweig
2021-07-14 18:19 ` [PATCH v4 0/3] Apple M1 DART IOMMU driver Robin Murphy
2021-07-14 18:19   ` Robin Murphy
2021-07-14 18:19   ` Robin Murphy
2021-07-14 20:51   ` Arnd Bergmann
2021-07-14 20:51     ` Arnd Bergmann
2021-07-14 20:51     ` Arnd Bergmann
2021-07-15  6:52     ` Joerg Roedel
2021-07-15  6:52       ` Joerg Roedel
2021-07-15  6:52       ` Joerg Roedel
2021-07-16  6:24   ` Christoph Hellwig
2021-07-16  6:24     ` Christoph Hellwig
2021-07-16  6:24     ` Christoph Hellwig
2021-07-16 15:32     ` Robin Murphy
2021-07-16 15:32       ` Robin Murphy
2021-07-16 15:32       ` Robin Murphy

Reply instructions:

You may reply publicly 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=YNx2U4GPoKxV3PWd@maud \
    --to=alyssa@collabora.com \
    --cc=alyssa.rosenzweig@collabora.com \
    --cc=arnd@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=graf@amazon.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=joro@8bytes.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marcan@marcan.st \
    --cc=mark.kettenis@xs4all.nl \
    --cc=maz@kernel.org \
    --cc=mohamed.mediouni@caramail.com \
    --cc=r.czerwinski@pengutronix.de \
    --cc=robh+dt@kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=stan@corellium.com \
    --cc=sven@svenpeter.dev \
    --cc=will@kernel.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.