RE: [PATCH v3 09/28] xen/vm_event: consolidate CONFIG_VM_EVENT

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, November 11, 2025 4:14 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; oleksii.kurochko@xxxxxxxxx; Andrew
> Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
> Anthony PERARD <anthony.perard@xxxxxxxxxx>; Orzel, Michal
> <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>; Petre
> Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>; Daniel P. Smith
> <dpsmith@xxxxxxxxxxxxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Tamas K
> Lengyel <tamas@xxxxxxxxxxxxx>
> 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