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

[Jan Beulich <jbeulich@suse.com>]

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.

Jan