[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?
> From looking at the function it's also not quite clear to me whether "false"
> is
> the correct return value here. Tamas?
>
> > --- a/xen/arch/x86/include/asm/monitor.h
> > +++ b/xen/arch/x86/include/asm/monitor.h
> > @@ -32,6 +32,7 @@ struct monitor_msr_bitmap {
> > DECLARE_BITMAP(high, 8192);
> > };
> >
> > +#ifdef COMFIG_VM_EVENT
>
> Typo aside, isn't the entire file (perhaps minus some stubs) useful only when
> VM_EVENT=y?
>
Yes, maybe only arch_monitor_get_capabilities() needs the wrapping. As it
invokes hvm_has_set_descriptor_access_exiting(), which is declared only when
VM_EVENT=y
> Jan
