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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 17.09.2019 17:32, Jan Beulich wrote:
> 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_.
> 

There is no problem, I understand the risk of having suspicious return 
values. I am not hanged on having this return. I used this to skip 
adding a new return value. I can do this if we agree on a suitable name 
for a new return value in enum hvm_translation_result. I can propose 
HVMTRANS_bad_gfn_access.

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