From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.5138.1606259727480710711 for ; Tue, 24 Nov 2020 15:15:27 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=HaLUDGxD; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1606259726; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=9lz6FtBaKqX0CNFe+l0GbwJY3sJL5QomtDtdipVVZRs=; b=HaLUDGxDqAbw1Lt8LXYr3xVn2dnDxiUBUrrHtZWSBYhK3bfFrhn9t8j0zETNCONb5//YJ8 kNLaWFnQge2biDQS5yfGke3X5caumAHatgwWCMSf6IULJBIrlV3tEYZm4cIwzwqBOZuFhR IEQP6DUQ7mTqA9dA7vFPz28hRLtZ58E= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-219-sxFfMyHeOpus1PHugTzyKg-1; Tue, 24 Nov 2020 18:15:22 -0500 X-MC-Unique: sxFfMyHeOpus1PHugTzyKg-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 604981074640; Tue, 24 Nov 2020 23:15:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-25.ams2.redhat.com [10.36.112.25]) by smtp.corp.redhat.com (Postfix) with ESMTP id DB7D11001281; Tue, 24 Nov 2020 23:15:16 +0000 (UTC) Subject: Re: [PATCH v2 3/6] OvmfPkg: convert ES Reset Block structure to be guided To: jejb@linux.ibm.com, 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" References: <20201120184521.19437-1-jejb@linux.ibm.com> <20201120184521.19437-4-jejb@linux.ibm.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 25 Nov 2020 00:15:15 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/24/20 20:05, James Bottomley wrote: > On Mon, 2020-11-23 at 23:16 +0100, Laszlo Ersek wrote: >> On 11/20/20 19:45, James Bottomley wrote: >>> 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 [...] > 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. The outermost table footer reports the cumulative size, so a zero-terminator at the front is not needed. > 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. Right, I think you are correct. >> (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. Great catch! :) >> (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. OK, thanks. I guess I should be OK with either interpretation, as long as we'll have considered it from a consistency perspective. >> (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. Ahh wait, no, now I understand it. When I wrote my previous comment, I didn't understand why you listed all of the fields explicitly, *except* the size. In particular, why you listed the GUID (*unlike* the size), in spite of the GUID *too* being pre-declared (*like* the size). But now I understand -- you didn't re-declare the GUID *field* for this particular GUIDed structure, you only provided the *value* of the GUID field. Perhaps juggling the words could improve understanding: GUID ("SEV-ES reset block"): 00f771de-1a7e-4fcb-890e-68c77e2fb44e But, it's up to you. I agree the listing is consistent the way you proposed. Thanks! Laszlo