Re: [Xen-devel] [PATCH 1/4] x86/hyperv: misc cleanup
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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/4] x86/hyperv: misc cleanup
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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/4] x86/hyperv: misc cleanup
On Wed, Feb 12, 2020 at 05:53:54PM +0100, Roger Pau Monné wrote: > On Wed, Feb 12, 2020 at 04:09:15PM +, 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. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/4] x86/hyperv: misc cleanup
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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/4] x86/hyperv: misc cleanup
> -Original Message-
> From: Wei Liu On Behalf Of Wei Liu
> Sent: 12 February 2020 17:09
> To: Xen Development List
> Cc: Durrant, Paul ; Michael Kelley
> ; Wei Liu ; Wei Liu
> ; Jan Beulich ; Andrew Cooper
> ; Roger Pau Monné
> 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
Reviewed-by: Paul Durrant
> ---
> 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
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/4] x86/hyperv: misc cleanup
On Wed, Feb 12, 2020 at 04:09:15PM +, 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 LGTM (and I'm certainly not going to oppose to the _addr suffix): Reviewed-by: Roger Pau Monné Thanks. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
