| Message ID | 90e7ed4e-aff5-1b0b-e3a8-fbb4c058a7d1@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/HVM: implement memory read caching | expand |
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>
> -----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 = ¤t->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
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 = ¤t->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
--- 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 = ¤t->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 = ¤t->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);
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.