Re: [PATCH v2 12/14] platform/x86/intel/ifs: Add current_batch sysfs entry

[Thiago Macieira <thiago.macieira@intel.com>]

On Saturday, 12 November 2022 11:20:30 PST Borislav Petkov wrote:
> This sounds to me like there's a special order in which those batches
> should be executed?
> 
> I thought they're simply collections of test sequences which can be run
> in any order...

As Ashok replied, no, they are not ordered. But running them from first to last 
is the simplest algorithm to code.

If we did support file names, then the directory order would be also as good as 
any (unsorted).

> It is not about seeing - you simply give it the filename -
> request_firmware* does the "seeing". Either the file's there or it
> isn't.

I meant knowing which files are there. If they don't form a specific pattern, 
then it's impossible to know what they have been named. And if they do form a 
specific pattern, what's the harm in just using the sequence number? It's much 
easier for the kernel to remember a single 8-bit number than a file name.

It also allows for new techniques like deploying a single cpio or tarball with 
everything with an update to the kernel, without having userspace have to 
update to match.

> There's a reason I wrote:
> 
> "There will be no requirement on the naming - only on the filename
> length and it should be in that directory /lib/firmware/intel/ifs_0/"
> 
> Of course the driver should load only from that directory.

Thank you for that explanation. But my argument was that the application 
driving this might be deployed as a container, as part of a container-
orchestration and scheduling system, while the firmware files would already be 
pre-installed on the machine and updated with the regular firmware update 
mechanism. If the container can't see what files are there, it would have to 
resort to the guessing I mentioned above. CSPs could avoid this by bind-
mounting /lib/firmware into the container, but we'd prefer not to add yet 
another constraint.

> All that doesn't matter - if the CPU *must* wait 15 minutes between
> batches, then that should be enforced by the driver and not relied upon
> by userspace to DTRT.

If's enforced by the CPU today. How it determines when a test can be run is 
besides the point; the driver will ask it to run and the CPU will reply it 
couldn't. That information is conveyed back to userspace by changing the 
"status" back to "untested".

> This all has nothing to do with whether you give it a number or a
> filename. How you glue your testing around it together is a userspace
> issue - all the kernel driver needs to be able to do is load the
> sequence and execute it.
> 
> Echoing filenames into sysfs is no different from echoing numbers into
> it - former is simpler. If the CPU says it cannot execute the sequence
> currently, you have to think about how you retry that sequence. How you
> specify it doesn't matter.

Right, it's no different from echoing file names, but it's much simpler to 
increment a number than do a directory listing and sort the file names, so it 
can pick up from where it left off.

I mentioned this complication to explain why the userspace won't be able to 
simply echo each file name and expect things to have reached full coverage. 
Unfortunately, userspace needs to cope with the fact that there will be a 
timeout for certain generations. It's not what we'd have preferred; it's a 
hardware constraint we have to adapt to.

WIth this constraint in mind, having a single number simplifies userspace. You 
can't do it with a three-line shell script, but we're not expecting that shell 
scripts are the means to use this feature in the first place.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Cloud Software Architect - Intel DCAI Cloud Engineering