On 26 June 2017 at 04:09, Zhuo, Qiuxu <qiuxu.zhuo@xxxxxxxxx> wrote: >> 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. Are you running 64-bit firmware on this board? Because the EDK2 side lacks the 'packed' attribute, which means there is 4 bytes of padding after the first member of EFI_CAPSULE_TABLE. > 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. > Ah yes, I missed that bit before. So yes, it probably makes most sense to keep your definition, and update the EDK2 side so it works on 64-bit as well. >> 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! > I hope we can settle this by the end of this week, and if the clarification aligns with your definition, I will queue the patches as they are. Thanks, Ard. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
