>
>
>
> > + 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.
>
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.
>
> > 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.
>
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
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. :)
>
> > + # 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?
>
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.
>
>
> > + """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?
>
>
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.
>
> > + 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."
>
> 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.
