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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#74770): https://edk2.groups.io/g/devel/message/74770 Mute This Topic: https://groups.io/mt/82479052/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
