From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web08.34789.1623067504403116112 for ; Mon, 07 Jun 2021 05:05:04 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=SqmEgm4M; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1623067503; h=from:from:reply-to: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=2grdbRLZlC6ilVxiN4JUJyCc0ANFaLOEl3V5C8kv/Og=; b=SqmEgm4MzcQI9mOfVRenqTmNpwME7X2fzBxquZuTzCpkbSWEsRqGUrjGExrozcnbW4NdwF rz5+KSXUUl/IiazDsgBv2h8lMJZghIIcgyV073JW/zAQSi3vrXht+OK4jO0kqkQ5jAnJT7 kB4UBrVKWn8Yp3tcoE/ppJtbPg21/A4= 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-112-xYLVyNt1P_Cz8sX1R9hceQ-1; Mon, 07 Jun 2021 08:05:00 -0400 X-MC-Unique: xYLVyNt1P_Cz8sX1R9hceQ-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 95835802B4F; Mon, 7 Jun 2021 12:04:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-75.ams2.redhat.com [10.36.114.75]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 31EF95E275; Mon, 7 Jun 2021 12:04:55 +0000 (UTC) Subject: Re: [edk2-devel] [RESEND PATCH RFC v3 00/22] Add AMD Secure Nested Paging (SEV-SNP) support From: "Laszlo Ersek" To: Brijesh Singh Cc: devel@edk2.groups.io, James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Erdem Aktas , Eric Dong , Ray Ni , Rahul Kumar , Ard Biesheuvel Reply-To: devel@edk2.groups.io, lersek@redhat.com References: <20210526231118.12946-1-brijesh.singh@amd.com> <4aa01f68-e76c-65a7-192b-f65ca62517f1@redhat.com> <38eca7ad-831e-bccb-1578-cf8084c86a39@amd.com> <87b681ea-294a-4ace-a4ee-c0ced54daa78@redhat.com> Message-ID: Date: Mon, 7 Jun 2021 14:04:54 +0200 MIME-Version: 1.0 In-Reply-To: <87b681ea-294a-4ace-a4ee-c0ced54daa78@redhat.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: 8bit On 06/04/21 15:09, Laszlo Ersek wrote: > On 06/04/21 13:50, Brijesh Singh wrote: >> The main issue is I typed wrong branch name in the cover letter. The >> branch name should be "sev-snp-rfc-3" and not "sev-snp-rfc-2". I >> apologies for it :(. Ray asked the branch name and I replied him with >> the correct branch. > > Hmmm... indeed, but that discussion happened off-list, namely under the > original posting of this v3 RFC set that did not reach the list. And now > I was working with my list folder. >> >> https://github.com/AMDESE/ovmf/tree/sev-snp-rfc-3 >> >> This branch was based on commit 5531fd48ded1271b8775725355ab83994e4bc77c >> from the upstream.  > > Right, this branch indeed averts problem (2); it is in sync with the > posted series. Thanks! > > Problem (1) stays the same -- git-rebase reports the same issue as > git-am above, for patch#21. So, I'm going to review this version on the > list, but I'll skip patch#21 (or perhaps I'll attempt to make useful > comments there too, if I can). I re-reviewed patch #3 today, and reviewed patch #4 as well. Because the data flow was not explained in advance, regarding the "SevSnpEnabled" and "HypervisorFeatures" fields, I wasted a huge amount of time reviewing ResetVector details that ultimately proved irrelevant. Additionally, I've found that several patches modify multiple modules in one step, typically ResetVector and PlatformPei. Honestly, this is inexplicable to me, given the edk2 coding style. The edk2 style permits multi-module patches only in the most exceptional cases. In a typical producer/consumer setup, the producer module patch comes first, the consumer module patch comes second. Breaking this edk2 rule is very detrimental to my efficiency as a reviewer. Either way, I'm done reviewing RFCv3; primarily because tomorrow and the day after I have some time-sensitive work to do, and afterwards, I'm going to disapper for a good while, to salvage the shreds of my brain that I've been left with, after the last two weeks. In the meantime, assuming no other reviewer wishes to comment RFCv3, please rework this series carefully -- even if it has "RFC" status, that's not a license to break edk2 coding style, structure patches incorrectly, diverge from spec-dictated constants, omit detailed explanations in commit messages (data flow!), duplicate code (assembly code!) mindlessly, and so on. Thanks Laszlo