On Wed, 07 Sep 2022 12:35:54 +0100,
"Radovanovic, Aleksandar" [off-list ref] wrote:

[AMD Official Use Only - General]

quoted

-----Original Message-----
From: Marc Zyngier <maz@kernel.org>
Sent: 07 September 2022 12:17
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Gupta, Nipun <Nipun.Gupta@amd.com>; robh+dt@kernel.org;
krzysztof.kozlowski+dt@linaro.org; gregkh@linuxfoundation.org;
rafael@kernel.org; eric.auger@redhat.com; alex.williamson@redhat.com;
cohuck@redhat.com; Gupta, Puneet (DCG-ENG)
[off-list ref]; song.bao.hua@hisilicon.com;
mchehab+huawei@kernel.org; f.fainelli@gmail.com;
jeffrey.l.hugo@gmail.com; saravanak@google.com;
Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com;
robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
masahiroy@kernel.org; ndesaulniers@google.com; linux-arm-
kernel@lists.infradead.org; linux-kbuild@vger.kernel.org; linux-
kernel@vger.kernel.org; devicetree@vger.kernel.org; kvm@vger.kernel.org;
okaya@kernel.org; Anand, Harpreet [off-list ref]; Agarwal,
Nikhil [off-list ref]; Simek, Michal [off-list ref];
Radovanovic, Aleksandar [off-list ref]; git (AMD-
Xilinx) [off-list ref]
Subject: Re: [RFC PATCH v3 4/7] bus/cdx: add cdx-MSI domain with gic-its
domain as parent

[CAUTION: External Email]

On Tue, 06 Sep 2022 18:19:06 +0100,
Jason Gunthorpe [off-list ref] wrote:

quoted

On Tue, Sep 06, 2022 at 07:17:58PM +0530, Nipun Gupta wrote:

quoted

+static void cdx_msi_write_msg(struct irq_data *irq_data,
+                         struct msi_msg *msg) {
+   /*
+    * Do nothing as CDX devices have these pre-populated
+    * in the hardware itself.
+    */
+}

Huh?

There is no way it can be pre-populated, the addr/data pair,
especially on ARM, is completely under SW control.

There is nothing in the GIC spec that says that.

quoted

There is some commonly used IOVA base in Linux for the ITS page, but
no HW should hardwire that.

That's not strictly true. It really depends on how this block is integrated, and
there is a number of existing blocks that know *in HW* how to signal an LPI.

See, as the canonical example, how the mbigen driver doesn't need to know
about the address of GITS_TRANSLATER.

Yes, this messes with translation (the access is downstream of the
SMMU) if you relied on it to have some isolation, and it has a "black hole"
effect as nobody can have an IOVA that overlaps with the physical address of
the GITS_TRANSLATER register.

But is it illegal as per the architecture? No. It's just stupid.

        M.

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

To give some context, CDX devices are specific to embedded ARM CPUs
on the FPGA and a lot of the CDX hardware core is under the control
of the system firmware, not the application CPUs.

That being said, the MSI address is always going to be the GIC
GITS_TRANSLATER, which is known to the system firmware, as it is
fixed per FPGA platform. At present, we do not allow the application
CPU OS to change this - I believe this is for security reasons, but
this may or may not be a good idea in general.

I'm sure that being downstream of the SMMU is a security feature...

As Marc mentions, CDX
MSI writes are downstream of the SMMU and, if SMMU does not provide
identity mapping for GITS_TRANSLATER, then we have a problem and may
need to allow the OS to write the address part. However, even if we
did, the CDX hardware is limited in that it can only take one
GITS_TRANSLATER register target address per system, not per CDX
device, nor per MSI vector.

If the MSI generation is downstream of the SMMU, why should the SMMU
provide a 1:1 mapping for GITS_TRANSLATER? I don't think it should
provide a mapping at all in this case. But it looks like I don't
really understand how these things are placed relative to each
other... :-/

As for the data part (EventID in GIC parlance), this is always going
to be the CDX device-relative vector number - I believe this can't
be changed, it is a hardware limitation (but I need to
double-check). That should be OK, though, as I believe this is
exactly what Linux would write anyway, as each CDX device should be
in its own IRQ domain (i.e. have its own ITS device table).

But that's really the worse part. You have hardcoded what is the
*current* Linux behaviour. Things change. And baking SW behaviour into
a piece of HW looks incredibly shortsighted...

The best I can propose is to pass the addr/data info to firmware
here, which will then decide what to do with it. At least, it can
assert that the values are what the hardware expects and fail loudly
if not, rather than having a silently misconfigured system.

And then what? It means that by agreeing to support this bus, we are
agreeing to *never* change the EventID allocation scheme.

I'm not signing up to this.

	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