On Thu, Aug 19, 2021 at 07:09:42PM +0200, Borislav Petkov wrote:

On Thu, Aug 19, 2021 at 09:58:31AM -0500, Michael Roth wrote:

quoted

Not sure what you mean here. All the interfaces introduced here are used
by acpi.c. There is another helper added later (efi_bp_find_vendor_table())
in "enable SEV-SNP-validated CPUID in #VC handler", since it's not used
here by acpi.c.

Maybe I got confused by the amount of changes in a single patch. I'll
try harder with your v5. :)

quoted

There is the aforementioned efi_bp_find_vendor_table() that does the
simple iteration, but I wasn't sure how to build the "find one of these,
but this one is preferred" logic into it in a reasonable way.

Instead of efi_foreach_conf_entry() you simply do a bog-down simple
loop and each time you stop at a table, you examine it and overwrite
pointers, if you've found something better.

With "overwrite pointers" I mean you cache the pointers to those conf
tables you iterate over and dig out so that you don't have to do it a
second time. That is, *if* you need them a second time. I believe you
call at least efi_bp_get_conf_table() twice... you get the idea.

Sorry I'm still a little confused on how to determine "something better",
since it's acpi.c that decides which GUID is preferred, whereas
efi_find_vendor_table() is a library function with no outside knowledge
other than its arguments, so to return the preferred pointer it would need
both/multiple GUIDs passed in as arguments, wouldn't it? (or a callback)

Another alternative is something like what
drivers/firmware/efi/efi.c:common_tables does, where interesting table
GUIDs are each associated with a pointer, and all the pointers can then
be initialized with to the corresponding table address with a single pass.
But would need to be careful to re-initialize those pointers when BSS gets
cleared, or declare them in __section(".data"). Is that closer to what
you were thinking?

quoted

I could just call it once for each of these GUIDs though. I was
hesitant to do so since it's less efficient than existing code, but if
it's worth it for the simplification then I'm all for it.

Yeah, this is executed once during boot so I don't think you can make it
more efficient than a single iteration over the config table blobs.

In v5, I've simplified things to just call efi_find_vendor_table() once
for ACPI_20_TABLE_GUID, then once for ACPI_TABLE_GUID if that's not
available. So definitely doesn't sound like what you are suggesting here,
but does at least simplify code and gets rid of the efi_foreach* stuff. But
happy to rework things if you had something else in mind.

I hope that makes more sense.

-- 
Regards/Gruss,
    Boris.

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7Cmichael.roth%40amd.com%7C2a4304e70b5b4f2137f808d963340eec%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649897546039774%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GKDogD%2BOCN0swhmT4RZ2%2B3JmURF3e4qq%2FzgrxxFqJt0%3D&reserved=0