| Message ID | 20200212160918.18470-2-liuwe@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Xen on Hyper-V: Implement L0 assisted TLB flush | expand |
On Wed, Feb 12, 2020 at 04:09:15PM +0000, Wei Liu wrote: > Change hv_vp_index to use uint32_t because that is what is defined in TLFS. > > Add "_addr" suffix to hv_do_rep_hypercall's parameters. Being of type paddr_t I'm unsure the _addr suffix adds any value to the name. > > Signed-off-by: Wei Liu <liuwe@microsoft.com> LGTM (and I'm certainly not going to oppose to the _addr suffix): Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks.
> -----Original Message----- > From: Wei Liu <wei.liu.xen@gmail.com> On Behalf Of Wei Liu > Sent: 12 February 2020 17:09 > To: Xen Development List <xen-devel@lists.xenproject.org> > Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Michael Kelley > <mikelley@microsoft.com>; Wei Liu <liuwe@microsoft.com>; Wei Liu > <wl@xen.org>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com> > Subject: [PATCH 1/4] x86/hyperv: misc cleanup > > Change hv_vp_index to use uint32_t because that is what is defined in > TLFS. > > Add "_addr" suffix to hv_do_rep_hypercall's parameters. > > Signed-off-by: Wei Liu <liuwe@microsoft.com> Reviewed-by: Paul Durrant <pdurrant@amazon.com> > --- > xen/arch/x86/guest/hyperv/hyperv.c | 2 +- > xen/arch/x86/guest/hyperv/private.h | 2 +- > xen/include/asm-x86/guest/hyperv-hcall.h | 5 +++-- > 3 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/x86/guest/hyperv/hyperv.c > b/xen/arch/x86/guest/hyperv/hyperv.c > index 70f4cd5ae0..b7044f7193 100644 > --- a/xen/arch/x86/guest/hyperv/hyperv.c > +++ b/xen/arch/x86/guest/hyperv/hyperv.c > @@ -31,7 +31,7 @@ > struct ms_hyperv_info __read_mostly ms_hyperv; > DEFINE_PER_CPU_READ_MOSTLY(void *, hv_input_page); > DEFINE_PER_CPU_READ_MOSTLY(void *, hv_vp_assist); > -DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index); > +DEFINE_PER_CPU_READ_MOSTLY(uint32_t, hv_vp_index); > > static uint64_t generate_guest_id(void) > { > diff --git a/xen/arch/x86/guest/hyperv/private.h > b/xen/arch/x86/guest/hyperv/private.h > index 956eff831f..eb14ea43e5 100644 > --- a/xen/arch/x86/guest/hyperv/private.h > +++ b/xen/arch/x86/guest/hyperv/private.h > @@ -26,6 +26,6 @@ > > DECLARE_PER_CPU(void *, hv_input_page); > DECLARE_PER_CPU(void *, hv_vp_assist); > -DECLARE_PER_CPU(unsigned int, hv_vp_index); > +DECLARE_PER_CPU(uint32_t, hv_vp_index); > > #endif /* __XEN_HYPERV_PRIVIATE_H__ */ > diff --git a/xen/include/asm-x86/guest/hyperv-hcall.h b/xen/include/asm- > x86/guest/hyperv-hcall.h > index 4d3b131b3a..3396caccdd 100644 > --- a/xen/include/asm-x86/guest/hyperv-hcall.h > +++ b/xen/include/asm-x86/guest/hyperv-hcall.h > @@ -61,7 +61,8 @@ static inline uint64_t hv_do_fast_hypercall(uint16_t > code, > > static inline uint64_t hv_do_rep_hypercall(uint16_t code, uint16_t > rep_count, > uint16_t varhead_size, > - paddr_t input, paddr_t output) > + paddr_t input_addr, > + paddr_t output_addr) > { > uint64_t control = code; > uint64_t status; > @@ -71,7 +72,7 @@ static inline uint64_t hv_do_rep_hypercall(uint16_t > code, uint16_t rep_count, > control |= (uint64_t)rep_count << HV_HYPERCALL_REP_COMP_OFFSET; > > do { > - status = hv_do_hypercall(control, input, output); > + status = hv_do_hypercall(control, input_addr, output_addr); > if ( (status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS ) > break; > > -- > 2.20.1
On 12.02.2020 17:09, Wei Liu wrote: > --- a/xen/arch/x86/guest/hyperv/private.h > +++ b/xen/arch/x86/guest/hyperv/private.h > @@ -26,6 +26,6 @@ > > DECLARE_PER_CPU(void *, hv_input_page); > DECLARE_PER_CPU(void *, hv_vp_assist); > -DECLARE_PER_CPU(unsigned int, hv_vp_index); > +DECLARE_PER_CPU(uint32_t, hv_vp_index); You've got a co-maintainer ack, i.e. so be it, but FTR this is against what CodingStyle says, afaict: "Fixed width types should only be used when a fixed width quantity is meant (which for example may be a value read from or to be written to a register)." If you handed the address (perhaps indirectly, e.g. by converting to a physical one first) of this variable to Hyper-V, then things would be different. But this_cpu(hv_vp_index) = vp_index_msr; would, if unsigned int was wider than 32 bits, not cause any issues. And this is the only place the variable currently gets accessed, and I expect future uses will just be reads of it (as can be seen later in the series). Jan
On Wed, Feb 12, 2020 at 05:53:54PM +0100, Roger Pau Monné wrote: > On Wed, Feb 12, 2020 at 04:09:15PM +0000, Wei Liu wrote: > > Change hv_vp_index to use uint32_t because that is what is defined in TLFS. > > > > Add "_addr" suffix to hv_do_rep_hypercall's parameters. > > Being of type paddr_t I'm unsure the _addr suffix adds any value to > the name. > Andrew asked us to add that prefix for hv_do_hypercall. I discovered the same thing should've been done for hv_do_rep_hypercall as well. Wei.
On Thu, Feb 13, 2020 at 10:46:56AM +0100, Jan Beulich wrote: > On 12.02.2020 17:09, Wei Liu wrote: > > --- a/xen/arch/x86/guest/hyperv/private.h > > +++ b/xen/arch/x86/guest/hyperv/private.h > > @@ -26,6 +26,6 @@ > > > > DECLARE_PER_CPU(void *, hv_input_page); > > DECLARE_PER_CPU(void *, hv_vp_assist); > > -DECLARE_PER_CPU(unsigned int, hv_vp_index); > > +DECLARE_PER_CPU(uint32_t, hv_vp_index); > > You've got a co-maintainer ack, i.e. so be it, but FTR this is > against what CodingStyle says, afaict: "Fixed width types should > only be used when a fixed width quantity is meant (which for > example may be a value read from or to be written to a register)." > If you handed the address (perhaps indirectly, e.g. by converting > to a physical one first) of this variable to Hyper-V, then things > would be different. But > > this_cpu(hv_vp_index) = vp_index_msr; > > would, if unsigned int was wider than 32 bits, not cause any Did you mean "wouldn't" here? > issues. And this is the only place the variable currently gets > accessed, and I expect future uses will just be reads of it (as > can be seen later in the series). > Yes. Wei. > Jan
On 13.02.2020 13:24, Wei Liu wrote: > On Thu, Feb 13, 2020 at 10:46:56AM +0100, Jan Beulich wrote: >> On 12.02.2020 17:09, Wei Liu wrote: >>> --- a/xen/arch/x86/guest/hyperv/private.h >>> +++ b/xen/arch/x86/guest/hyperv/private.h >>> @@ -26,6 +26,6 @@ >>> >>> DECLARE_PER_CPU(void *, hv_input_page); >>> DECLARE_PER_CPU(void *, hv_vp_assist); >>> -DECLARE_PER_CPU(unsigned int, hv_vp_index); >>> +DECLARE_PER_CPU(uint32_t, hv_vp_index); >> >> You've got a co-maintainer ack, i.e. so be it, but FTR this is >> against what CodingStyle says, afaict: "Fixed width types should >> only be used when a fixed width quantity is meant (which for >> example may be a value read from or to be written to a register)." >> If you handed the address (perhaps indirectly, e.g. by converting >> to a physical one first) of this variable to Hyper-V, then things >> would be different. But >> >> this_cpu(hv_vp_index) = vp_index_msr; >> >> would, if unsigned int was wider than 32 bits, not cause any > > Did you mean "wouldn't" here? I don't think so - the not comes after the second comma. Jan
diff --git a/xen/arch/x86/guest/hyperv/hyperv.c b/xen/arch/x86/guest/hyperv/hyperv.c index 70f4cd5ae0..b7044f7193 100644 --- a/xen/arch/x86/guest/hyperv/hyperv.c +++ b/xen/arch/x86/guest/hyperv/hyperv.c @@ -31,7 +31,7 @@ struct ms_hyperv_info __read_mostly ms_hyperv; DEFINE_PER_CPU_READ_MOSTLY(void *, hv_input_page); DEFINE_PER_CPU_READ_MOSTLY(void *, hv_vp_assist); -DEFINE_PER_CPU_READ_MOSTLY(unsigned int, hv_vp_index); +DEFINE_PER_CPU_READ_MOSTLY(uint32_t, hv_vp_index); static uint64_t generate_guest_id(void) { diff --git a/xen/arch/x86/guest/hyperv/private.h b/xen/arch/x86/guest/hyperv/private.h index 956eff831f..eb14ea43e5 100644 --- a/xen/arch/x86/guest/hyperv/private.h +++ b/xen/arch/x86/guest/hyperv/private.h @@ -26,6 +26,6 @@ DECLARE_PER_CPU(void *, hv_input_page); DECLARE_PER_CPU(void *, hv_vp_assist); -DECLARE_PER_CPU(unsigned int, hv_vp_index); +DECLARE_PER_CPU(uint32_t, hv_vp_index); #endif /* __XEN_HYPERV_PRIVIATE_H__ */ diff --git a/xen/include/asm-x86/guest/hyperv-hcall.h b/xen/include/asm-x86/guest/hyperv-hcall.h index 4d3b131b3a..3396caccdd 100644 --- a/xen/include/asm-x86/guest/hyperv-hcall.h +++ b/xen/include/asm-x86/guest/hyperv-hcall.h @@ -61,7 +61,8 @@ static inline uint64_t hv_do_fast_hypercall(uint16_t code, static inline uint64_t hv_do_rep_hypercall(uint16_t code, uint16_t rep_count, uint16_t varhead_size, - paddr_t input, paddr_t output) + paddr_t input_addr, + paddr_t output_addr) { uint64_t control = code; uint64_t status; @@ -71,7 +72,7 @@ static inline uint64_t hv_do_rep_hypercall(uint16_t code, uint16_t rep_count, control |= (uint64_t)rep_count << HV_HYPERCALL_REP_COMP_OFFSET; do { - status = hv_do_hypercall(control, input, output); + status = hv_do_hypercall(control, input_addr, output_addr); if ( (status & HV_HYPERCALL_RESULT_MASK) != HV_STATUS_SUCCESS ) break;
Change hv_vp_index to use uint32_t because that is what is defined in TLFS. Add "_addr" suffix to hv_do_rep_hypercall's parameters. Signed-off-by: Wei Liu <liuwe@microsoft.com> --- xen/arch/x86/guest/hyperv/hyperv.c | 2 +- xen/arch/x86/guest/hyperv/private.h | 2 +- xen/include/asm-x86/guest/hyperv-hcall.h | 5 +++-- 3 files changed, 5 insertions(+), 4 deletions(-)