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.web12.1306.1620240969743264007 for ; Wed, 05 May 2021 11:56:10 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ix4ORwV1; 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=1620240968; 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=vS/I76Z8sZ1DNFX3LPJldnNB4I9OOa2SBbmxFQJSEE4=; b=ix4ORwV18u5jypTVGjKZ/gagfGJ5mtIg6CPmrFh/nxU+9RdrYrZkyJDwjTDZ2C8RnWc+bV NQu41AUqCR6EBADyrzSNiSJ/HNksdgxotm4cVCaCbpZ694dHB106w8U6w/HMkPtVVTCwWR 40KFSiIn5zmJZexE5+NT/JIffkXFSwU= 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-104-glgWFI8oNpK3MIJUVTtL2g-1; Wed, 05 May 2021 14:56:05 -0400 X-MC-Unique: glgWFI8oNpK3MIJUVTtL2g-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CE698107ACE4; Wed, 5 May 2021 18:56:03 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-136.ams2.redhat.com [10.36.113.136]) by smtp.corp.redhat.com (Postfix) with ESMTP id AC3735D703; Wed, 5 May 2021 18:56:01 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH RFC v2 05/28] MdePkg: Add AsmPvalidate() support To: devel@edk2.groups.io, brijesh.singh@amd.com Cc: James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Ard Biesheuvel , Erdem Aktas References: <20210430115148.22267-1-brijesh.singh@amd.com> <20210430115148.22267-6-brijesh.singh@amd.com> <2d5e8530-7433-f27e-3a89-9a9ce49bcc08@amd.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 5 May 2021 20:56:00 +0200 MIME-Version: 1.0 In-Reply-To: <2d5e8530-7433-f27e-3a89-9a9ce49bcc08@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 05/04/21 21:07, Brijesh Singh wrote: > > On 5/4/21 8:58 AM, Laszlo Ersek wrote: >> (4) The order of parameters listed in this comment block differs from >> the actual parameter list. >> >> The ECC plugin of the edk2 CI will catch this issue anyway. So, >> before submitting the patch set to the list, please submit a personal >> PR on github.com against the main repo, just to run CI on your >> patches. >> > Interestingly I did ran the series with edk2 CI and everything passed > before I submitted for the review. But, I will go ahead and fix the > order. Thank you for being careful up-front -- and then I don't understand this result. The "EccCheck" plugin is not disabled in "MdePkg/MdePkg.ci.yaml", and I distinctly remember ECC being incredibly pedantic about any function leading comment matching the function's declaration. Anyway, thanks for updating this, in the next version. >> Return EFI_UNSUPPORTED (0x8000_0003), or even EFI_NO_MAPPING >> (0x8000_0017), for value 6 (FAIL_SIZEMISMATCH). > > I am not sure if we really want to do this. You will see later in the > patches that in some cases the PVALIDATE will return a failure and we > will need to know the failure code to determine the next steps. > Especially this particular error code is used later. This error > happens when the page size of the backing pages does not match with > the pvalidated size. In those cases we need to retry the PVALIDATE > with lower page size so that a validation succeed. One such a example > is: > > - Guest ask hypervisor to add the page as 2M in RMP table. > > - Hypervisor added the page as 512 4K pages - because it was not able > to find a large backing pages. > > - Guest attempts to pvalidate the page as a 2M. The pvalidate will > return a failure saying its a size mismatch between the requested > pvalidated and RMP table. The recommendation is that guest should try > with a smaller page size. > > I would prefer to pass the pvalidate error as-is to caller so that it > can make the correct decision. My intent is certainly not to mask, or collapse, distinct outputs from PVALIDATE. The EFI_STATUS codes that I suggested for AsmPvalidate() keep the distinct PVALIDATE results distinct. The AMD APM (vol3) describes three result codes (SUCCESS, FAIL_INPUT, FAIL_SIZEMISMATCH), and I suggested a *bijective* EFI_STATUS mapping for those -- EFI_SUCCESS, EFI_INVALID_PARAMETER, and EFI_UNSUPPORTED; in that order. EFI_NO_MAPPING was only an alternative for EFI_UNSUPPORTED -- in case you preferred EFI_NO_MAPPING to EFI_UNSUPPORTED, for representing FAIL_SIZEMISMATCH. Each one of the EFI_STATUS codes I suggest corresponds uniquely to a direct PVALIDATE result code (injectivity), and each direct PVALIDATE result code is covered (surjectivity). So it's a bijection. CF is only meaningful on output if PVALIDATE succeeds, according to the AMD APM: If the instruction completed successfully, the rFLAGS bit CF indicates if the contents of the RMP entry were changed or not. Therefore CF should only be checked (and it *can* be checked, via the BOOLEAN "RmpEntryUpdated" output parameter that I also recommended) if EFI_SUCCESS is returned by AsmPvalidate(). Furthermore, I *did* check the (sole) AsmPvalidate() call site in the series, before posting my earlier comment. It goes like this, in patch #21 ("OvmfPkg/MemEncryptSevLib: Add support to validate system RAM"): > Ret = AsmPvalidate (RmpPageSize, Validate, Address, &EFlags); > > // > // Check the rFlags.CF to verify that PVALIDATE updated the RMP entry. > // If there was a no change in the RMP entry then we are either double > // validating or invalidating the memory. This can lead to a security compromise. > // > if (EFlags.Bits.CF) { > DEBUG (( > DEBUG_ERROR, "%a:%a: Double %a detected for address 0x%Lx\n", > gEfiCallerBaseName, > __FUNCTION__, > Validate ? "Validate" : "Invalidate", > Address > )); > SnpPageStateFailureTerminate (); > } > > return Ret; This logic is incorrect, in my opinion. CF should not be checked before ensuring that Ret equal SUCCESS. The correct logic for IssuePvalidate() would be: Status = AsmPvalidate (RmpPageSize, Validate, Address, &RmpEntryUpdated); if (EFI_ERROR (Status)) { return Status; } if (!RmpEntryUpdated) { SnpPageStateFailureTerminate (); } And then the caller of IssuePvalidate() -- PvalidateRange() -- can rely on the status codes, returned by IssuePvalidate(), with the following meanings: - EFI_SUCCESS: PVALIDATE completed *and* it successfully updated the RMP entry - EFI_INVALID_PARAMETER: PVALIDATE failed with FAIL_INPUT (and CF was meaningless on output) - EFI_UNSUPPORTED (or EFI_NO_MAPPING, if you like): PVALIDATE failed with FAIL_SIZEMISMATCH (and CF was meaningless on output); PVALIDATE should be retried with the small (4K) page size. I entirely agree with your high-level goal here. My point is that the mapping that I'm proposing loses *no* information, it is more idiomatic for edk2, and it's not difficult to implement in assembly. If you absolutely insist on returning status codes 0, 1, and 6, from AsmPvalidate(), I can live with that too (although I do think it's a lot less idiomatic for edk2); in that case however, please make the return type of AsmPvalidate() UINT32. (And to repeat: my other point is that the CF checking logic in IssuePvalidate() is currently wrong, as it relies on a value (CF) which may be indeterminate in some cases.) Thanks! Laszlo