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.web09.21703.1605853790277558572 for ; Thu, 19 Nov 2020 22:29:50 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@ibm.com header.s=pp1 header.b=UAozf9/Q; spf=pass (domain: linux.ibm.com, ip: 148.163.158.5, mailfrom: jejb@linux.ibm.com) Received: from pps.filterd (m0127361.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 0AK63MaH132161; Fri, 20 Nov 2020 01:29:48 -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=gU/lVR5DiPr44h5TA775xyC7AMNGqh8Y2q/BggU5OpQ=; b=UAozf9/Q95GbaSzvC6e3rUSNxSYABGAvATrgVUCPmKajowy97hnRFVYKWq1D+bMPl2rG OYuIHIjyNfWBMSZzw5pfvF97eR4Sd/yvTSSZfp+AjQV5Lg30vaRIEiyCNGKK3CUNaPko uKgt0ED/RFtK/AqEQGP2FukOVE5oqoaZWJaPXwS1cLAQKAC4FsJCrpQeH25VBufs9Pff CNGm95R7PcpGN+7cyLcDjT5ewLnfUm2h7wcqLfMz9agj0KnkMVCbCwwwDBW9s3PiD5Kj t5Y+I46tpnQCl8Tch0nwf/4NWI1tjS5Wn/DvukKWD2AEv9ebHfbbcFmoOKikHFU72svy xg== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 34x1m0sqna-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 20 Nov 2020 01:29:47 -0500 Received: from m0127361.ppops.net (m0127361.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.36/8.16.0.36) with SMTP id 0AK63eHk136162; Fri, 20 Nov 2020 01:29:47 -0500 Received: from ppma04dal.us.ibm.com (7a.29.35a9.ip4.static.sl-reverse.com [169.53.41.122]) by mx0a-001b2d01.pphosted.com with ESMTP id 34x1m0sqn3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 20 Nov 2020 01:29:47 -0500 Received: from pps.filterd (ppma04dal.us.ibm.com [127.0.0.1]) by ppma04dal.us.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 0AK6QVJf001962; Fri, 20 Nov 2020 06:29:47 GMT Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by ppma04dal.us.ibm.com with ESMTP id 34t6v9ynns-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 20 Nov 2020 06:29:46 +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 0AK6Tbv937028280 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 20 Nov 2020 06:29:37 GMT Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 9A30678063; Fri, 20 Nov 2020 06:29:43 +0000 (GMT) Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 80C3B7805E; Fri, 20 Nov 2020 06:29:41 +0000 (GMT) Received: from jarvis.int.hansenpartnership.com (unknown [9.80.207.206]) by b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP; Fri, 20 Nov 2020 06:29:41 +0000 (GMT) Message-ID: <4f54470da2553416f5ccd18564c02d204b6f068e.camel@linux.ibm.com> Subject: Re: [edk2-devel] [PATCH 3/4] OvmfPkg: create a SEV secret area in the AmdSev memfd From: jejb@linux.ibm.com Reply-To: jejb@linux.ibm.com To: Brijesh Singh , Laszlo Ersek , 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" Date: Thu, 19 Nov 2020 22:29:40 -0800 In-Reply-To: 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> 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-20_01:2020-11-19,2020-11-20 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 phishscore=0 lowpriorityscore=0 clxscore=1015 spamscore=0 mlxscore=0 bulkscore=0 malwarescore=0 suspectscore=0 mlxlogscore=999 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2011200040 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit 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. James