linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Niklas Schnelle <schnelle@linux.ibm.com>,
	Leon Romanovsky <leon@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rdma@vger.kernel.org, llvm@lists.linux.dev,
	Michael Guralnik <michaelgur@mellanox.com>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Will Deacon <will@kernel.org>
Subject: Re: [PATCH rdma-next 1/2] arm64/io: add memcpy_toio_64
Date: Wed, 24 Jan 2024 08:26:28 +0000	[thread overview]
Message-ID: <86ede787d7.wl-maz@kernel.org> (raw)
In-Reply-To: <20240124012723.GD1455070@nvidia.com>

On Wed, 24 Jan 2024 01:27:23 +0000,
Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> On Tue, Jan 23, 2024 at 08:38:55PM +0000, Catalin Marinas wrote:
> > (fixed Marc's email address)
> > 
> > On Wed, Jan 17, 2024 at 01:29:06PM +0000, Mark Rutland wrote:
> > > On Wed, Jan 17, 2024 at 08:36:18AM -0400, Jason Gunthorpe wrote:
> > > > On Wed, Jan 17, 2024 at 12:30:00PM +0000, Mark Rutland wrote:
> > > > > On Tue, Jan 16, 2024 at 02:51:21PM -0400, Jason Gunthorpe wrote:
> > > > > > I'm just revising this and I'm wondering if you know why ARM64 has this:
> > > > > > 
> > > > > > #define __raw_writeq __raw_writeq
> > > > > > static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)
> > > > > > {
> > > > > > 	asm volatile("str %x0, [%1]" : : "rZ" (val), "r" (addr));
> > > > > > }
> > > > > > 
> > > > > > Instead of
> > > > > > 
> > > > > > #define __raw_writeq __raw_writeq
> > > > > > static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)
> > > > > > {
> > > > > > 	asm volatile("str %x0, %1" : : "rZ" (val), "m" (*(volatile u64 *)addr));
> > > > > > }
> > > > > > 
> > > > > > ?? Like x86 has.
> > > > > 
> > > > > I believe this is for the same reason as doing so in all of our other IO
> > > > > accessors.
> > > > > 
> > > > > We've deliberately ensured that our IO accessors use a single base register
> > > > > with no offset as this is the only form that HW can represent in ESR_ELx.ISS.SRT
> > > > > when reporting a stage-2 abort, which a hypervisor may use for
> > > > > emulating IO.
> > > > 
> > > > Wow, harming bare metal performace to accommodate imperfect emulation
> > > > sounds like a horrible reason :(
> > > 
> > > Having working functionality everywhere is a very good reason. :)
> > > 
> > > > So what happens with this patch where IO is done with STP? Are you
> > > > going to tell me I can't do it because of this?
> > > 
> > > I'm not personally going to make that judgement, but it's certainly something
> > > for Catalin and Will to consider (and I've added Marc in case he has any
> > > opinion).
> > 
> > Good point, I missed this part. We definitely can't use STP in the I/O
> > accessors, we'd have a big surprise when running the same code in a
> > guest with emulated I/O.
> 
> Unfortunately there is no hard distinction in KVM/qemu for "emulated
> IO" and "VFIO MMIO". Even devices using VFIO can get funneled down the
> emulated path for legitimate reasons.
> 
> Again, userspace is already widely deployed using complex IO
> accessors. ST4 has been out there for years and at this moment this
> patch with STP is already being deployed in production environments.

Then you will get to keep the pieces. Good luck.

> Even if you refuse to take STP to mainline it *will* be running in VMs
> under ARM hypervisors.

A hypervisor can't do anything with it. If you cared to read the
architecture, you'd know by now. So your VM will be either dead, or
dog slow, depending on your hypervisor. In any case, I'm sure it will
reflect positively on your favourite software.

> What exactly do you think should be done about that?

Well, you could use KVM_CAP_ARM_NISV_TO_USER in userspace and see
everything slow down. Your call.

> I thought the guiding mantra here was that any time KVM does not
> perfectly emulate bare metal it is a bug. "We can't assume all VMs are
> Linux!". Indeed we recently had some long and *very* theoretical
> discussions about possible incompatibilties due to kvm changes in the
> memory attributes thread.
>
> But here it seems to be just shrugging off something so catastrophic
> as performance IO accessors *that are widely deployed already* don't
> work reliably in VMs!?!?
> 
> "Oh well, don't use them"!?

Exactly.

You can also take this to the ARM architects and get them to update
the architecture to mandate full syndrome information for all
load/store instructions, and you'll get something useful in 2034.
Maybe.

Or you can stop whining and try to get better performance out of what
we have today.

