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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.09.2019 16:11, Alexandru Stefan ISAILA wrote:
> 
> 
> On 17.09.2019 11:09, Jan Beulich wrote:
>> On 17.09.2019 09:52, Alexandru Stefan ISAILA wrote:
>>> On 16.09.2019 18:58, Jan Beulich wrote:
>>>> On 16.09.2019 10:10, Alexandru Stefan ISAILA wrote:
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -3224,6 +3224,14 @@ static enum hvm_translation_result __hvm_copy(
>>>>>                return HVMTRANS_bad_gfn_to_mfn;
>>>>>            }
>>>>>    
>>>>> +        if ( unlikely(v->arch.vm_event) &&
>>>>> +             v->arch.vm_event->send_event &&
>>>>> +             hvm_monitor_check_p2m(addr, gfn, pfec, npfec_kind_with_gla) 
>>>>> )
>>>>> +        {
>>>>> +            put_page(page);
>>>>> +            return HVMTRANS_gfn_paged_out;
>>>>
>>>> I'm sorry, but there is _still_ no comment next to this apparent
>>>> mis-use of HVMTRANS_gfn_paged_out.
>>>
>>> I will add this comment here:
>>>
>>> "/*
>>>     * In case a vm event was sent return paged_out so the emulation will
>>>     * stop with no side effect
>>>     */"
>>
>> First of all - why "was sent"? The event is yet to be sent, isn't it?
> 
> Yes it should state "if the event is sent".

"is sent" is still not indicating this is something yet to happen.
"is to be sent" would be to me (caveat - I'm not a native speaker).

>> And then I'm afraid this still isn't enough. __hvm_copy() gets used
>> for many purposes. For example, while looking into this again when
>> preparing the reply here, I've noticed that above you may wrongly
>> call hvm_monitor_check_p2m() with npfec_kind_with_gla - there's no
>> linear address when HVMCOPY_linear is not set. If, while putting
> 
> You are right, a check for HVMCOPY_linear should go in the if so we are 
> sure that it is called with a linear address only.
> 
>> together what the comment needs to explain (i.e. everything that
>> can't be implied from the code you add), you considered all cases
>> you should have noticed this yourself.
> 
> With this new check in place (HVMCOPY_linear) __hvm_copy() will be 
> called from linear_read() linear_write() where it will pass down 
> X86EMUL_RETRY.
> 
> The comment can change to:
> "If a event is sent return paged_out. The emulation will have no side 
> effect and will return X86EMUL_RETRY"

I'm sorry to be a pain in your neck, but no, this still is not
sufficient imo. The comment, whatever wording you choose,
should make clear that you have understood the possible effects
of using a suspicious return value, and it should also make
clear to readers that this is in fact not going to cause a
problem _for any caller_.

Jan

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