From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web08.23652.1605869968078140047 for ; Fri, 20 Nov 2020 02:59:28 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=KzWYJiVK; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1605869967; 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=gsZczNj2EbtLuxDT0hJoohaF9T4p/jMEvj4o8L9caB8=; b=KzWYJiVKSCaX/kH8mJNMVKIFmbt1uR+TInbkY8jz5dJIM0HN5S9ptGMHO0uSUKahR7m0y7 ShpDgqxY/PDbtagZmANeuQvu6F1LEzD4/EHUjtQtxZ/mmCwdc5EfFSEy0wJ+KIKWSdARAQ 3H9KLGge1iTsD3kpjvYi/QXK4mbF2BA= 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-526-AJwYSapbMzuK8PuTlmtsuA-1; Fri, 20 Nov 2020 05:59:23 -0500 X-MC-Unique: AJwYSapbMzuK8PuTlmtsuA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 83119107ACE3; Fri, 20 Nov 2020 10:59:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-10.ams2.redhat.com [10.36.115.10]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6BA4E5D9D0; Fri, 20 Nov 2020 10:59:18 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 3/4] OvmfPkg: create a SEV secret area in the AmdSev memfd To: jejb@linux.ibm.com, Brijesh Singh , devel@edk2.groups.io Cc: dovmurik@linux.vnet.ibm.com, Dov.Murik1@il.ibm.com, ashish.kalra@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: <20201112001316.11341-1-jejb@linux.ibm.com> <20201112001316.11341-4-jejb@linux.ibm.com> <6db69ccd-340f-2df2-718b-5f7db09da0b8@redhat.com> <111856145fea09b5ed88a1dfa7b7d7ff6eece639.camel@linux.ibm.com> <0576e9ad-e844-c50a-a329-ba19138b587d@redhat.com> <4f54470da2553416f5ccd18564c02d204b6f068e.camel@linux.ibm.com> From: "Laszlo Ersek" Message-ID: <27f74a14-50d8-920c-6a95-00760123acc1@redhat.com> Date: Fri, 20 Nov 2020 11:59:17 +0100 MIME-Version: 1.0 In-Reply-To: <4f54470da2553416f5ccd18564c02d204b6f068e.camel@linux.ibm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 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/20/20 07:29, James Bottomley wrote: > On Thu, 2020-11-19 at 13:41 -0600, Brijesh Singh wrote: >> On 11/19/20 1:50 AM, Laszlo Ersek wrote: >>> On 11/18/20 21:23, James Bottomley wrote: >>>> On Mon, 2020-11-16 at 23:46 +0100, Laszlo Ersek wrote: >>>>> On 11/12/20 01:13, James Bottomley wrote: >>>> [... I made all the changes above this] >>>>>> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>>>>> b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>>>>> index 980e0138e7..7d3214e55d 100644 >>>>>> --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>>>>> +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>>>>> @@ -35,6 +35,8 @@ ALIGN 16 >>>>>> ; the build time RIP value. The GUID must always be 48 >>>>>> bytes >>>>>> from the >>>>>> ; end of the firmware. >>>>>> ; >>>>>> +; 0xffffffc2 (-0x3e) - Base Location of the SEV Launch >>>>>> Secret >>>>>> +; 0xffffffc6 (-0x3a) - Size of SEV Launch Secret >>>>>> ; 0xffffffca (-0x36) - IP value >>>>>> ; 0xffffffcc (-0x34) - CS segment base [31:16] >>>>>> ; 0xffffffce (-0x32) - Size of the SEV-ES reset block >>>>>> @@ -51,6 +53,8 @@ ALIGN 16 >>>>>> TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB >>>>>> 0 >>>>>> >>>>>> sevEsResetBlockStart: >>>>>> + DD SEV_LAUNCH_SECRET_BASE >>>>>> + DD SEV_LAUNCH_SECRET_SIZE >>>>>> DD SEV_ES_AP_RESET_IP >>>>>> DW sevEsResetBlockEnd - sevEsResetBlockStart >>>>>> DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F >>>>> (5) I'd prefer if we could introduce a new GUID-ed structure >>>>> for these new fields. The logic in QEMU should be extended to >>>>> start scanning at 4GB-48 for GUIDS. If the GUID is not >>>>> recognized, then terminate scanning. Otherwise, act upon the >>>>> GUID-ed structure found there as necessary, and then determine >>>>> the next GUID *candidate* location by subtracting the last >>>>> recognized GUID-ed structure's "size" field. >>>> So for this one, we can do it either way. However, the current >>>> design of the sevEsRestBlock is (according to AMD) to allow the >>>> addition of SEV specific information. Each piece of information >>>> is a specific offset from the GUID and the length of the >>>> structure can only grow, so the ordering is fixed once the info >>>> is added and you can tell if the section contains what you're >>>> looking for is present if the length covers it. >>>> >>>> We can certainly move this to a fully GUID based system, which >>>> would allow us to have an unordered list rather than the strict >>>> definition the never decreasing length scheme allows, but if we >>>> do that, the length word above becomes redundant. >>> Well, GUIDed structs in UEFI/PI are sometimes permitted to grow >>> compatibily, and for that, either a revision field or a size field >>> is necessary / used. I kind of desire both here -- it makes sense >>> to extend (for example) the SEV-ES reset block with relevant >>> information, and to add other blocks of information (identified >>> with different GUIDs). >>> >>> Basically I wouldn't want to finalize the SEV-ES AP reset block >>> just yet, *but* I also think this new information does not beloing >>> in the SEV-ES *AP reset block*. The new info is related to SEV-ES >>> alright, but not to the AP reset block, in my opinion. If you read >>> the larger context (the docs) in the assembly source around >>> "sevEsResetBlockStart", the launch secret just doesn't seem to fit >>> that. >>> >>>> I don't have a huge preference for either mechanism ... they seem >>>> to work equally well, but everyone should agree before I replace >>>> the length based scheme. I agree we should all agree about it >>>> first. >>> >>> And, to reiterate, I'd like to keep both the length fields and the >>> GUID-ed identification. In other words, a GUID should not imply an >>> exact struct size, just a minimum struct size. >> >> I agree with the GUID based approach, it aligns well with the future >> needs. Looking forwardm we will need to reserve couple of pages >> (secret and cpuid) for the SNP. In my WIP patches I extended reset >> block to define a new GUID for those new fields. >> >> https://github.com/AMDESE/ovmf/commit/87d47319411763d91219b377da709efdb057e662#diff-0ca7ec2856c316694c87b519c95db3270e0cac798eb09745cce167aad7f2d46dR28 >> >> And I am using this qemu patch to iterate through all the GUIDs and >> call the corresponding callbacks. >> >> https://github.com/AMDESE/qemu/commit/16a1266353d372cbb7c1998f27081fb8aa4d31e9 > > OK, if that's not yet upstream, I think we should do this properly: > that means having a guid and length that identifies the entire table > and then all the incorporated guids and lengths. That way we don't > have a double meaning for the reset block guid as both identifying the > start of the table and the reset vector data. Also it means we don't > need a zero guid to signal the end of the table. And also means the > reset block GUID doesn't have to always be present (if it got > deprecated for some reason). > > However, the downside is that you'll have to pull out the table by this > new guid at 0xffffffd0 and its length and then iterate over the table > to find the reset block guid ... but that will make it very easy to add > the additional guids. I agree with doing things properly. Thanks Laszlo