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

[<dan.j.williams@intel.com>]

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;
 };
 
...i.e. a container_of() relationship, but that makes pci_ops mandatory.
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.

> I struggle to find any other _ops doing the same owner
> caching easily.

struct file_operations::owner looks like one example.

> 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"?