> From: Ard Biesheuvel [mailto:ard.biesheuvel@xxxxxxxxxx] > > Actually, no, The issue I raised the last time around was not addressed anywhere, and is not even mentioned in the commit log. > > The problem is that there is an ambiguity in the implementation of the configuration table entries that represent capsule with the EFI_CAPSULE_PERSIST_ACROSS_RESET and EFI_CAPSULE_POPULATE_SYSTEM_TABLE flags set. > > First of all, there is the padding issue: > > typedef struct { > u32 capsule_array_number; > void *capsule_addr[]; > } __packed efi_capsule_table_t; > > This deviates from the implementation used by EDK2 (which does not pack the array), which means this code is currently broken on 64-bit implementations. > Then, there is the issue of the capsule_addr member, > which is described by the spec as follows: > > """ > The EFI System Table entry must point to an array of capsules that contain the same CapsuleGuid value. > """ > > This is open to interpretation, but an 'array of capsules' is not the same as an 'array of pointers to capsules'. > > I have reminded the USWG again that this needs to be clarified. > It looks like that my test BIOS on Intel Kabylake board interprets it *pointer" and pack the array, so my patch can work on that board. Yes, the spec is ambiguous that the description[1] in spec treats it as *pointer", and description[2] treats it as *capsule* (header + data ???) [1] "Firmware that processes a capsule that has the CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE Flag set in its header will coalesce the contents of the capsule from the ScatterGatherList into a contiguous buffer and must then place a *pointer* to this coalesced capsule in the EFI System Table after the system has been reset. Agents searching for this capsule will look in the EFI_CONFIGURATION_TABLE and search for the capsule’s GUID and associated *pointer* to retrieve the data after the reset." [2] " The EFI System Table entry must point to an array of *capsules* that contain the same CapsuleGuid value." I tend to [1] :-), and it seems to me that [2] has a disadvantage that it must have a contiguous physical memory large enough for all capsules. > For more info, please refer to > https://lists.01.org/pipermail/edk2-devel/2017-March/008141.html > + CapsuleTable->CapsuleArraySize = Size - sizeof(EFI_CAPSULE_TABLE); I think 'CapsuleTable->CapsuleArraySize = Size - sizeof (U32)' presents the size of capsule array. > + for (Index = 0; Index < CapsuleNumber; Index++) { > + CopyMem(&CapsuleTable->Capsule[Index], CapsulePtrCache[Index], sizeof(EFI_CAPSULE_TABLE)); > + } Here it only copies the capsule headers (EFI_CAPSULE_TABLE) to ' CapsuleTable->Capsule[Index]'. Should it also copy capsule data for each capsule here? Otherwise OS can only get the capsule header I think. Ard if you get clarification (capsule pointer or capsule header+data in EFI System Table entry) from USWG, would you please notify me and I can update my patch accordingly. Thanks! BR qiuxu ��.n��������+%������w��{.n�����{����* jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
