| Message ID | a404fafe-0057-0ea7-93c3-754f96da8743@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | memaccess: reduce include dependencies | expand |
On Mon, Mar 9, 2020 at 5:49 AM Jan Beulich <jbeulich@suse.com> wrote: > > The common header doesn't itself need to include public/vm_event.h nor > public/memory.h. Drop their inclusion. This requires using the non- > typedef names in two prototypes and an inline function; by not changing > the callers and function definitions at the same time it'll remain > certain that the build would fail if the typedef itself was changed. Just curious, what's the benefit of doing this? Tamas
On 09.03.2020 16:07, Tamas K Lengyel wrote: > On Mon, Mar 9, 2020 at 5:49 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> The common header doesn't itself need to include public/vm_event.h nor >> public/memory.h. Drop their inclusion. This requires using the non- >> typedef names in two prototypes and an inline function; by not changing >> the callers and function definitions at the same time it'll remain >> certain that the build would fail if the typedef itself was changed. > > Just curious, what's the benefit of doing this? Less dependencies that (almost) every CU gets, and hence statistically less rebuilds of (almost) everything when only a rather special purpose header file changes. Jan
On 09/03/2020 15:10, Jan Beulich wrote: > On 09.03.2020 16:07, Tamas K Lengyel wrote: >> On Mon, Mar 9, 2020 at 5:49 AM Jan Beulich <jbeulich@suse.com> wrote: >>> The common header doesn't itself need to include public/vm_event.h nor >>> public/memory.h. Drop their inclusion. This requires using the non- >>> typedef names in two prototypes and an inline function; by not changing >>> the callers and function definitions at the same time it'll remain >>> certain that the build would fail if the typedef itself was changed. >> Just curious, what's the benefit of doing this? > Less dependencies that (almost) every CU gets, and hence statistically > less rebuilds of (almost) everything when only a rather special purpose > header file changes. Also fractionally faster compile times generally (reduced parsing of unrelated header files), and overall, a reduction in the complexity of the tangled mess that is our header files. ~Andrew
On Mon, Mar 9, 2020 at 5:49 AM Jan Beulich <jbeulich@suse.com> wrote: > > The common header doesn't itself need to include public/vm_event.h nor > public/memory.h. Drop their inclusion. This requires using the non- > typedef names in two prototypes and an inline function; by not changing > the callers and function definitions at the same time it'll remain > certain that the build would fail if the typedef itself was changed. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/include/asm-arm/mem_access.h > +++ b/xen/include/asm-arm/mem_access.h > @@ -17,9 +17,11 @@ > #ifndef _ASM_ARM_MEM_ACCESS_H > #define _ASM_ARM_MEM_ACCESS_H > > +struct vm_event_st; > + > static inline > bool p2m_mem_access_emulate_check(struct vcpu *v, > - const vm_event_response_t *rsp) > + const struct vm_event_st *rsp) > { > /* Not supported on ARM. */ > return false; > --- a/xen/include/asm-x86/mem_access.h > +++ b/xen/include/asm-x86/mem_access.h > @@ -26,6 +26,8 @@ > #ifndef __ASM_X86_MEM_ACCESS_H__ > #define __ASM_X86_MEM_ACCESS_H__ > > +struct vm_event_st; Wouldn't it make more sense to define this in xen/mem_access.h instead of having to do it in both asm versions? Nothing directly includes asm/mem_access.h, all users include xen/mem_access.h Tamas
On 09.03.2020 16:51, Tamas K Lengyel wrote: > On Mon, Mar 9, 2020 at 5:49 AM Jan Beulich <jbeulich@suse.com> wrote: >> --- a/xen/include/asm-arm/mem_access.h >> +++ b/xen/include/asm-arm/mem_access.h >> @@ -17,9 +17,11 @@ >> #ifndef _ASM_ARM_MEM_ACCESS_H >> #define _ASM_ARM_MEM_ACCESS_H >> >> +struct vm_event_st; >> + >> static inline >> bool p2m_mem_access_emulate_check(struct vcpu *v, >> - const vm_event_response_t *rsp) >> + const struct vm_event_st *rsp) >> { >> /* Not supported on ARM. */ >> return false; >> --- a/xen/include/asm-x86/mem_access.h >> +++ b/xen/include/asm-x86/mem_access.h >> @@ -26,6 +26,8 @@ >> #ifndef __ASM_X86_MEM_ACCESS_H__ >> #define __ASM_X86_MEM_ACCESS_H__ >> >> +struct vm_event_st; > > Wouldn't it make more sense to define this in xen/mem_access.h instead > of having to do it in both asm versions? Nothing directly includes > asm/mem_access.h, all users include xen/mem_access.h If that's what you prefer - I can certainly do so. It'll look a little odd then, as the forward declaration has to come ahead of #include <asm/mem_access.h> Just let me know if you really prefer it that way. Jan
On Mon, Mar 9, 2020 at 10:03 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 09.03.2020 16:51, Tamas K Lengyel wrote: > > On Mon, Mar 9, 2020 at 5:49 AM Jan Beulich <jbeulich@suse.com> wrote: > >> --- a/xen/include/asm-arm/mem_access.h > >> +++ b/xen/include/asm-arm/mem_access.h > >> @@ -17,9 +17,11 @@ > >> #ifndef _ASM_ARM_MEM_ACCESS_H > >> #define _ASM_ARM_MEM_ACCESS_H > >> > >> +struct vm_event_st; > >> + > >> static inline > >> bool p2m_mem_access_emulate_check(struct vcpu *v, > >> - const vm_event_response_t *rsp) > >> + const struct vm_event_st *rsp) > >> { > >> /* Not supported on ARM. */ > >> return false; > >> --- a/xen/include/asm-x86/mem_access.h > >> +++ b/xen/include/asm-x86/mem_access.h > >> @@ -26,6 +26,8 @@ > >> #ifndef __ASM_X86_MEM_ACCESS_H__ > >> #define __ASM_X86_MEM_ACCESS_H__ > >> > >> +struct vm_event_st; > > > > Wouldn't it make more sense to define this in xen/mem_access.h instead > > of having to do it in both asm versions? Nothing directly includes > > asm/mem_access.h, all users include xen/mem_access.h > > If that's what you prefer - I can certainly do so. It'll look a > little odd then, as the forward declaration has to come ahead of > > #include <asm/mem_access.h> > > Just let me know if you really prefer it that way. Well, I find it ugly either way. I would prefer if it's forward declared just at one spot, with a comment explaining why it's needed/done that way. Thanks, Tamas
--- a/xen/include/asm-arm/mem_access.h +++ b/xen/include/asm-arm/mem_access.h @@ -17,9 +17,11 @@ #ifndef _ASM_ARM_MEM_ACCESS_H #define _ASM_ARM_MEM_ACCESS_H +struct vm_event_st; + static inline bool p2m_mem_access_emulate_check(struct vcpu *v, - const vm_event_response_t *rsp) + const struct vm_event_st *rsp) { /* Not supported on ARM. */ return false; --- a/xen/include/asm-x86/mem_access.h +++ b/xen/include/asm-x86/mem_access.h @@ -26,6 +26,8 @@ #ifndef __ASM_X86_MEM_ACCESS_H__ #define __ASM_X86_MEM_ACCESS_H__ +struct vm_event_st; + /* * 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 @@ -36,12 +38,12 @@ */ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla, struct npfec npfec, - vm_event_request_t **req_ptr); + struct vm_event_st **req_ptr); /* Check for emulation and mark vcpu for skipping one instruction * upon rescheduling if required. */ bool p2m_mem_access_emulate_check(struct vcpu *v, - const vm_event_response_t *rsp); + const struct vm_event_st *rsp); /* Sanity check for mem_access hardware support */ bool p2m_mem_access_sanity_check(const struct domain *d); --- a/xen/include/xen/mem_access.h +++ b/xen/include/xen/mem_access.h @@ -24,8 +24,6 @@ #include <xen/types.h> #include <xen/mm.h> -#include <public/memory.h> -#include <public/vm_event.h> #include <asm/mem_access.h> /*
The common header doesn't itself need to include public/vm_event.h nor public/memory.h. Drop their inclusion. This requires using the non- typedef names in two prototypes and an inline function; by not changing the callers and function definitions at the same time it'll remain certain that the build would fail if the typedef itself was changed. Signed-off-by: Jan Beulich <jbeulich@suse.com>