[v4,3/7] x86/HVM: introduce "curr" into hvmemul_rep_{mov, sto}s()
diff mbox series

Message ID 90e7ed4e-aff5-1b0b-e3a8-fbb4c058a7d1@suse.com
State Superseded
Headers show
Series
  • x86/HVM: implement memory read caching
Related show

Commit Message

Jan Beulich Jan. 31, 2020, 4:43 p.m. UTC
There are a number of uses of "current" already, and more may appear
down the road. Latch into a local variable.

At this occasion also drop stray casts from code getting touched anyway.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v4: New.

Comments

Andrew Cooper Feb. 3, 2020, 3:05 p.m. UTC | #1
On 31/01/2020 16:43, Jan Beulich wrote:
> There are a number of uses of "current" already, and more may appear
> down the road. Latch into a local variable.
>
> At this occasion also drop stray casts from code getting touched anyway.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Durrant, Paul Feb. 6, 2020, 1:26 p.m. UTC | #2
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan
> Beulich
> Sent: 31 January 2020 16:43
> To: xen-devel@lists.xenproject.org
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; Wei
> Liu <wl@xen.org>; Paul Durrant <paul@xen.org>
> Subject: [Xen-devel] [PATCH v4 3/7] x86/HVM: introduce "curr" into
> hvmemul_rep_{mov, sto}s()
> 
> There are a number of uses of "current" already, and more may appear
> down the road. Latch into a local variable.
> 
> At this occasion also drop stray casts from code getting touched anyway.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Paul Durrant <pdurrant@amazon.com>

...with one suggestion:

