Re: [Xen-devel] [PATCH] x86/HVM: correct hvmemul_map_linear_addr() for multi-page case

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.08.2023 16:30, Roger Pau Monné wrote:
> On Wed, Sep 12, 2018 at 03:09:35AM -0600, Jan Beulich wrote:
>> The function does two translations in one go for a single guest access.
>> Any failure of the first translation step (guest linear -> guest
>> physical), resulting in #PF, ought to take precedence over any failure
>> of the second step (guest physical -> host physical).
> 
> If my understanding is correct, this is done so that any #PF that
> would arise from the access is injected to the guest, regardless of
> whether there might also be gfn -> mfn errors that would otherwise
> prevent the #PF from being detected.

Yes.

>> @@ -600,27 +614,28 @@ static void *hvmemul_map_linear_addr(
>>              goto out;
>>  
>>          case HVMTRANS_bad_gfn_to_mfn:
>> -            err = NULL;
>> -            goto out;
>> +            err = update_map_err(err, NULL);
>> +            continue;
>>  
>>          case HVMTRANS_gfn_paged_out:
>>          case HVMTRANS_gfn_shared:
>> -            err = ERR_PTR(~X86EMUL_RETRY);
>> -            goto out;
>> +            err = update_map_err(err, ERR_PTR(~X86EMUL_RETRY));
>> +            continue;
>>  
>>          default:
>> -            goto unhandleable;
>> +            err = update_map_err(err, ERR_PTR(~X86EMUL_UNHANDLEABLE));
>> +            continue;
>>          }
>>  
>>          *mfn++ = page_to_mfn(page);
> 
> I have to admit it find it weird that since now we don't exit the loop
> when HVMTRANS_bad_gfn_to_mfn is returned, the item at mfn[0] might
> point to the gfn -> mfn translation for the second half of the access.
> AFAICT that would happen if the first half of the access fails to
> translate with an error !HVMTRANS_bad_linear_to_gfn and the second
> half is successful.
> 
> I guess it doesn't matter much, because the vmap below will be
> skipped, might still be worth to add a comment.

I could add one, but as you say it doesn't matter much, plus - I don't
see a good place where such a comment would go.

> In fact, if the first translation fails the following ones could use
> the cheaper paging_gva_to_gfn(), as we no longer care to get the
> underlying page, and just need to figure out whether the access would
> trigger a #PF?

That would be an option, yes, at the expense of (slightly) more
complicated logic. Of course going that route would then also address
your mfn[0] remark above, without the need for any comment.

Jan