> Damn I hope it crashes the VM and doesn't corrupt the MMIO. I just
> debugged a x86 KVM issue with it corrupting VFIO MMIO and that was a
> total nightmare to find.
> 
> > If eight STRs without other operations interleaved give us the
> > write-combining on most CPUs (with Normal NC), we should go with this
> > instead of STP.
> 
> __iowrite64_copy() is a performance IO accessor, we should not degrade
> it because buggy hypervisors might exist that have a problem with STP
> or other instructions. :( :(
> 
> Anyhow, I know nothing about whatever this issue is - Mark said:
> 
>  > FWIW, IIUC the immediate-offset forms *without* writeback can still
>  > be reported usefully in ESR_ELx,
> 
> Which excludes the post/pre increment forms - but does STP and ST4
> also have some kind of problem because the emulation path can't know
> about wider than a 64 bit access?
> 
> What is the plan for ST64B? Don't get to use that either?

ST64 has full syndrome information, making it possible to emulate.

In any case, there is no magic there. Everything is documented, and
has been for the past... 15 years?

	M.

-- 
Without deviation from the norm, progress is not possible.

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

  reply	other threads:[~2024-01-24  8:27 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-23 19:04 [PATCH rdma-next 0/2] Add and use memcpy_toio_64() Leon Romanovsky
2023-11-23 19:04 ` [PATCH rdma-next 1/2] arm64/io: add memcpy_toio_64 Leon Romanovsky
2023-11-24 10:16   ` Mark Rutland
2023-11-24 12:23     ` Jason Gunthorpe
2023-11-27 12:42       ` Catalin Marinas
2023-11-27 13:45         ` Jason Gunthorpe
2023-12-04 17:31           ` Catalin Marinas
2023-12-04 18:23             ` Jason Gunthorpe
2023-12-05 17:21               ` Catalin Marinas
2023-12-05 17:51                 ` Jason Gunthorpe
2023-12-05 19:34                   ` Catalin Marinas
2023-12-05 19:51                     ` Jason Gunthorpe
2023-12-06 11:09                       ` Catalin Marinas
2023-12-06 12:59                         ` Jason Gunthorpe
2024-01-16 18:51                           ` Jason Gunthorpe
2024-01-17 12:30                             ` Mark Rutland
2024-01-17 12:36                               ` Jason Gunthorpe
2024-01-17 12:41                                 ` Jason Gunthorpe
2024-01-17 13:29                                 ` Mark Rutland
2024-01-23 20:38                                   ` Catalin Marinas
2024-01-24  1:27                                     ` Jason Gunthorpe
2024-01-24  8:26                                       ` Marc Zyngier [this message]
2024-01-24 13:06                                         ` Jason Gunthorpe
2024-01-24 13:32                                           ` Marc Zyngier
2024-01-24 15:52                                             ` Jason Gunthorpe
2024-01-24 17:54                                               ` Catalin Marinas
2024-01-25  1:29                                                 ` Jason Gunthorpe
2024-01-26 16:15                                                   ` Catalin Marinas
2024-01-26 17:09                                                     ` Jason Gunthorpe
2024-01-24 11:38                                     ` Mark Rutland
2024-01-24 12:40                                       ` Catalin Marinas
2024-01-24 13:27                                         ` Jason Gunthorpe
2024-01-24 17:22                                           ` Catalin Marinas
2024-01-24 19:26                                             ` Jason Gunthorpe
2024-01-25 17:43                                               ` Jason Gunthorpe
2024-01-26 14:56                                                 ` Catalin Marinas
2024-01-26 15:24                                                   ` Jason Gunthorpe
2024-01-17 14:07                               ` Mark Rutland
2024-01-17 15:28                                 ` Jason Gunthorpe
2024-01-17 16:05                                   ` Will Deacon
2024-01-18 16:18                                     ` Jason Gunthorpe
2024-01-24 11:31                                       ` Mark Rutland
2023-11-24 12:58   ` Robin Murphy
2023-11-24 13:45     ` Jason Gunthorpe
2023-11-24 15:32       ` Robin Murphy
2023-11-24 14:10   ` Niklas Schnelle
2023-11-24 14:20     ` Jason Gunthorpe
2023-11-24 14:48       ` Niklas Schnelle
2023-11-24 14:53         ` Niklas Schnelle
2023-11-24 14:55         ` Jason Gunthorpe
2023-11-24 15:59           ` Niklas Schnelle
2023-11-24 16:06             ` Jason Gunthorpe
2023-11-27 17:43               ` Niklas Schnelle
2023-11-27 17:51                 ` Jason Gunthorpe
2023-11-28 16:28                   ` Niklas Schnelle
2024-01-16 17:33                     ` Jason Gunthorpe
2024-01-17 13:20                       ` Niklas Schnelle
2024-01-17 13:26                         ` Jason Gunthorpe
2024-01-17 17:55                           ` Jason Gunthorpe
2024-01-18 13:46                             ` Niklas Schnelle
2024-01-18 14:00                               ` Jason Gunthorpe
2024-01-18 15:59                                 ` Niklas Schnelle
2024-01-18 16:21                                   ` Jason Gunthorpe
2024-01-18 16:25                                     ` Niklas Schnelle
2024-01-19 11:52                                       ` Niklas Schnelle
2024-02-16 12:09                                   ` Niklas Schnelle
2024-02-16 12:39                                     ` Jason Gunthorpe
2023-11-23 19:04 ` [PATCH rdma-next 2/2] IB/mlx5: Use memcpy_toio_64() for write combining stores Leon Romanovsky

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=86ede787d7.wl-maz@kernel.org \
    --to=maz@kernel.org \
    --cc=arnd@arndb.de \
    --cc=catalin.marinas@arm.com \
    --cc=jgg@nvidia.com \
    --cc=leon@kernel.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=llvm@lists.linux.dev \
    --cc=mark.rutland@arm.com \
    --cc=michaelgur@mellanox.com \
    --cc=nathan@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=schnelle@linux.ibm.com \
    --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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).