> ---
> v4: New.
> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1747,7 +1747,8 @@ static int hvmemul_rep_movs(
>  {
>      struct hvm_emulate_ctxt *hvmemul_ctxt =
>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> -    struct hvm_vcpu_io *vio = &current->arch.hvm.hvm_io;
> +    struct vcpu *curr = current;
> +    struct hvm_vcpu_io *vio = &curr->arch.hvm.hvm_io;
>      unsigned long saddr, daddr, bytes;
>      paddr_t sgpa, dgpa;
>      uint32_t pfec = PFEC_page_present;
> @@ -1807,8 +1808,8 @@ static int hvmemul_rep_movs(
>      }
> 
>      /* Check for MMIO ops */
> -    (void) get_gfn_query_unlocked(current->domain, sgpa >> PAGE_SHIFT,
> &sp2mt);
> -    (void) get_gfn_query_unlocked(current->domain, dgpa >> PAGE_SHIFT,
> &dp2mt);
> +    get_gfn_query_unlocked(curr->domain, sgpa >> PAGE_SHIFT, &sp2mt);
> +    get_gfn_query_unlocked(curr->domain, dgpa >> PAGE_SHIFT, &dp2mt);

Since we have more than one 'curr->domain', I think it would be nice to a latched 'currd' domain pointer too.

  Paul
Jan Beulich Feb. 6, 2020, 1:35 p.m. UTC | #3
On 06.02.2020 14:26, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan
>> Beulich
>> Sent: 31 January 2020 16:43
>> To: xen-devel@lists.xenproject.org
>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; Wei
>> Liu <wl@xen.org>; Paul Durrant <paul@xen.org>
>> Subject: [Xen-devel] [PATCH v4 3/7] x86/HVM: introduce "curr" into
>> hvmemul_rep_{mov, sto}s()
>>
>> There are a number of uses of "current" already, and more may appear
>> down the road. Latch into a local variable.
>>
>> At this occasion also drop stray casts from code getting touched anyway.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Paul Durrant <pdurrant@amazon.com>

Thanks.

> ...with one suggestion:
> 
>> ---
>> v4: New.
>>
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -1747,7 +1747,8 @@ static int hvmemul_rep_movs(
>>  {
>>      struct hvm_emulate_ctxt *hvmemul_ctxt =
>>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>> -    struct hvm_vcpu_io *vio = &current->arch.hvm.hvm_io;
>> +    struct vcpu *curr = current;
>> +    struct hvm_vcpu_io *vio = &curr->arch.hvm.hvm_io;
>>      unsigned long saddr, daddr, bytes;
>>      paddr_t sgpa, dgpa;
>>      uint32_t pfec = PFEC_page_present;
>> @@ -1807,8 +1808,8 @@ static int hvmemul_rep_movs(
>>      }
>>
>>      /* Check for MMIO ops */
>> -    (void) get_gfn_query_unlocked(current->domain, sgpa >> PAGE_SHIFT,
>> &sp2mt);
>> -    (void) get_gfn_query_unlocked(current->domain, dgpa >> PAGE_SHIFT,
>> &dp2mt);
>> +    get_gfn_query_unlocked(curr->domain, sgpa >> PAGE_SHIFT, &sp2mt);
>> +    get_gfn_query_unlocked(curr->domain, dgpa >> PAGE_SHIFT, &dp2mt);
> 
> Since we have more than one 'curr->domain', I think it would be nice to
> a latched 'currd' domain pointer too.

I did actually consider it, but didn't do so because (a) it's just
two references (which means the variable is liable to end up in
memory, yielding no improvement) and (b) it's better in sync with
the stos counterpart of the function this way.

Jan

Patch
diff mbox series

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1747,7 +1747,8 @@  static int hvmemul_rep_movs(
 {
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
-    struct hvm_vcpu_io *vio = &current->arch.hvm.hvm_io;
+    struct vcpu *curr = current;
+    struct hvm_vcpu_io *vio = &curr->arch.hvm.hvm_io;
     unsigned long saddr, daddr, bytes;
     paddr_t sgpa, dgpa;
     uint32_t pfec = PFEC_page_present;
@@ -1807,8 +1808,8 @@  static int hvmemul_rep_movs(
     }
 
     /* Check for MMIO ops */
-    (void) get_gfn_query_unlocked(current->domain, sgpa >> PAGE_SHIFT, &sp2mt);
-    (void) get_gfn_query_unlocked(current->domain, dgpa >> PAGE_SHIFT, &dp2mt);
+    get_gfn_query_unlocked(curr->domain, sgpa >> PAGE_SHIFT, &sp2mt);
+    get_gfn_query_unlocked(curr->domain, dgpa >> PAGE_SHIFT, &dp2mt);
 
     if ( sp2mt == p2m_mmio_direct || dp2mt == p2m_mmio_direct ||
          (sp2mt == p2m_mmio_dm && dp2mt == p2m_mmio_dm) )
@@ -1873,7 +1874,7 @@  static int hvmemul_rep_movs(
         rc = hvm_copy_from_guest_phys(buf, sgpa, bytes);
 
     if ( rc == HVMTRANS_okay )
-        rc = hvm_copy_to_guest_phys(dgpa, buf, bytes, current);
+        rc = hvm_copy_to_guest_phys(dgpa, buf, bytes, curr);
 
     xfree(buf);
 
@@ -1910,7 +1911,8 @@  static int hvmemul_rep_stos(
 {
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
-    struct hvm_vcpu_io *vio = &current->arch.hvm.hvm_io;
+    struct vcpu *curr = current;
+    struct hvm_vcpu_io *vio = &curr->arch.hvm.hvm_io;
     unsigned long addr, bytes;
     paddr_t gpa;
     p2m_type_t p2mt;
@@ -1943,7 +1945,7 @@  static int hvmemul_rep_stos(
     }
 
     /* Check for MMIO op */
-    (void)get_gfn_query_unlocked(current->domain, gpa >> PAGE_SHIFT, &p2mt);
+    get_gfn_query_unlocked(curr->domain, gpa >> PAGE_SHIFT, &p2mt);
 
     switch ( p2mt )
     {
@@ -1992,7 +1994,7 @@  static int hvmemul_rep_stos(
         if ( df )
             gpa -= bytes - bytes_per_rep;
 
-        rc = hvm_copy_to_guest_phys(gpa, buf, bytes, current);
+        rc = hvm_copy_to_guest_phys(gpa, buf, bytes, curr);
 
         if ( buf != p_data )
             xfree(buf);