On 5/4/23 15:53, Dexuan Cui wrote:
> - if (_tdx_hypercall(TDVMCALL_MAP_GPA, start, end - start, 0, 0))
> + while (1) {
> + memset(&args, 0, sizeof(args));
> + args.r10 = TDX_HYPERCALL_STANDARD;
> + args.r11 = TDVMCALL_MAP_GPA;
> + args.r12 = start;
> + args.r13 = end - start;
> +
> + ret = __tdx_hypercall_ret(&args);
> + if (ret != TDVMCALL_STATUS_RETRY)
> + break;
> + /*
> + * The guest must retry the operation for the pages in the
> + * region starting at the GPA specified in R11. Make sure R11
> + * contains a sane value.
> + */
> + map_fail_paddr = args.r11;
> + if (map_fail_paddr < start || map_fail_paddr >= end)
> + return false;
This should probably also say: "r11" comes from the untrusted VMM.
Sanity check it.
Should this *really* be "map_fail_paddr >= end"? Or is "map_fail_paddr
> end" sufficient. In other words, is it really worth failing this if a
VMM said to retry a 0-byte region at the end?
> + if (map_fail_paddr == start) {
> + retry_cnt++;
> + if (retry_cnt > max_retry_cnt)
I think we can spare two bytes in a few spots to make these 'count'
instead of 'cnt'.
> + return false;
> + } else {
> + retry_cnt = 0;
> + start = map_fail_paddr;
> + }
> + }
this fails the "normal operation should be at the lowest indentation"
rule. How about this:
while (retry_count < max_retries) {
...
/* "Consume" a retry without forward progress: */
if (map_fail_paddr == start) {
retry_count++;
continue;
}
start = map_fail_paddr;
retry_count = 0;
}
// plus maybe a wee bit different 'ret' processing
'max_retries' also ends up being a misnomer. You can have as many
retries as there are pages plus 'max_retries'. It's really "maximum
allowed consecutive failures". Maybe it should be "max_retries_per_page".
