Re: [Xen-devel] [PATCH v13] x86/emulate: Send vm_event from emulate

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 23.09.2019 16:43, Jan Beulich wrote:
> On 23.09.2019 14:05, Alexandru Stefan ISAILA wrote:
>> @@ -599,8 +600,15 @@ static void *hvmemul_map_linear_addr(
>>               err = NULL;
>>               goto out;
>>   
>> -        case HVMTRANS_gfn_paged_out:
>> +        case HVMTRANS_need_retry:
>> +            /*
>> +             * hvm_translate_get_page() does not return HVMTRANS_need_retry.
>> +             * It can dropped if future work requires this.
>> +             */
> 
> To me, "it" in this comment can only refer to something mentioned in
> the prior sentence. But to be honest I'd drop the 2nd sentence
> altogether, adding "currently" to the 1st one. (Same further down
> then.)
> 
>> +            ASSERT_UNREACHABLE();
>> +            /* fall through */
>>           case HVMTRANS_gfn_shared:
>> +        case HVMTRANS_gfn_paged_out:
>>               err = ERR_PTR(~X86EMUL_RETRY);
>>               goto out;
> 
> It also escapes me why you felt like moving the
> "case HVMTRANS_gfn_paged_out:" line.
> 
>> @@ -1852,19 +1870,27 @@ static int hvmemul_rep_movs(
>>   
>>       xfree(buf);
>>   
>> -    if ( rc == HVMTRANS_gfn_paged_out )
>> -        return X86EMUL_RETRY;
>> -    if ( rc == HVMTRANS_gfn_shared )
>> -        return X86EMUL_RETRY;
>> -    if ( rc != HVMTRANS_okay )
>> +    switch ( rc )
>>       {
>> -        gdprintk(XENLOG_WARNING, "Failed memory-to-memory REP MOVS: sgpa=%"
>> -                 PRIpaddr" dgpa=%"PRIpaddr" reps=%lu bytes_per_rep=%u\n",
>> -                 sgpa, dgpa, *reps, bytes_per_rep);
>> -        return X86EMUL_UNHANDLEABLE;
>> +    case HVMTRANS_need_retry:
>> +        /*
>> +         * hvm_copy_to_guest_phys() does not return HVMTRANS_need_retry.
>> +         * It can dropped if future work requires this.
>> +         */
> 
> Unlike in its rep_stos counterpart, here the return value may come
> from hvm_copy_from_guest_phys() or hvm_copy_to_guest_phys(), and I
> think the comment should not say otherwise.
> 
> With these changes (which are of enough of a cosmetic nature that
> they could probably be taken care of while committing, provided
> you agree), non-monitor-specific parts
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 

I agree, thanks for the review and the help with this patch

Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel