Message-ID: <e656fc58-a910-44b3-adb2-56f952dd740e@t-8ch.de>
Date:   Tue, 11 Apr 2023 18:16:44 +0200 (GMT+02:00)
From:   Thomas Weißschuh  <thomas@...ch.de>
To:     Jorge Lopez <jorgealtxwork@...il.com>
Cc:     hdegoede@...hat.com, platform-driver-x86@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7] Introduction-of-HP-BIOSCFG-driver-documentation

Hi Jorge!


Apr 4, 2023 22:24:36 Jorge Lopez <jorgealtxwork@...il.com>:

> Hi Thomas,
>
> BTW, I decided to submit all files individually to facilitate the
> review process.  Only Makefile and Kconfig files will be provided as a
> single patch.
> I will be out of town until April 11 and will reply back upon my return.

Sorry for the slow response.

>
> Please see my comments below.
>
>>
>>> +             HP specific types
>>> +             -----------------
>>> +                     - ordered-list - a set of ordered list valid values
>>> +                     - sure-start
>>
>> Could you explain what "sure-start" does?
>> Is it actually an attribute type of which multiple attributes can exist?
>>
>
> It is an attribute type of which multiple attributes can exist.
> At this moment  Sure-Start reports both the number of audit logs and
> all logs reported by BIOS.
> Sure-Start is exposed directly under
> /sys/class/firmware-attributes/*/attributes/.   Sure Start does not
> provide any authentication.

But what does it mean?
For ordered-list there is a nice explanation.

>
>> Or are there just some global properties that need to be exposed?
>> If it is global it should be directly under
>> /sys/class/firmware-attributes/*/authentication/
>> without needing the type.
>>
>>> +
>>> +
>>>               All attribute types support the following values:
>>>
>>>               current_value:
>>> @@ -42,16 +48,16 @@ Description:
>>>                               description of the at <attr>
>>>
>>>               display_name_language_code:
>>> -                                             A file that can be read to obtain
>>> -                                             the IETF language tag corresponding to the
>>> -                                             "display_name" of the <attr>
>>> +                             A file that can be read to obtain
>>> +                             the IETF language tag corresponding to the
>>> +                             "display_name" of the <attr>
>>
>> Are these reindentations and other cleanups intentional?
>>
>> If they are intentional and there are no interactions with your actual
>> patch you could split them into their own patch and submit them
>> separately.
>>
>> This way we wouldn't have to worry about them here anymore.
>
> They were unintentionally.  I will reset them back in the next review
>>
>> Note:
>> These indentations are different from the newly introduced documentation.
>>
>>>
>>> +             audit_log_entries:
>>> +                                     A read-only file that returns the events in the log.
>>> +                                     Values are separated using semi-colon (``;``)
>>> +
>>> +                                     Audit log entry format
>>> +
>>> +                                     Byte 0-15:   Requested Audit Log entry  (Each Audit log is 16 bytes)
>>> +                                     Byte 16-127: Unused
>>
>> How to interpret each log entry?
>>
>
> Byte 0: status
> 1: event id
> 2: msg number
> 3: severity
> 4: source ID
> 5: system state at event
> 6-12 Time stamp
> 13-15: internal buffer data
>
> Application needs to have knowledge of the data provided by BIOS in
> order to interpret the audit log.
>
>> If it is an opaque thing from the firmware that would also be useful to
>> know.
>>
>>> +
>>> +             audit_log_entry_count:
>>> +                                     A read-only file that returns the number of existing audit log events available to be read.
>>> +                                     Values are separated using comma (``,``)
>>> +
>>> +                                     [No of entries],[log entry size],[Max number of entries supported]
>>
>> Will log entry size always be 16? Or can it be bigger in the future when
>> more bytes are used?
>> This should be mentioned.
>
> Log entry size is always 16 bytes in size.  The reason is to report a
> maximum of 256 entries.  Total 4096 bytes

Does it make sense to expose the number 16 from sysfs if it never can change anyways?

Note you can also customize the filesize reported in sysfs to expose the maximum size to be used by userspace.

>>
>> Is audit_log_entry_count ever used without reading audit_log_entries
>> right after?
> Yes. The counter is necessary to determine how many logs are available
> to be read.

I think the cleaner interface would be to have users provide a buffer to read into and then they check the return value of read().
Users can't trust the count value anyways as it is prone to TOCTOU races.

>
>> If not the count file could be dropped.
>>
>>> +What:                /sys/class/firmware-attributes/*/authentication/SPM/status
>>> +Date:                March 29
>>> +KernelVersion:       5.18
>>> +Contact:     "Jorge Lopez" <jorge.lopez2@...com>
>>> +Description: 'status' is a read-only file that returns ASCII text reporting
>>> +             the status information.
>>> +
>>> +               State:  Not Provisioned / Provisioned / Provisioning in progress
>>> +               Version:  Major.   Minor
>>> +               Feature Bit Mask: <16-bit unsigned number display in hex>
>>
>> How are these bits to be interpreted?
> This information is provided by BIOS.  It is one of those obscure
> values from BIOS.
>>
>>> +               SPM Counter: <16-bit unsigned number display in base 10>
>>> +               Signing Key Public Key Modulus (base64):
>>> +               KEK Public Key Modulus (base64):
>>
>> Is " (base64)" supposed to be part of the contents of the file?
>
> The information reported for Signing Key and KEK public key are
> reported as base64 values.  It applies only to the data and not to the
> file contents.

Put is the file format:
KEK Public Key Modulus (base64): ...
KEK Public Key Modulus: ...

The docs indicate the former.

>>
>>> +
>>> +
>>> +What:                /sys/class/firmware-attributes/*/authentication/SPM/statusbin
>>> +Date:                March 29
>>> +KernelVersion:       5.18
>>> +Contact:     "Jorge Lopez" <jorge.lopez2@...com>
>>> +Description: 'statusbin' is a read-only file that returns identical status
>>> +             information reported by 'status' file in binary format.
>>
>> This documentation should contain enough information to understand the
>> files contents.
>>
>>
>> I understand that one WMI call will return all the fields that are part
>> of the "status" and "statusbin" in one response.
>>
>> Are these WMI calls especially expensive or called especially
>> frequently?
>>
>
> Unfortunately the WMI to read the Status binary data is expensive
> hence the reason of only calling once.

Hm, I still dislike the interface, sorry.
What about caching the values in the driver and exposing them via different files?

>> If not I would still argue to split them into one file per field and
>> remove the statusbin file.
>>
>
> Regards,
> > Jorge

Powered by blists - more mailing lists