Re: [Xen-devel] [PATCH v4 3/7] x86/HVM: introduce "curr" into hvmemul_rep_{mov, sto}s()

[Jan Beulich]

On 06.02.2020 14:26, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-boun...@lists.xenproject.org> On Behalf Of Jan
>> Beulich
>> Sent: 31 January 2020 16:43
>> To: xen-devel@lists.xenproject.org
>> Cc: George Dunlap <george.dun...@eu.citrix.com>; Andrew Cooper
>> <andrew.coop...@citrix.com>; Roger Pau Monné <roger....@citrix.com>; Wei
>> Liu <w...@xen.org>; Paul Durrant <p...@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 <jbeul...@suse.com>
> 
> Reviewed-by: Paul Durrant <pdurr...@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

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