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: Tue, 22 Nov 2022 21:49:11 +0100	[thread overview]
Message-ID: <87k03mg1fc.ffs@tglx> (raw)
In-Reply-To: <Y3wreJlD/7TsMCme@nvidia.com>

Jason,

On Mon, Nov 21 2022 at 21:52, Jason Gunthorpe wrote:
> On Mon, Nov 21, 2022 at 08:40:05PM +0100, Thomas Gleixner wrote:
>> 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.
>
> No, not allocation, just wrap in a stack variable:
>
>   struct foo_bar_domain_data arg = {.pasid = XX};
>
>   msi_domain_alloc_irq_at(..., &arg);
>
> Then there is a great big clue right in the code who is supposed to be
> consuming that opaque argument. grep the code for foo_bar_domain_data
> and you can find the receiving side

Agreed for the one or two sane people who actually will create their
data struct. The rest will just hand in a random pointer or a casted
integer, which is pretty useless for finding the counterpart.

>> 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. 
>
> I'm not saying not work, I'm asking about the style choice

Sure. The other reason why made this choice is that for many cases it
spares a callback to actually convert the pointer into real storage,
which is necessary because the data it points to is on stack.

Just copying the data into the MSI descriptor solves this nicely without
having some extra magic.

I guess we're nearing bike shed realm by now :) Let's pick one evil and
see how it works out. Coccinelle is there to help us fixing it up when
it turns out to be the wrong evil. :)

Thanks,

        tglx