| Message ID | 93366e9e-1afb-296d-6280-5d375869410e@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/HVM: implement memory read caching | expand |
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich > Sent: 03 March 2020 10:20 > To: xen-devel@lists.xenproject.org > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Roger Pau Monné <roger.pau@citrix.com>; Wei Liu > <wl@xen.org>; Paul Durrant <paul@xen.org> > 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@suse.com> Reviewed-by: Paul Durrant <pdurrant@amzn.com> > --- > 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@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
--- 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 ) { + 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); /*
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@suse.com> --- v5: New.