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

[Alexandru Stefan ISAILA <aisaila@bitdefender.com>]

On 30.07.2019 17:54, Jan Beulich wrote:
> On 30.07.2019 16:12, Alexandru Stefan ISAILA wrote:
>>
>>
>> On 30.07.2019 16:27, Jan Beulich wrote:
>>> On 30.07.2019 14:21, Alexandru Stefan ISAILA wrote:
>>>>
>>>>>>>>> @@ -629,6 +697,14 @@ static void *hvmemul_map_linear_addr(
>>>>>>>>>           
>>>>>>>>>                       ASSERT(p2mt == p2m_ram_logdirty || !p2m_is_readonly(p2mt));
>>>>>>>>>                   }
>>>>>>>>> +
>>>>>>>>> +        if ( curr->arch.vm_event &&
>>>>>>>>> +            curr->arch.vm_event->send_event &&
>>>>>>>>> +            hvm_emulate_send_vm_event(addr, gfn, pfec) )
>>>>>>>>> +        {
>>>>>>>>> +            err = ERR_PTR(~X86EMUL_RETRY);
>>>>>>>>> +            goto out;
>>>>>>>>> +        }
>>>>>>>>
>>>>>>>> Did you notice that there's an immediate exit from the loop only
>>>>>>>> in case the linear -> physical translation fails? This is
>>>>>>>> relevant for page fault delivery correctness for accesses
>>>>>>>> crossing page boundaries. I think you want to use
>>>>>>>> update_map_err() and drop the "goto out". I can't really make up
>>>>>>>
>>>>>>> By update_map_err() are you saying to have the err var assigned and then
>>>>>>> drop "goto out"? If so how do I keep the err from my access violation
>>>>>>> without exiting from the loop?
>>>>>>
>>>>>> Counter question: Why do you _need_ to keep "your" value of err?
>>>>>> If, just as an example, there's going to be a #PF on the other
>>>>>> half of the access, then "your" access violation is of no interest
>>>>>> at all.
>>>>>
>>>>> You are right, there is no need to keep the "goto out" here. It was just
>>>>> for optimization in the idea that there is no need to do further steps
>>>>> but I can drop the "goto out" and the code will work the same.
>>>>>
>>>>
>>>> There is a problem with dropping the "goto out". If everything goes fine
>>>> then it will return the mapping and I don't want that. This can be
>>>> stopped by checking if ( err ) after the loop and it is not null then
>>>> goto out. And going with this idea I can init *err = NULL and drop the
>>>> err = NULL from hvmemul_map_linear_addr(). Is this ok for the next version?
>>>
>>> I'd prefer to see the code to decide. If you want this settled before
>>> sending the next full version, then please send at least the resulting
>>> patch hunk(s).
>>>
>>
>> Here is a diff for hvmemul_map_linear_addr():
>>
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -543,10 +543,11 @@ static void *hvmemul_map_linear_addr(
>>         struct hvm_emulate_ctxt *hvmemul_ctxt)
>>     {
>>         struct vcpu *curr = current;
>> -    void *err, *mapping;
>> +    void *err = NULL, *mapping;
>>         unsigned int nr_frames = ((linear + bytes - !!bytes) >> PAGE_SHIFT) -
>>             (linear >> PAGE_SHIFT) + 1;
>>         unsigned int i;
>> +    gfn_t gfn;
>>
>>         /*
>>          * mfn points to the next free slot.  All used slots have a page
>> reference
>> @@ -585,7 +586,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);
>>
>>             switch ( res )
>>             {
>> @@ -599,7 +600,6 @@ static void *hvmemul_map_linear_addr(
>>                 goto out;
>>
>>             case HVMTRANS_bad_gfn_to_mfn:
>> -            err = NULL;
>>                 goto out;
>>
>>             case HVMTRANS_gfn_paged_out:
>> @@ -622,14 +622,22 @@ static void *hvmemul_map_linear_addr(
>>                 }
>>
>>                 if ( p2mt == p2m_ioreq_server )
>> -            {
>> -                err = NULL;
>>                     goto out;
>> -            }
>>
>>                 ASSERT(p2mt == p2m_ram_logdirty || !p2m_is_readonly(p2mt));
>> +
>> +            if ( curr->arch.vm_event &&
>> +                 curr->arch.vm_event->send_event &&
>> +                 hvm_emulate_send_vm_event(addr, gfn, pfec) )
>> +                err = ERR_PTR(~X86EMUL_RETRY);
>>             }
>>         }
>> +    /* Check if any vm_event was sent */
>> +    if ( err )
>> +        goto out;
>>
>>         /* Entire access within a single frame? */
>>         if ( nr_frames == 1 )
> 
> First of all I have to apologize: In earlier replies I referred
> to update_map_err(). I notice only now that this is a still
> pending change of mine, which Andrew continues to object to,
> while I continue to think it (in one form or another) is needed:
> https://lists.xenproject.org/archives/html/xen-devel/2018-09/msg01250.html
> 
> Given the unpatched code, I think your change is correct, but
> quite possibly your earlier variant was, too. But since the
> unpatched code is imo wrong, I'd prefer if the VM event side
> change was put on top of the fixed code, in order to not further
> complicate the actual fix (which we may also want to backport).

Thanks for the clarification, I will have to wait for this patch to go 
in and then submit another version. This new function will work with my 
new error.

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