Re: [PATCH 05/11] RISC-V: drivers/iommu/riscv: Add sysfs interface

From: Tomasz Jeznach <tjeznach@rivosinc.com>
To: Baolu Lu <baolu.lu@linux.intel.com>
Cc: Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
	 Robin Murphy <robin.murphy@arm.com>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	 Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	 Anup Patel <apatel@ventanamicro.com>,
	Sunil V L <sunilvl@ventanamicro.com>,
	 Nick Kossifidis <mick@ics.forth.gr>,
	Sebastien Boeuf <seb@rivosinc.com>,
	iommu@lists.linux.dev,  linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org,  linux@rivosinc.com
Subject: Re: [PATCH 05/11] RISC-V: drivers/iommu/riscv: Add sysfs interface
Date: Thu, 20 Jul 2023 10:47:40 -0700	[thread overview]
Message-ID: <CAH2o1u7WO=Kpki1qMGoq3uJ2vVqhCvffvX0hLGc7o9op2vTCbw@mail.gmail.com> (raw)
In-Reply-To: <2556751a-c439-bb69-a102-583dd985fc5e@linux.intel.com>

On Thu, Jul 20, 2023 at 5:51 AM Baolu Lu <baolu.lu@linux.intel.com> wrote:
>
> On 2023/7/20 3:33, Tomasz Jeznach wrote:
> > +#define sysfs_dev_to_iommu(dev) \
> > +     container_of(dev_get_drvdata(dev), struct riscv_iommu_device, iommu)
> > +
> > +static ssize_t address_show(struct device *dev,
> > +                         struct device_attribute *attr, char *buf)
> > +{
> > +     struct riscv_iommu_device *iommu = sysfs_dev_to_iommu(dev);
> > +     return sprintf(buf, "%llx\n", iommu->reg_phys);
>
> Use sysfs_emit() please.
>

ack. Thanks, will update.

> > +}
> > +
> > +static DEVICE_ATTR_RO(address);
> > +
> > +#define ATTR_RD_REG32(name, offset)                                  \
> > +     ssize_t reg_ ## name ## _show(struct device *dev,               \
> > +                     struct device_attribute *attr, char *buf)       \
> > +{                                                                    \
> > +     struct riscv_iommu_device *iommu = sysfs_dev_to_iommu(dev);     \
> > +     return sprintf(buf, "0x%x\n",                                   \
> > +                     riscv_iommu_readl(iommu, offset));              \
> > +}
> > +
> > +#define ATTR_RD_REG64(name, offset)                                  \
> > +     ssize_t reg_ ## name ## _show(struct device *dev,               \
> > +                     struct device_attribute *attr, char *buf)       \
> > +{                                                                    \
> > +     struct riscv_iommu_device *iommu = sysfs_dev_to_iommu(dev);     \
> > +     return sprintf(buf, "0x%llx\n",                                 \
> > +                     riscv_iommu_readq(iommu, offset));              \
> > +}
> > +
> > +#define ATTR_WR_REG32(name, offset)                                  \
> > +     ssize_t reg_ ## name ## _store(struct device *dev,              \
> > +                     struct device_attribute *attr,                  \
> > +                     const char *buf, size_t len)                    \
> > +{                                                                    \
> > +     struct riscv_iommu_device *iommu = sysfs_dev_to_iommu(dev);     \
> > +     unsigned long val;                                              \
> > +     int ret;                                                        \
> > +     ret = kstrtoul(buf, 0, &val);                                   \
> > +     if (ret)                                                        \
> > +             return ret;                                             \
> > +     riscv_iommu_writel(iommu, offset, val);                         \
> > +     return len;                                                     \
> > +}
> > +
> > +#define ATTR_WR_REG64(name, offset)                                  \
> > +     ssize_t reg_ ## name ## _store(struct device *dev,              \
> > +                     struct device_attribute *attr,                  \
> > +                     const char *buf, size_t len)                    \
> > +{                                                                    \
> > +     struct riscv_iommu_device *iommu = sysfs_dev_to_iommu(dev);     \
> > +     unsigned long long val;                                         \
> > +     int ret;                                                        \
> > +     ret = kstrtoull(buf, 0, &val);                                  \
> > +     if (ret)                                                        \
> > +             return ret;                                             \
> > +     riscv_iommu_writeq(iommu, offset, val);                         \
> > +     return len;                                                     \
> > +}
>
> So this allows users to change the registers through sysfs? How does
> it synchronize with the iommu driver?
>

The only writable registers are for debug interface and performance
monitoring counters, without any synchronization requirements between
user / driver.  In follow up patch series performance counters will be
also removed from sysfs, replaced by integration with perfmon
subsystem. The only remaining will be a debug access, providing user
access to address translation, in short it provides an interface to
query SPA based on provided IOVA/RID/PASID. There was a discussion in
RVI IOMMU TG forum if it's acceptable to expose such an interface to
the privileged user, and the conclusion was that it's very likely not
exposing more info than privileged users already are able to acquire
by looking at in-memory data structures.

Read-only registers are to provide debug access to track queue
head/tail pointers and interrupt states.

> Best regards,
> baolu

regards,
- Tomasz