Re: [PATCH resend v6 04/10] PCI/TSM: Authenticate devices via platform TSM

[Alexey Kardashevskiy <aik@amd.com>]

On 26/9/25 09:00, dan.j.williams@intel.com wrote:
> Alexey Kardashevskiy wrote:
> [..]
>>>> It is still a rather global thing. May I suggest this?
>>>
>>> I am not too keen on this.
>>>
>>> Yes, it is global, but less often used compared to @ops, and I do not
>>> want both @ops and @tsm_dev in @pci_tsm.
>>
>> Why exactly?
> 
> ...because it complicates the data structure merely for code convenience
> which is often the wrong tradeoff.
> 
> Here are the current options:
> 
> 1/ Current:
> struct pci_tsm {
>          struct pci_dev *pdev;
>          struct pci_dev *dsm_dev;
>          const struct pci_tsm_ops *ops;
> };
> 
> 2/ Alternative:
> struct pci_tsm {
>          struct pci_dev *pdev;
>          struct pci_dev *dsm_dev;
>          struct tsm_dev *tsm_dev;
> };
> 
> 2/ Proposed:
> struct pci_tsm {
>          struct pci_dev *pdev;
>          struct pci_dev *dsm_dev;
>          struct tsm_dev *tsm_dev;
>          const struct pci_tsm_ops *ops;
> };
> 
> I rank 3 last because it implies that @tsm_dev and @ops may have
> different lifetimes or otherwise may not be related to the same object
> until you read the code.
>
> I rank 2 after 1 because most of 'struct pci_tsm_ops' do not need the
> tsm_dev parameter.
> 
> Now, I would maybe go with 2 if 'struct tsm_dev' could be defined as:
> 
> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> index 376139585797..3619ffa8f8c1 100644
> --- a/include/linux/tsm.h
> +++ b/include/linux/tsm.h
> @@ -112,7 +112,7 @@ struct pci_tsm_ops;
>   struct tsm_dev {
>          struct device dev;
>          int id;
> -       const struct pci_tsm_ops *pci_ops;
> +       const struct pci_tsm_ops pci_ops;

Either variant of 2 (with "*pci_ops" or "pci_ops") is fine by me.


>   };
>   
> ...i.e. a container_of() relationship, but that makes pci_ops mandatory.

Do you need to be able to resolve tsm_dev from pci_ops pointer?


> It is already the case that TDX has as a few host-services in mind that
> may end up sharing common infrastructure at a TSM device level, so 1
> remains the preference.
> 
>>> So the options are lookup @ops
>>> from @tsm_dev or lookup @tsm_dev from @ops. Given @ops is used more
>>> often that is how I came up with the current arrangement.
>>
>> I am looking at:
>> https://github.com/AMDESE/linux-kvm/commit/9e3caf921ad6ddd6bd860ec307b986649322a618
>> and not really sure "more often" applies here.
>>
>> And do we have to check now if tsm_dev passed in probe() is the same
>> as the owner?
> 
> @ops are not passed passed back in probe so ->probe() can not verify.
> Also ->probe() sets the operations to use via pci_tsm_link_constructor()
> so at that point it does not matter how ->probe() got invoked the result
> is still the TSM driver returning the ops associated with @tsm_dev.


Any pci_tsm_ops::probe implementation can be called with any tsm_dev. So every probe() should verify that BUG_ON(tsm_dev::pci_ops::probe != itself). Although it is not possible right now. And I do not see a better way. So I do not insist really. But my head explodes at this point :)


>> I struggle to find any other _ops doing the same owner
>> caching easily.
> 
> struct file_operations::owner looks like one example.

Not exactly the same - struct module does not have back reference to file_operations. And the file_operations::owner is an ELF with some code but tsm_dev is just some memory from kmalloc(). I guess "dev" implies "module" though.

> 
>> Or merge struct pci_tsm_ops into struct tsm_dev to stop pretending
>> that pci_tsm_ops is an interface, and then we won't even need that
>> @owner. Dunno. Aneesh? :)
> 
> Not sure what you mean by "pretending that pci_tsm_ops is an interface"?

Most of these _ops are stateless (so no "owner") set of hooks, often "static" (and should be "const", and live in .ro sections, not sure if they do). And this _ops could follow the pattern. Thanks,


-- 
Alexey