Re: [RFC PATCH v3 2/4] PCI/doe: Add Data Object Exchange support

From: Dan Williams <dan.j.williams@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Ira Weiny <ira.weiny@intel.com>,
	linux-cxl@vger.kernel.org, Linux PCI <linux-pci@vger.kernel.org>,
	Bjorn Helgaas <helgaas@kernel.org>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Ben Widawsky <ben.widawsky@intel.com>,
	Chris Browy <cbrowy@avery-design.com>,
	Linux ACPI <linux-acpi@vger.kernel.org>,
	"Schofield, Alison" <alison.schofield@intel.com>,
	Vishal L Verma <vishal.l.verma@intel.com>,
	Linuxarm <linuxarm@huawei.com>, Fangjian <f.fangjian@huawei.com>,
	Greg KH <gregkh@linuxfoundation.org>
Subject: Re: [RFC PATCH v3 2/4] PCI/doe: Add Data Object Exchange support
Date: Wed, 19 May 2021 12:20:17 -0700	[thread overview]
Message-ID: <CAPcyv4ii3KC6MBBxJrnCUCm_JGS7ugL+JTFUu9QTBnPUhQFtfQ@mail.gmail.com> (raw)
In-Reply-To: <20210519180057.00002ac3@Huawei.com>

On Wed, May 19, 2021 at 10:03 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
[..]
> > > "The DOE Busy bit can be used to indicate that the DOE responder is
> > >  temporarily unable to accept a data object. It is necessary for a
> > >  DOE requester to ensure that individual data object transfers are
> > >  completed, and that a request/response contract is completed, for
> > >  example using a mutex mechanism to block other conflicting traffic
> > >  for cases where such conflicts are possible."
> >
> > I read that as the specification mandating my proposal to disallow
> > multi-initiator access. My only mistake was making the exclusion apply
> > to reads and not limiting it to the minimum of config write exclusion.
>
> Key thing is even that isn't enough.   The mutex isn't about stopping
> temporary access, it's about ensuring "request/response contract is completed".
> So you would need userspace to be able to take a lock to stop the kernel
> from using the DOE whilst it completes it's request/response pair and
> userspace to guarantee it doesn't do anything stupid.

A userspace lockout of the kernel is not needed if userspace is
outright forbidden from corrupting the kernel's state machine. I.e.
kernel enforced full disable of user initiated config-write to DOE
registers, not the ephemeral pci_cfg_access_lock() proposal.

> Easiest way to do that is provide proper interfaces that allows the
> kernel to fully mediate the access + don't support direct userspace access
> for normal operation. (treat it the same as an other config space write)

Again, it's the parenthetical at issue. I struggle to see this as just
another errant / unwanted config-write when there is legitimate reason
for userspace to expect that touching the DOE is not destructive to
device operation as opposed to writes to other critical registers.
Where the kernel's legitimate-access and userspace's legitimate-access
to a resource collide, the kernel provides a mediation interface that
precludes conflicts. Otherwise, I don't understand why the kernel is
going through the trouble of /dev/mem and pci-mmap restrictions if it
is not supposed to be concerned about userspace corrupting driver
state.