Re: [edk2-devel] [PATCH RFC v2 05/28] MdePkg: Add AsmPvalidate() support

["Brijesh Singh" <brijesh.singh@amd.com>]

On 5/4/21 3:28 PM, Brijesh Singh wrote:
> On 5/4/21 2:55 PM, Brijesh Singh via groups.io wrote:
>> On 5/4/21 2:07 PM, Brijesh Singh via groups.io wrote:
>>>> 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.
>>>
>> I am perfectly fine if the function return UINTN and then use #define
>> instead of the enum to define the PVALIDATE return code. So that caller
>> can check the error code. Let me know your thought on #define instead of
>> the enum.
> Apologies, I missed the fact that you said document the mapping between
> the PVALIDATE return value and EFI_STATUS. So a caller is responsible to
> look at the EFI document to know what the error code means. The
> unsupported here does not mean that PVALIDATE is not support on
> platform. I am good with it. I will go ahead with it.

While coding it I am coming to realizing that mapping from PVALIDATE
return value to EFI will add more code in assemblym and wanted to make
sure that we all are okay with it. The proposed mapping looks like this:

// No mapping required, both the values are zero

PVALIDATE_RET_SUCCESS -> EFI_SUCCESS   

// Pvalidate.nasm need to map from PVALIDATE fail input (1) to
EFI_INVALID_PRAMS (2)

PVALIATE_RET_FAIL_INPUT -> EFI_INVALID_PARAMS

// Pvalidate.nasm need to map from PVALIDATE SIZE_MISMATCH (6) to
EFI_UNSUPPORT(3)

PVALIATE_RET_SIZEMISMATCH -> EFI_UNSUPPORTED

May I recommend to use UINTN and simply propagate the PVALIDATE return
value  or use the below mapping to simplify the pvalidate.nasm
implementation:

PVALIDATE_RET_SUCCESS(0) -> EFI_SUCCESS(0)

PVALIDATE_RET_FAIL_INPUT(1) -> EFI_LOAD_ERROR(1)

PVALIDATE_RET_SIZE_MISMATCH(6) -> EFI_NOT_READY(6)

Thanks