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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

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():


diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index d75d3e6fd6..49dbfa730c 100644
--- 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 )


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