On 20.09.2019 17:22, Jan Beulich wrote:
> On 20.09.2019 14:16, Alexandru Stefan ISAILA wrote:
>> In order to have __hvm_copy() issue ~X86EMUL_RETRY a new return type,
>> HVMTRANS_need_retry, was added and all the places that consume HVMTRANS*
>> and needed adjustment where changed accordingly.
>
> This is wrong and hence confusing: __hvm_copy() would never return
> ~X86EMUL_RETRY. In fact I think you've confused yourself enough to
> make a questionable (possibly resulting) change:
The idea was to get X86EMUL_RETRY down the line from __hvm_copy().
I will adjust this.
>
>> @@ -582,7 +583,7 @@ static void *hvmemul_map_linear_addr(
>> ASSERT(mfn_x(*mfn) == 0);
>>
>> res = hvm_translate_get_page(curr, addr, true, pfec,
>> - &pfinfo, &page, NULL, &p2mt);
>> + &pfinfo, &page, &gfn, &p2mt);
>
> This function ...
>
>> switch ( res )
>> {
>> @@ -601,6 +602,7 @@ static void *hvmemul_map_linear_addr(
>>
>> case HVMTRANS_gfn_paged_out:
>> case HVMTRANS_gfn_shared:
>> + case HVMTRANS_need_retry:
>
> ... can't return this value, so you should omit this addition,
> letting the new return value go through "default:".
It is very clear that HVMTRANS_need_retry will not be returned form that
function. At least for now. I thought you wanted to have every possible
case covered in the switch. I can remove that case, there is not problem
here because, like I've said, it will never enter that case.
But as you said later work with HVMTRANS_need_retry will result in
returning X86EMUL_RETRY. Any way it's your call if I should remove it or
not.
>
>> @@ -1852,6 +1864,8 @@ static int hvmemul_rep_movs(
>>
>> xfree(buf);
>>
>> + ASSERT(rc != HVMTRANS_need_retry);
>> +
>> if ( rc == HVMTRANS_gfn_paged_out )
>> return X86EMUL_RETRY;
>> if ( rc == HVMTRANS_gfn_shared )
>> @@ -1964,6 +1978,8 @@ static int hvmemul_rep_stos(
>> if ( buf != p_data )
>> xfree(buf);
>>
>> + ASSERT(rc != HVMTRANS_need_retry);
>> +
>> switch ( rc )
>> {
>> case HVMTRANS_gfn_paged_out:
>
> Looking at this again, I think it would better be an addition to
> the switch() (using ASSERT_UNREACHABLE()). Generally this is
> true for the rep_movs case as well, but that one would first
> need converting to switch(), which I agree is beyond the scope
I agree that this is beyond the scope of this patch but it's not that
big of a change and it can be done.
But isn't having a default ASSERT_UNREACHABLE(); in both switch cases
change the behavior of the function?
> of this change. In both cases a brief comment would seem
> worthwhile adding, clarifying that the new return value can
> result from hvm_copy_*_guest_linear() only. This might become
> relevant in particular if, down the road, we invent more cases
> where HVMTRANS_need_retry is produced.
Is this comment aimed for the commit message or another place?
>
> Then again maybe switching rep_movs to switch() would still be
> a good thing to do here: Don't you agree that from an abstract
> pov in both cases above X86EMUL_RETRY should be produced, if at
> a future point physical accesses could also produce
> HVMTRANS_need_retry? With this retaining the assertions is
> certainly an option, but I think the fallback return value for
> this case should still be X86EMUL_RETRY.
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
