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

[Jan Beulich <JBeulich@xxxxxxxx>]

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).

Andrew, as to that old pending patch, I'm afraid I haven't been
convinced in the slightest by your argumentation, regardless of
the actual behavior of the XTF test you've created. There are
two fundamental points you've not addressed during the earlier
discussion:
1) For a guest behavior should be entirely transparent as far as
2nd level translation goes, unless the _only_ issue results from
it. That's because on bare hardware there simply is no 2nd level
translation.
2) Somewhat related, consider the case of the guest handling the
#PF on the second half of the access by a means which makes the
reason for the 2nd stage "fault" go away, or not recur. In that
case we've wrongly (i.e. at least needlessly) dealt with the 2nd
stage "fault".

I am, btw, not convinced that the behavior as you've observed it
is actually "correct" in the sense of "sensible". But the sub-
thread where I've brought up the behavior of LOCKed accesses has
been left dangling, as much as the other one, ending with me
stating that with the patch in place we'll have less "surprising"
behavior.

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