Re: [RFC PATCH v3 4/7] bus/cdx: add cdx-MSI domain with gic-its domain as parent

[Marc Zyngier <maz@kernel.org>]

On Wed, 07 Sep 2022 12:35:54 +0100,
"Radovanovic, Aleksandar" <aleksandar.radovanovic@amd.com> wrote:
> 
> [AMD Official Use Only - General]
> 
> 
> 
> > -----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)
> > <puneet.gupta@amd.com>; 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 <harpreet.anand@amd.com>; Agarwal,
> > Nikhil <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>;
> > Radovanovic, Aleksandar <aleksandar.radovanovic@amd.com>; git (AMD-
> > Xilinx) <git@amd.com>
> > 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 <jgg@nvidia.com> wrote:
> > >
> > > On Tue, Sep 06, 2022 at 07:17:58PM +0530, Nipun Gupta wrote:
> > >
> > > > +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.
> > 
> > > 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