Re: [PATCH] fpga: region: handle compat_id as an uuid

From: Tom Rix
Date: Fri Jul 30 2021 - 08:07:09 EST

On 7/29/21 6:48 PM, Xu Yilun wrote:
On Thu, Jul 29, 2021 at 12:16:47PM -0700, Tom Rix wrote:
On 7/29/21 11:51 AM, Moritz Fischer wrote:
On Wed, Jul 28, 2021 at 01:36:56AM +0000, Wu, Hao wrote:
On 7/26/21 3:12 PM, Russ Weight wrote:
On 7/26/21 1:26 PM, trix@xxxxxxxxxx wrote:
From: Tom Rix <trix@xxxxxxxxxx>

An fpga region's compat_id is exported by the sysfs
as a 128 bit hex string formed by concatenating two
64 bit values together.

The only user of compat_id is dfl. Its user library
opae converts this value into a uuid.

$ cat /sys/class/fpga_region/region1/compat_id

Is reported as
$ fpgainfo bmc
Pr Interface Id : f3c99413-5081-4aad-bced-07eb84a6d0bb

Storing a uuid as 2 64 bit values is vendor specific.
And concatenating them together is vendor specific.

It is better to store and print out as a vendor neutral uuid.

Change fpga_compat_id from a struct to a union.
Keep the old 64 bit values for dfl.
Sysfs output is now
I'm fowarding feedback from Tim Whisonant, one of the OPAE userspace

I think that this change to the sysfs for the compat_id node will
end up breaking the SDK, which does not expect the '-' characters to
be included when parsing the sysfs value. Currently, it is parsed as
a raw hex string without regard to any '-' characters. This goes for
any "guid" currently exported by sysfs and for what we read in the
device MMIO space.
Yes, it will.

And there are other places, like dfl-afu-main.c:afu_id_show()

outputs raw hex that sdk turns into a uuid.

Some options.

If no one but dfl will ever use it, then v1 of patchset.

If others can use it but don't want to change dfl, then v2 of patchset,
my favorite.

Or this one for uuid for everyone, what have been v3 but changed too much.

could dfl change generally to output uuid's to the sysfs ?

this would be generally helpful and a one time disruption to the sdk.
This change limited the output format to uuid_t, but if any hardware doesn't
use uuid_t on hardware may have to convert it back from the sysfs output in
userspace. Leave it to print hardware values (e.g. from register), and convert
it in userspace should be fine too I think.
I'm not entirely sure. I seem to recall there being examples of sysfs
files returning different things for different drivers.

That being said it seems largely cosmetic to add the '-' in between.

If it breaks userspace, I'm against it. If you *need* it make a
compat_uuid entry or something in that case?
My gripe is

For a nominally common interface, compat_id has a vendor specific output.

If for example another vendor wanted to use this field but their natural
format was an OF string.

16 bytes of raw hex would not work for them, so they would roll their own.

which defeats the purpose of a common interface.

The language in the docs as-is is vague on the output format.

DFL is the only user of the interface.

So ver 2

Keeps the output as-is for dfl, so nothing breaks in userspace

And adds flexibility for vendors to output their appropriate natural form.

So compat_id becomes generally useful.
Mixing types seems be strongly against in Documentation/filesystems/sysfs.rst.
So in my opinion there should be a determined format for the output. The
concern for this patch is which one is a better format, uuid style or
128 bit raw hex?

And I vote for 128 bit raw hex, as other vendors may not use uuid_t as
the identifier, may be an OF string. So we don't have to force them
decorate it as the uuid style.

So you would be ok with v2 of this patchset ?




- Moritz