Re: [Xen-devel] [PATCH v5 4/4] x86/HVM: __hvm_copy()'s size parameter is an unsigned quantity

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan 
> Beulich
> Sent: 03 March 2020 10:20
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné 
> <roger.pau@xxxxxxxxxx>; Wei Liu
> <wl@xxxxxxx>; Paul Durrant <paul@xxxxxxx>
> Subject: [EXTERNAL][Xen-devel] [PATCH v5 4/4] x86/HVM: __hvm_copy()'s size 
> parameter is an unsigned
> quantity
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> There are no negative sizes. Make the function's parameter as well as
> that of its derivates "unsigned int". Similarly make its local "count"
> variable "unsigned int", and drop "todo" altogether. Don't use min_t()
> anymore to calculate "count". Restrict its scope as well as that of
> other local variables of the function.
> 
> While at it I've also noticed that {copy_{from,to},clear}_user_hvm()
> have been returning "unsigned long" for no apparent reason, as their
> respective "size" parameters have already been "unsigned int". Adjust
> this as well as a slightly wrong comment there at the same time.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Paul Durrant <pdurrant@xxxxxxxx>

> ---
> v5: New.
> 
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3249,14 +3249,9 @@ enum hvm_translation_result hvm_translat
>  #define HVMCOPY_phys       (0u<<2)
>  #define HVMCOPY_linear     (1u<<2)
>  static enum hvm_translation_result __hvm_copy(
> -    void *buf, paddr_t addr, int size, struct vcpu *v, unsigned int flags,
> +    void *buf, paddr_t addr, unsigned int size, struct vcpu *v, unsigned int 
> flags,
>      uint32_t pfec, pagefault_info_t *pfinfo)
>  {
> -    gfn_t gfn;
> -    struct page_info *page;
> -    p2m_type_t p2mt;
> -    int count, todo = size;
> -
>      ASSERT(is_hvm_vcpu(v));
> 
>      /*
> @@ -3275,12 +3270,14 @@ static enum hvm_translation_result __hvm
>          return HVMTRANS_unhandleable;
>  #endif
> 
> -    while ( todo > 0 )
> +    while ( size > 0 )

'while ( size )'

ought to do.

>      {
> +        struct page_info *page;
> +        gfn_t gfn;
> +        p2m_type_t p2mt;
>          enum hvm_translation_result res;
>          unsigned int pgoff = addr & ~PAGE_MASK;
> -
> -        count = min_t(int, PAGE_SIZE - pgoff, todo);
> +        unsigned int count = min((unsigned int)PAGE_SIZE - pgoff, size);
> 
>          res = hvm_translate_get_page(v, addr, flags & HVMCOPY_linear,
>                                       pfec, pfinfo, &page, &gfn, &p2mt);
> @@ -3336,7 +3333,7 @@ static enum hvm_translation_result __hvm
>          addr += count;
>          if ( buf )
>              buf += count;
> -        todo -= count;
> +        size -= count;
>          put_page(page);
>      }
> 
> @@ -3344,21 +3341,21 @@ static enum hvm_translation_result __hvm
>  }
> 
>  enum hvm_translation_result hvm_copy_to_guest_phys(
> -    paddr_t paddr, void *buf, int size, struct vcpu *v)
> +    paddr_t paddr, void *buf, unsigned int size, struct vcpu *v)
>  {
>      return __hvm_copy(buf, paddr, size, v,
>                        HVMCOPY_to_guest | HVMCOPY_phys, 0, NULL);
>  }
> 
>  enum hvm_translation_result hvm_copy_from_guest_phys(
> -    void *buf, paddr_t paddr, int size)
> +    void *buf, paddr_t paddr, unsigned int size)
>  {
>      return __hvm_copy(buf, paddr, size, current,
>                        HVMCOPY_from_guest | HVMCOPY_phys, 0, NULL);
>  }
> 
>  enum hvm_translation_result hvm_copy_to_guest_linear(
> -    unsigned long addr, void *buf, int size, uint32_t pfec,
> +    unsigned long addr, void *buf, unsigned int size, uint32_t pfec,
>      pagefault_info_t *pfinfo)
>  {
>      return __hvm_copy(buf, addr, size, current,
> @@ -3367,7 +3364,7 @@ enum hvm_translation_result hvm_copy_to_
>  }
> 
>  enum hvm_translation_result hvm_copy_from_guest_linear(
> -    void *buf, unsigned long addr, int size, uint32_t pfec,
> +    void *buf, unsigned long addr, unsigned int size, uint32_t pfec,
>      pagefault_info_t *pfinfo)
>  {
>      return __hvm_copy(buf, addr, size, current,
> @@ -3375,7 +3372,7 @@ enum hvm_translation_result hvm_copy_fro
>                        PFEC_page_present | pfec, pfinfo);
>  }
> 
> -unsigned long copy_to_user_hvm(void *to, const void *from, unsigned int len)
> +unsigned int copy_to_user_hvm(void *to, const void *from, unsigned int len)
>  {
>      int rc;
> 
> @@ -3389,7 +3386,7 @@ unsigned long copy_to_user_hvm(void *to,
>      return rc ? len : 0; /* fake a copy_to_user() return code */
>  }
> 
> -unsigned long clear_user_hvm(void *to, unsigned int len)
> +unsigned int clear_user_hvm(void *to, unsigned int len)
>  {
>      int rc;
> 
> @@ -3400,10 +3397,11 @@ unsigned long clear_user_hvm(void *to, u
>      }
> 
>      rc = hvm_copy_to_guest_linear((unsigned long)to, NULL, len, 0, NULL);
> -    return rc ? len : 0; /* fake a copy_to_user() return code */
> +
> +    return rc ? len : 0; /* fake a clear_user() return code */
>  }
> 
> -unsigned long copy_from_user_hvm(void *to, const void *from, unsigned len)
> +unsigned int copy_from_user_hvm(void *to, const void *from, unsigned int len)
>  {
>      int rc;
> 
> --- a/xen/include/asm-x86/hvm/guest_access.h
> +++ b/xen/include/asm-x86/hvm/guest_access.h
> @@ -1,8 +1,8 @@
>  #ifndef __ASM_X86_HVM_GUEST_ACCESS_H__
>  #define __ASM_X86_HVM_GUEST_ACCESS_H__
> 
> -unsigned long copy_to_user_hvm(void *to, const void *from, unsigned len);
> -unsigned long clear_user_hvm(void *to, unsigned int len);
> -unsigned long copy_from_user_hvm(void *to, const void *from, unsigned len);
> +unsigned int copy_to_user_hvm(void *to, const void *from, unsigned int len);
> +unsigned int clear_user_hvm(void *to, unsigned int len);
> +unsigned int copy_from_user_hvm(void *to, const void *from, unsigned int 
> len);
> 
>  #endif /* __ASM_X86_HVM_GUEST_ACCESS_H__ */
> --- a/xen/include/asm-x86/hvm/support.h
> +++ b/xen/include/asm-x86/hvm/support.h
> @@ -70,9 +70,9 @@ enum hvm_translation_result {
>   * address range does not map entirely onto ordinary machine memory.
>   */
>  enum hvm_translation_result hvm_copy_to_guest_phys(
> -    paddr_t paddr, void *buf, int size, struct vcpu *v);
> +    paddr_t paddr, void *buf, unsigned int size, struct vcpu *v);
>  enum hvm_translation_result hvm_copy_from_guest_phys(
> -    void *buf, paddr_t paddr, int size);
> +    void *buf, paddr_t paddr, unsigned int size);
> 
>  /*
>   * Copy to/from a guest linear address. @pfec should include PFEC_user_mode
> @@ -96,10 +96,10 @@ typedef struct pagefault_info
>  } pagefault_info_t;
> 
>  enum hvm_translation_result hvm_copy_to_guest_linear(
> -    unsigned long addr, void *buf, int size, uint32_t pfec,
> +    unsigned long addr, void *buf, unsigned int size, uint32_t pfec,
>      pagefault_info_t *pfinfo);
>  enum hvm_translation_result hvm_copy_from_guest_linear(
> -    void *buf, unsigned long addr, int size, uint32_t pfec,
> +    void *buf, unsigned long addr, unsigned int size, uint32_t pfec,
>      pagefault_info_t *pfinfo);
> 
>  /*
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel