<div dir="ltr"><div class="gmail_quote gmail_quote_container"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + def run_app(self, numvfs: int | None = 0) -> list[CryptodevResults]: > + """Run the cryptodev application with the given app parameters. > + > + Raises: > + SkippedTestException: If the device type is not supported on the main session. > + RemoteCommandExecutionError: If there is an error running the command. > + > + Returns: > + list[CryptodevResults]: The list of parsed results for the cryptodev application. Missing the args docstring. Speaking of which, What is the theory behind passing a vf count from the testsuites to the app? In terms of what the testsuites require, and whether that aspect should be configurable from the user side, why the default is zero, etc. </blockquote><div> The theory behind this is that some test suites in the future that use scheduling will require to use multiple VFs. I think that if this varies on a per test case basis, then it should be configurable within a test case. In regard to the default, it should be assigned to one, not zero. As the application requires at least one VF. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > diff --git a/dts/configurations/nodes.example.yaml b/dts/configurations/nodes.example.yaml > index 3f23df11ec..1fc0a38bf5 100644 > --- a/dts/configurations/nodes.example.yaml > +++ b/dts/configurations/nodes.example.yaml > @@ -20,6 +20,12 @@ > hugepages_2mb: # optional; if removed, will use system hugepage configuration > number_of: 256 > force_first_numa: false > + # cryptodevs: # optional; add each physical bdf as its own element in the list I would rephrase to clarify: "Uncomment to run cryptodev tests; add physical cryptodev devices by their BDF" I also think that we are at a place now where we have a number of optional fields in the config files that can be commented or uncommented, and there may be value in explicitly calling this out, perhaps with a one liner above all of the YAML fields. I think you are going in the right direction with your comment, but then we have other fields which are optional that don't have the same explanation, or it's phrased differently, resulting in documentation which is inconsistent and potentially confusing to new people who just want to fill out their config files. What do you think agree/disagree? If you agree can you come up with something? Thanks. </blockquote><div> I think this is a reasonable request, maybe we can have a disclaimer at the top of the file that states which variables are optional with a short description of each one and some optional tag each time it is used within the file. For example, # Optional Arguments: # cryptodevs: uncomment to run cryptodev tests; add physical cryptodev devices by their BDF # crypto_driver: the device type of the DUT e.g. (crypto_qat, crypto_zuc, crypto_aesni_mb) # hugepages_2mb: optional; add each physical bdf as its own element in the list, # if removed, will use system hugepage configuration </div><div> cryptodevs: # see 'Optional Arguments' - name: cryptodev-0 pci: "ffff:ff:ff.f" os_driver_for_dpdk: vfio-pci # OS driver that DPDK will use for the cryptodev VFs os_driver: _ # This is unused but a value is necessary to run dts crypto_driver: # see 'Optional Arguments' Let me know what you think. :) </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + # os_driver: _ # This is unused but a value is necessary to run dts If there is really no point in populating the os_driver field, is it possible to remove the field and force the config parser step in the framework be tolerant to this? </blockquote><div> This is kind of a shortcut, A crypto device is identical to the already implemented port class; except that it does not require an os driver. This field cannot be made optional with this approach but its value has no effect on test runs.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + """Retrieve virtual functions from active crypto vfs. > + > + If numvfs is `None`, returns all crypto virtual functions. Otherwise, returns > + `numvfs` number of virtual functions per physical function rounded down. If a number of > + `numvfs` is less than the number of physical functions, one virtual function is returned. I am wondering about this design. So, the form of the return value is unpredictable? What do the testsuites actually require? </blockquote><div> This will be redesigned in my v4, It should return the exact number of vfs requested or raise an exception if an invalid number of vfs are called.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + def instantiate_crypto_vf_ports(self) -> None: > + """Create max number of virtual functions allowed on the SUT node. > + > + Raises: > + InternalError: If crypto virtual functions could not be created on a port. > + """ > + from framework.context import get_ctx > + > + ctx = get_ctx() > + > + for port in ctx.sut_node.cryptodevs: > + self.crypto_pf_ports.append(port) > + self.delete_crypto_vf_ports() populating crypto_pf_ports in the instantiate_crypto_vf_ports method? Possible single responsibility principle violation but not a huge deal if it cannot be cleanly populated "earlier." </blockquote><div>This follows the behavior of the 'instantiate_vf_ports' method. The idea here is that we will never want to instantiate the crypto PF ports without the VFs. For this reason I thought it was appropriate to do it in one method. </div></div></div>