From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by mx.groups.io with SMTP id smtpd.web08.1562.1606244724108815605 for ; Tue, 24 Nov 2020 11:05:24 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@ibm.com header.s=pp1 header.b=GA1EMejG; spf=pass (domain: linux.ibm.com, ip: 148.163.158.5, mailfrom: jejb@linux.ibm.com) Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 0AOJ2REk055498; Tue, 24 Nov 2020 14:05:21 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : subject : from : reply-to : to : cc : date : in-reply-to : references : content-type : mime-version : content-transfer-encoding; s=pp1; bh=UpthwGAqtkx0U8EadsoDwQ0kQru//HFpejp6oWSmbdU=; b=GA1EMejGNOp5co+NWnZQ5lxUdMusNgf+1+1rvk/DESe4di8E/RAsS8vi6/py2Z1Rdplc 8jcT2LrCQR/inwS9emhHhnPqyYQnmPYQ0oIQsuPBahhT4OJGVBLz+TwUFcDSZa6hf4xY eheXAyDjASNPpfeYTEWnEE3Se7Ov3N5xGKtQfA69Fjwxy3GWmslaQJ4q8V1rz/OuVZPl /ubc086+XP66zumoMmrVado8a45nKMF3V+201CYblDgDZQ9sip/G0DEkgZ3ZBqAnnHTh 3L3Y6IeO7aR5ziDo5kY/295nGoU2MHUo1ClAZ+7iMM4JrqEVuAiwYgjgv9cjaCr2n0JN Rw== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 350vbvhfd6-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 24 Nov 2020 14:05:21 -0500 Received: from m0098421.ppops.net (m0098421.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.36/8.16.0.36) with SMTP id 0AOJ2nxD056692; Tue, 24 Nov 2020 14:05:21 -0500 Received: from ppma03dal.us.ibm.com (b.bd.3ea9.ip4.static.sl-reverse.com [169.62.189.11]) by mx0a-001b2d01.pphosted.com with ESMTP id 350vbvhfcr-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 24 Nov 2020 14:05:21 -0500 Received: from pps.filterd (ppma03dal.us.ibm.com [127.0.0.1]) by ppma03dal.us.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 0AOJ4GuV014901; Tue, 24 Nov 2020 19:05:20 GMT Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by ppma03dal.us.ibm.com with ESMTP id 34xth9g418-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 24 Nov 2020 19:05:20 +0000 Received: from b03ledav004.gho.boulder.ibm.com (b03ledav004.gho.boulder.ibm.com [9.17.130.235]) by b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 0AOJ5ACk17760788 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 24 Nov 2020 19:05:10 GMT Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id AF1AE7805E; Tue, 24 Nov 2020 19:05:16 +0000 (GMT) Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 836AA7805C; Tue, 24 Nov 2020 19:05:14 +0000 (GMT) Received: from jarvis.int.hansenpartnership.com (unknown [9.85.194.234]) by b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP; Tue, 24 Nov 2020 19:05:14 +0000 (GMT) Message-ID: Subject: Re: [PATCH v2 3/6] OvmfPkg: convert ES Reset Block structure to be guided From: "James Bottomley" Reply-To: jejb@linux.ibm.com To: Laszlo Ersek , devel@edk2.groups.io Cc: dovmurik@linux.vnet.ibm.com, Dov.Murik1@il.ibm.com, ashish.kalra@amd.com, brijesh.singh@amd.com, tobin@ibm.com, david.kaplan@amd.com, jon.grimm@amd.com, thomas.lendacky@amd.com, frankeh@us.ibm.com, "Dr . David Alan Gilbert" Date: Tue, 24 Nov 2020 11:05:13 -0800 In-Reply-To: References: <20201120184521.19437-1-jejb@linux.ibm.com> <20201120184521.19437-4-jejb@linux.ibm.com> User-Agent: Evolution 3.34.4 MIME-Version: 1.0 X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.312,18.0.737 definitions=2020-11-24_05:2020-11-24,2020-11-24 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0 spamscore=0 mlxscore=0 priorityscore=1501 lowpriorityscore=0 malwarescore=0 adultscore=0 mlxlogscore=999 clxscore=1015 impostorscore=0 suspectscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2011240111 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit On Mon, 2020-11-23 at 23:16 +0100, Laszlo Ersek wrote: > On 11/20/20 19:45, James Bottomley wrote: > > Convert the current ES reset block structure to an extensible guid > > based structure by appending a header and length, which allow for > > multiple guid based data packets to be inserted. > > > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077 > > Signed-off-by: James Bottomley > > > > --- > > > > v2: added > > --- > > OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 49 +++++++++++++++- > > ---- > > 1 file changed, 38 insertions(+), 11 deletions(-) > > (1) Please update the subject line to: > > OvmfPkg/ResetVector: convert SEV-ES Reset Block structure to be > GUIDed > > - edk2 prefers including module names too in the patch subjects > - "ES" is harder to understand than "SEV-ES" > - "GUIDed" is harder to misread as "guided" > - subject length is still OK (70 chars) Done. > > diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm > > b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm > > index 980e0138e7fe..baf9d09f3625 100644 > > --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm > > +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm > > @@ -25,21 +25,40 @@ ALIGN 16 > > TIMES (0x1000 - ($ - EndOfPageTables) - 0x20) DB 0 > > %endif > > > > +; > > +; Padding to ensure first guid starts at 0xffffffd0 > > +; > > +TIMES (32 - ((guidedStructureEnd - guidedStructureStart) % 32)) DB > > 0 > > (2) This will insert 32 zero bytes if the size is already aligned to > 32 > bytes (because 32-0 = 32). In other words, the above produces 1 to 32 > zero bytes, dependent on table size. > > The variant I proposed in point (5) at > > https://edk2.groups.io/g/devel/message/67621 > > https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00781.html > > takes this into account, and only prepends 0 to 31 bytes (inclusive): > > TIMES (31 - (guidedStructureEnd - guidedStructureStart + 31) % 32) > DB 0 > > - This variant subtracts 1 inside the remainder operation (which is > expressed as adding 31). > > - For compensation, it adds 1 just outside of the remainder > operation. > This addition in effect increases the subtrahend for the leftmost > 32. > Therefore this (-1) addend is ultimately folded into the leftmost > 32, > yielding 31 on the leftmost side. > > TIMES (32 - ((guidedStructureEnd - guidedStructureStart - 1) % 32 + > 1)) DB 0 > ^^^ ^ > ^^ > | | > | c > ompensate > | i > n the > | r > emainder > | > slide down > residue > class > modulo 32 > > > TIMES (32 - ((guidedStructureEnd - guidedStructureStart + 31) % 32) > - 1) DB 0 > ^^^^ > ^^^ > | > | > | > unnest > | > increment > | > from > | > subtrahend > | > express > modular > subtraction > as > addition, to > avoid > using % on a > negative > integer (in > case size > were 0) > > TIMES (31 - ((guidedStructureEnd - guidedStructureStart + 31) % 32)) > DB 0 > ^^ > | > fold previous (-1) addend into leftmost constant > > - This juggling of 1 results in no changes for residue classes 1 > through > 31, but wraps the outermost result (the padding size) for residue > class 0, from 32 to 0. I get the mathematics, but I'm a bit hazy on the why. I structured my patch on the basis that some zero padding seems necessary (which does mean you have to add extra zeros when the structure fits exactly into 32 bytes). If we don't need any zero pad at all then I agree with what you say above, we should use the shortest feasible pad. My other question is why 32 above? If the object is simply to push the table guid to 0xffffffd0 in the OVMF.fd then TIMES (15 - ((guidedStructureEnd - guidedStructureStart + 15) % 16)) DB 0 or TIMES 16 - ((guidedStructureEnd - guidedStructureStart) % 16)) DB 0 Depending on whether we need zeros or not, always works because the code is align 16 not align 32. > > + > > +; Guided structure. To traverse this you should first verify the > > +; presence of the table header guid > > (3) suggest "table footer GUID" (the GUID follows the data, in > address order) Yes, I think header because it's what I come to first traversing backwards but perhaps footer is better because most people don't think backwards. > > +; (96b582de-1fb2-45f7-baea-a366c55a082d) at 0xffffffd0. If that > > +; is found, the two bytes at 0xffffffce are the entire table > > length. > > (4) can we make the whole table size field 32-bit? I don't have a > particular use case in mind, it just looks more extensible than 16- > bit. We can still keep the individual structs we have in mind 16-bit > sized. Actually, no ... or at least not unless we alter the reset vector. The problem is the reset vector is actually: nop nop jmp EarlyBspInitReal16 But that jmp is 16-bit code, meaning the relative jump which goes over the table is at most -32768, so the table can never be larger than about 32k bytes. That's not to say we can't have a larger table ... we definitely can, but it can't be where it is now. we'd have to do something different like make the table guid describe where the actual table is located (as a 32 bit offset and length) so as not to break up the 16 bit jump code. > > +; > > +; The table is composed of structures with the form: > > +; > > +; Data (arbitrary bytes identified by guid) > > +; length from start of guid to end of data (2 bytes) > > (5) This is hard to interpret, as "data" precedes "guid" in address > space (guid is a footer, not a header). > > I suggest "length from start of data to end of GUID" done. > > +; guid (16 bytes) > > +; > > +; so work back from the header using the length to traverse until > > you > > (6) suggest "from the footer" > > > > +; either find the guid you're looking for or run off the end of > > the > > +; table. > > (7) suggest "run off the beginning of the table" > > ... I realize "start" and "end" can be interpreted temporally and > spatially. In a forward traversal they are the same, but now they > aren't. I suggest we use the spatial (address space order) > interpretation. Well we have to be consistent, so if you're thinking backwards it's header and end, but the other way around it has to be footer and beginning, so I updated it. > > +; > > +guidedStructureStart: > > + > > ; > > ; SEV-ES Processor Reset support > > ; > > ; sevEsResetBlock: > > ; For the initial boot of an AP under SEV-ES, the "reset" RIP > > must be > > -; programmed to the RAM area defined by SEV_ES_AP_RESET_IP. A > > known offset > > -; and GUID will be used to locate this block in the firmware and > > extract > > -; the build time RIP value. The GUID must always be 48 bytes > > from the > > -; end of the firmware. > > +; programmed to the RAM area defined by SEV_ES_AP_RESET_IP. The > > data > > +; format is > > ; > > -; 0xffffffca (-0x36) - IP value > > -; 0xffffffcc (-0x34) - CS segment base [31:16] > > -; 0xffffffce (-0x32) - Size of the SEV-ES reset block > > -; 0xffffffd0 (-0x30) - SEV-ES reset block GUID > > -; (00f771de-1a7e-4fcb-890e-68c77e2fb44e) > > +; IP value [0:15] > > +; CS segment base [31:16] > > +; > > +; SEV-ES reset block GUID: 00f771de-1a7e-4fcb-890e-68c77e2fb44e > > (8) Did I understand from the v1 discussion that the corresponding > QEMU parser is not upstream yet? (Or at least not released?) Yes, current QEMU upstream has no support yet for searching for this table although I think patches have been sent. > (9) The 16-bit size field of the SEV-ES reset block structure is not > documented. Well, it is ... it's become part of the table structure, so it's documented at the top of the table as: ; The table is composed of structures with the form: ; ; Data (arbitrary bytes identified by guid) ; length from start of data to end of guid (2 bytes) ; guid (16 bytes) All the comment is doing is describing the layout of the data. I can add a length here if you like, but I'd probably better add one to the secret table as well to be consistent. > > ; > > ; A hypervisor reads the CS segement base and IP value. The CS > > segment base > > ; value represents the high order 16-bits of the CS segment > > base, so the > > @@ -48,8 +67,6 @@ ALIGN 16 > > ; program the EIP register with the IP value as read. > > ; > > > > -TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0 > > - > > sevEsResetBlockStart: > > DD SEV_ES_AP_RESET_IP > > DW sevEsResetBlockEnd - sevEsResetBlockStart > > @@ -57,6 +74,16 @@ sevEsResetBlockStart: > > DB 0x89, 0x0E, 0x68, 0xC7, 0x7E, 0x2F, 0xB4, 0x4E > > sevEsResetBlockEnd: > > > > +; > > +; Table header: length of whole table followed by table header > > (10) I suggest "table footer" (twice) done. Regards, James > > +; guid: 96b582de-1fb2-45f7-baea-a366c55a082d > > +; > > + DW guidedStructureEnd - guidedStructureStart > > + DB 0xDE, 0x82, 0xB5, 0x96, 0xB2, 0x1F, 0xF7, 0x45 > > + DB 0xBA, 0xEA, 0xA3, 0x66, 0xC5, 0x5A, 0x08, 0x2D > > + > > +guidedStructureEnd: > > + > > ALIGN 16 > > > > applicationProcessorEntryPoint: > > > > Thanks! > Laszlo >