Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Dan Williams <dan.j.williams@intel.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 21:18:18 +0100
Message-ID: <20210519211818.00002acf@Huawei.com> (raw)
In-Reply-To: <CAPcyv4ii3KC6MBBxJrnCUCm_JGS7ugL+JTFUu9QTBnPUhQFtfQ@mail.gmail.com>

On Wed, 19 May 2021 12:20:17 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> 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.

That would work but I thought was ruled out as an approach.
@Bjorn would this be acceptable?

> 
> > 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.

True for specific protocols (CDAT). I'm fairly sure, with IDE you can take down
the link encryption to the device, potentially (worst case?) resulting a memory
access failure and a machine reboot or corruption of persistent memory.

> 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.

The short answer is that lock requirement, in the above note, rules
out safe direct userspace use of the DOE (unless we can tell the kernel
is not going to ever use it).  Mediation must be done. Even if we safely
protect the kernel side via aborts, userspace transactions can be
interrupted in a fashion that is invisible to userspace (beyond maybe
a timeout if the userspace code is hardened against this). So there is no
legitimate use that is not fully mediated by the kernel. So ioctl
or defined per protocol interfaces are the way forwards.

Perhaps that's putting it rather strongly :)

Jonathan




  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-19 16:54 [RFC PATCH v3 0/4] PCI Data Object Exchange support + CXL CDAT Jonathan Cameron
2021-04-19 16:54 ` [RFC PATCH v3 1/4] PCI: Add vendor ID for the PCI SIG Jonathan Cameron
2021-04-19 16:54 ` [RFC PATCH v3 2/4] PCI/doe: Add Data Object Exchange support Jonathan Cameron
2021-05-06 21:59   ` Ira Weiny
2021-05-11 16:50     ` Jonathan Cameron
2021-05-13 21:20       ` Dan Williams
2021-05-14  8:47         ` Jonathan Cameron
2021-05-14 11:15           ` Lorenzo Pieralisi
2021-05-14 12:39             ` Jonathan Cameron
2021-05-14 18:37           ` Dan Williams
2021-05-17  8:40             ` Jonathan Cameron
2021-05-17  8:51               ` Greg KH
2021-05-17 17:21               ` Dan Williams
2021-05-18 10:04                 ` Jonathan Cameron
2021-05-19 14:18                   ` Dan Williams
2021-05-19 15:11                     ` Jonathan Cameron
2021-05-19 15:29                       ` Dan Williams
2021-05-19 16:20                         ` Jonathan Cameron
2021-05-19 16:33                           ` Jonathan Cameron
2021-05-19 16:53                             ` Dan Williams
2021-05-19 17:00                               ` Jonathan Cameron
2021-05-19 19:20                                 ` Dan Williams
2021-05-19 20:18                                   ` Jonathan Cameron [this message]
2021-05-19 23:51                                     ` Dan Williams
2021-05-20  0:16                                       ` Dan Williams
2021-05-20  8:22                                       ` Jonathan Cameron
2021-05-07  9:36   ` Jonathan Cameron
2021-05-07 23:10   ` Bjorn Helgaas
2021-05-12 12:44     ` Jonathan Cameron
2021-04-19 16:54 ` [RFC PATCH v3 3/4] cxl/mem: Add CDAT table reading from DOE Jonathan Cameron
2021-04-19 16:54 ` [RFC PATCH v3 4/4] cxl/mem: Add a debug parser for CDAT commands Jonathan Cameron

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=20210519211818.00002acf@Huawei.com \
    --to=jonathan.cameron@huawei.com \
    --cc=alison.schofield@intel.com \
    --cc=ben.widawsky@intel.com \
    --cc=cbrowy@avery-design.com \
    --cc=dan.j.williams@intel.com \
    --cc=f.fangjian@huawei.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=helgaas@kernel.org \
    --cc=ira.weiny@intel.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=vishal.l.verma@intel.com \
    /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-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/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-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

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


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