Re: [patch 19/33] genirq/msi: Provide msi_desc::msi_data

From: Thomas Gleixner <tglx@linutronix.de>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
	x86@kernel.org, Joerg Roedel <joro@8bytes.org>,
	Will Deacon <will@kernel.org>,
	linux-pci@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Marc Zyngier <maz@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Dave Jiang <dave.jiang@intel.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	Kevin Tian <kevin.tian@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Logan Gunthorpe <logang@deltatee.com>,
	Ashok Raj <ashok.raj@intel.com>, Jon Mason <jdmason@kudzu.us>,
	Allen Hubbe <allenbh@gmail.com>,
	"Ahmed S. Darwish" <darwi@linutronix.de>,
	Reinette Chatre <reinette.chatre@intel.com>
Subject: Re: [patch 19/33] genirq/msi: Provide msi_desc::msi_data
Date: Mon, 21 Nov 2022 20:40:05 +0100	[thread overview]
Message-ID: <87bkp0hzai.ffs@tglx> (raw)
In-Reply-To: <Y3uzVJ/7ecL8DBK7@nvidia.com>

On Mon, Nov 21 2022 at 13:20, Jason Gunthorpe wrote:
> On Fri, Nov 18, 2022 at 11:08:55PM +0100, Thomas Gleixner wrote:
>> Sure I could make both cookies plain u64, but I hate these forced type
>> casts and the above is simple to handle and understand.
>
> I guess, they aren't what I think of as cookies, so I wouldn't make
> them u64 in the first place.
>
> The argument to msi_domain_alloc_irq_at() ideally wants to be a
> per-domain-type struct so we can folow it around more cleanly. This is
> C so we have to type erase it as a void * through the core code, but
> OK.

When looking at the wire to MSI abomination and also PASID there is no
real per domain struct. It's plain integer information and I hate to
store it in a pointer. Especially as the pointer width on 32bit is not
necessarily sufficient.

Allocating 8 bytes and tracking them to be freed would be an horrible
idea.

> The second one is typically called "driver private data" in device
> driver subsystems that can't use container_of for some reason - just a
> chunk of data the driver can associate with a core owned struct.
>
> The usual pattern for driver private data is for the core to provide
> some kind of accessor void *get_priv() (think dev_get_drvdata()) or
> whatever.
>
> But I do understand your point about keeping the drivers away from
> things. Maybe some other pattern is better in this case.

At least from the two examples I have (IDXD and wire2MSI) the per
instance union works perfectly fine and I can't see a reason why
e.g. for your usecase

     cookie = { .ptr = myqueue };

would not work. The meaning of the cookie is domain implementation
defined and only the actual MSI domain and the related users know
whether its a value or a pointer and what to do with this information. I
named it cookie because from the core MSI code's view it's completely
opaque and aside of the fact that the allocation function copies the
cookie into msi_desc, the core does not care at all about it. Same for
the domain one which is solely handled by the domain setup code and is
e.g. used to enable the irq chip callbacks to do what they need to do.

Thanks,

        tglx