Re: [PATCH v4 05/10] samples/devsec: Introduce a PCI device-security bus + endpoint sample

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

Jonathan Cameron wrote:
> 
> +CC Gerd, of off chance we can use a Redhat PCI device ID for kernel
> emulation similar to those they let Qemu use.
> 
[..]
> > > Emulating something real?  If not maybe we should get an ID from another space
> > > (or reserve this one ;)  
> > 
> > I am happy to switch to something else, but no, I do not have time to
> > chase this through PCI SIG. I do not expect this id to cause conflicts,
> > but no guarantees.
> 
> Nothing to do with the SIG - you definitely don't want to try talking them
> into giving a Vendor ID for the kernel.  That's an Intel ID so you need to find
> the owner of whatever tracker Intel uses for these.

About the same level of difficulty...

> Or maybe we can ask for one of the Redhat ones (maintained by Gerd).

In the meantime I added this to the Kconfig because I also received a
report of an AMD platform being confused about this extra PCI domain.
This protects against allyesconfig builds autoloading this thing which
should only be used with unit tests.

diff --git a/samples/Kconfig b/samples/Kconfig
index 8441593fb654..9ad822d4e808 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -327,6 +327,7 @@ source "samples/damon/Kconfig"
 
 config SAMPLE_DEVSEC
 	tristate "Build a sample TEE Security Manager with an emulated PCI endpoint"
+	depends on m
 	depends on PCI
 	depends on VIRT_DRIVERS
 	depends on PCI_DOMAINS_GENERIC || X86
@@ -339,7 +340,11 @@ config SAMPLE_DEVSEC
 	  devsec_bus and devsec_tsm, exercise device-security enumeration, PCI
 	  subsystem use ABIs, device security flows. For example, exercise IDE
 	  (link encryption) establishment and TDISP state transitions via a
-	  Device Security Manager (DSM).
+	  Device Security Manager (DSM). Note the emulation uses a device-id
+	  (vendor: 0x8086 device: 0x7075) that is *assumed* unused and some
+	  architectures may be confused by this emulated PCI domain.
+
+	  If unsure, say N
 
 endif # SAMPLES
 
> 
> > 
> > > > +			.class_revision = cpu_to_le32(0x1),
> > > > +			.pref_mem_base = cpu_to_le16(PCI_PREF_RANGE_TYPE_64),
> > > > +			.pref_mem_limit = cpu_to_le16(PCI_PREF_RANGE_TYPE_64),
> > > > +		},  
> > > 
> > >   
> 
> ...
> > > > +static int __init devsec_tsm_init(void)
> > > > +{
> > > > +	__devsec_pci_ops = &devsec_pci_ops;  
> > > 
> > > I'm not immediately grasping why this global is needed.
> > > You never check if it's set, so why not just move definition of devsec_pci_ops
> > > early enough that can be directly used everywhere.  
> > 
> > Because devsec_pci_ops is used inside the ops it declares. So either
> > forward declare all those ops, or do this pointer assigment dance. I
> > opted for the latter as it is smaller.
> Ok. I guess in emulation that's a reasonable compromise.  Maybe leave
> a comment somewhere to avoid repeat of this feedback.

I expect all the low-level tsm drivers will struggle with this, so
expect to see this pattern repeat outside of emulation.