[Public] > -----Original Message----- > From: Jan Beulich <[email protected]> > Sent: Tuesday, November 11, 2025 4:14 PM > To: Penny, Zheng <[email protected]> > Cc: Huang, Ray <[email protected]>; [email protected]; Andrew > Cooper <[email protected]>; Roger Pau Monné <[email protected]>; > Anthony PERARD <[email protected]>; Orzel, Michal > <[email protected]>; Julien Grall <[email protected]>; Stefano Stabellini > <[email protected]>; Alexandru Isaila <[email protected]>; Petre > Pircalabu <[email protected]>; Daniel P. Smith > <[email protected]>; [email protected]; Tamas K > Lengyel <[email protected]> > Subject: Re: [PATCH v3 09/28] xen/vm_event: consolidate CONFIG_VM_EVENT > > On 11.11.2025 08:08, Penny, Zheng wrote: > > [Public] > > > > Hi, > > > > Sorry for the late response. Just got back from long annual leaves > > > >> -----Original Message----- > >>> --- a/xen/arch/x86/include/asm/mem_access.h > >>> +++ b/xen/arch/x86/include/asm/mem_access.h > >>> @@ -14,6 +14,7 @@ > >>> #ifndef __ASM_X86_MEM_ACCESS_H__ > >>> #define __ASM_X86_MEM_ACCESS_H__ > >>> > >>> +#ifdef CONFIG_VM_EVENT > >>> /* > >>> * Setup vm_event request based on the access (gla is -1ull if not > >>> available). > >>> * Handles the rw2rx conversion. Boolean return value indicates if > >>> event type @@ -25,6 +26,14 @@ bool p2m_mem_access_check(paddr_t > >>> gpa, unsigned long gla, > >>> struct npfec npfec, > >>> struct vm_event_st **req_ptr); > >>> +#else > >>> +static inline bool p2m_mem_access_check(paddr_t gpa, unsigned long gla, > >>> + struct npfec npfec, > >>> + struct vm_event_st > >>> +**req_ptr) { > >>> + return false; > >> > >> Leaving *req_ptr untouched feels dangerous; the fact that the sole > >> caller has what it uses set to NULL up front is secondary. > >> > > > > If we *req_ptr = NULL; compiler will not DCE the following code block when > VM_EVENT=n: > > ``` > > if ( req_ptr ) > > { > > if ( monitor_traps(curr, sync, req_ptr) < 0 ) > > rc = 0; > > > > xfree(req_ptr); > > } > > return rc; > > ``` > > Or am I misunderstanding what you suggest? > > First: It would have helped if you had also said where that code fragment > actually > was taken from. > > Seeing it's in hvm_hap_nested_page_fault(), I'm having trouble following why > the > compiler wouldn't be able to see that the local variable "req_ptr" there > would never > change value, i.e. remain NULL throughout its lifetime. If indeed there's a > compiler > shortcoming, that either wants working around or properly writing down. >
This runtime undefined error will only occur when we turn on CONFIG_UBSAN(, then -fsanitize=undefined in CFLAG). Idk why.... But if we strengthen the condition check with vm_event_is_enabled(), we will pass even when UBSAN=y. ``` if ( req_ptr && vm_event_is_enabled(curr) ) ``` > Jan
