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

["Laszlo Ersek" <lersek@redhat.com>]

On 05/05/21 01:03, Brijesh Singh wrote:
> 
> 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)

The additional complexity in the assembly source code is not lost on me.
I'm receptive to that.

I think EFI_LOAD_ERROR and EFI_NOT_READY would do more damage than good;
those error codes are meaningless in this context.

Therefore, please go ahead with the UINTN return type. And, whether you
pick #defines, or enum constants, for the symbolic names (for the
comparisons at the call sites), that's up to you.

Thanks!
Laszlo