| Message ID | 00ce7005d1d6db5c1ffc2d5023d34d4bd34ff841.1715102098.git.alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: Expose consistent topology to guests | expand |
On 08.05.2024 14:39, Alejandro Vallejo wrote: > Otherwise it's not possible to call functions described in hvm/vlapic.h from the > inline functions of hvm/hvm.h. > > This is because a static inline in vlapic.h depends on hvm.h, and pulls it > transitively through vpt.h. The ultimate cause is having hvm.h included in any > of the "v*.h" headers, so break the cycle moving the guilty inline into hvm.h. > > No functional change. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> In principle: Reviewed-by: Jan Beulich <jbeulich@suse.com> But see below for one possible adjustment. > --- > v2: > * New patch. Prereq to moving vlapic_cpu_policy_changed() onto hvm.h That hook invocation living outside of hvm/hvm.h was an outlier anyway, so even without the planned further work this is probably a good move. > --- a/xen/arch/x86/include/asm/hvm/hvm.h > +++ b/xen/arch/x86/include/asm/hvm/hvm.h > @@ -798,6 +798,12 @@ static inline void hvm_update_vlapic_mode(struct vcpu *v) > alternative_vcall(hvm_funcs.update_vlapic_mode, v); > } > > +static inline void hvm_vlapic_sync_pir_to_irr(struct vcpu *v) > +{ > + if ( hvm_funcs.sync_pir_to_irr ) > + alternative_vcall(hvm_funcs.sync_pir_to_irr, v); > +} The hook doesn't have "vlapic" in its name. Therefore instead or prepending hvm_ to the original name or the wrapper, how about replacing the vlapic_ that was there. That would then also fit better with the naming scheme used for other hooks and their wrappers. Happy to adjust while committing, so long as you don't disagree. Jan
On 22/05/2024 10:33, Jan Beulich wrote: > On 08.05.2024 14:39, Alejandro Vallejo wrote: >> Otherwise it's not possible to call functions described in hvm/vlapic.h from the >> inline functions of hvm/hvm.h. >> >> This is because a static inline in vlapic.h depends on hvm.h, and pulls it >> transitively through vpt.h. The ultimate cause is having hvm.h included in any >> of the "v*.h" headers, so break the cycle moving the guilty inline into hvm.h. >> >> No functional change. >> >> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > > In principle: > Reviewed-by: Jan Beulich <jbeulich@suse.com> > But see below for one possible adjustment. > >> --- >> v2: >> * New patch. Prereq to moving vlapic_cpu_policy_changed() onto hvm.h > > That hook invocation living outside of hvm/hvm.h was an outlier anyway, > so even without the planned further work this is probably a good move. > >> --- a/xen/arch/x86/include/asm/hvm/hvm.h >> +++ b/xen/arch/x86/include/asm/hvm/hvm.h >> @@ -798,6 +798,12 @@ static inline void hvm_update_vlapic_mode(struct vcpu *v) >> alternative_vcall(hvm_funcs.update_vlapic_mode, v); >> } >> >> +static inline void hvm_vlapic_sync_pir_to_irr(struct vcpu *v) >> +{ >> + if ( hvm_funcs.sync_pir_to_irr ) >> + alternative_vcall(hvm_funcs.sync_pir_to_irr, v); >> +} > > The hook doesn't have "vlapic" in its name. Therefore instead or prepending > hvm_ to the original name or the wrapper, how about replacing the vlapic_ > that was there. That would then also fit better with the naming scheme used > for other hooks and their wrappers. Happy to adjust while committing, so > long as you don't disagree. > > Jan Sounds reasonable. I wasn't sure whether vlapic was adding anything more than a namespace prefix to the function name. Are you happy to adjust that on commit? If so, I'm good with it in the form you propose. Cheers, Alejandro
On Wed, May 08, 2024 at 01:39:21PM +0100, Alejandro Vallejo wrote: > Otherwise it's not possible to call functions described in hvm/vlapic.h from the > inline functions of hvm/hvm.h. > > This is because a static inline in vlapic.h depends on hvm.h, and pulls it > transitively through vpt.h. The ultimate cause is having hvm.h included in any > of the "v*.h" headers, so break the cycle moving the guilty inline into hvm.h. > > No functional change. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> One cosmetic comment below. > --- > v2: > * New patch. Prereq to moving vlapic_cpu_policy_changed() onto hvm.h > --- > xen/arch/x86/hvm/irq.c | 6 +++--- > xen/arch/x86/hvm/vlapic.c | 4 ++-- > xen/arch/x86/include/asm/hvm/hvm.h | 6 ++++++ > xen/arch/x86/include/asm/hvm/vlapic.h | 6 ------ > xen/arch/x86/include/asm/hvm/vpt.h | 1 - > 5 files changed, 11 insertions(+), 12 deletions(-) > > diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c > index 4a9fe82cbd8d..4f5479b12c98 100644 > --- a/xen/arch/x86/hvm/irq.c > +++ b/xen/arch/x86/hvm/irq.c > @@ -512,13 +512,13 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v) > int vector; > > /* > - * Always call vlapic_sync_pir_to_irr so that PIR is synced into IRR when > - * using posted interrupts. Note this is also done by > + * Always call hvm_vlapic_sync_pir_to_irr so that PIR is synced into IRR > + * when using posted interrupts. Note this is also done by > * vlapic_has_pending_irq but depending on which interrupts are pending > * hvm_vcpu_has_pending_irq will return early without calling > * vlapic_has_pending_irq. > */ > - vlapic_sync_pir_to_irr(v); > + hvm_vlapic_sync_pir_to_irr(v); > > if ( unlikely(v->arch.nmi_pending) ) > return hvm_intack_nmi; > diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c > index 61a96474006b..8a244100009c 100644 > --- a/xen/arch/x86/hvm/vlapic.c > +++ b/xen/arch/x86/hvm/vlapic.c > @@ -98,7 +98,7 @@ static void vlapic_clear_irr(int vector, struct vlapic *vlapic) > > static int vlapic_find_highest_irr(struct vlapic *vlapic) > { > - vlapic_sync_pir_to_irr(vlapic_vcpu(vlapic)); > + hvm_vlapic_sync_pir_to_irr(vlapic_vcpu(vlapic)); > > return vlapic_find_highest_vector(&vlapic->regs->data[APIC_IRR]); > } > @@ -1516,7 +1516,7 @@ static int cf_check lapic_save_regs(struct vcpu *v, hvm_domain_context_t *h) > if ( !has_vlapic(v->domain) ) > return 0; > > - vlapic_sync_pir_to_irr(v); > + hvm_vlapic_sync_pir_to_irr(v); > > return hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, vcpu_vlapic(v)->regs); > } > diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h > index e1f0585d75a9..84911f3ebcb4 100644 > --- a/xen/arch/x86/include/asm/hvm/hvm.h > +++ b/xen/arch/x86/include/asm/hvm/hvm.h > @@ -798,6 +798,12 @@ static inline void hvm_update_vlapic_mode(struct vcpu *v) > alternative_vcall(hvm_funcs.update_vlapic_mode, v); > } > > +static inline void hvm_vlapic_sync_pir_to_irr(struct vcpu *v) > +{ > + if ( hvm_funcs.sync_pir_to_irr ) > + alternative_vcall(hvm_funcs.sync_pir_to_irr, v); Nit: for consistency the wrappers are usually named hvm_<hook_name>, so in this case it would be hvm_sync_pir_to_irr(), or the hvm_funcs field should be renamed to vlapic_sync_pir_to_irr. Thanks, Roger.
On 23.05.2024 16:37, Roger Pau Monné wrote: > On Wed, May 08, 2024 at 01:39:21PM +0100, Alejandro Vallejo wrote: >> --- a/xen/arch/x86/include/asm/hvm/hvm.h >> +++ b/xen/arch/x86/include/asm/hvm/hvm.h >> @@ -798,6 +798,12 @@ static inline void hvm_update_vlapic_mode(struct vcpu *v) >> alternative_vcall(hvm_funcs.update_vlapic_mode, v); >> } >> >> +static inline void hvm_vlapic_sync_pir_to_irr(struct vcpu *v) >> +{ >> + if ( hvm_funcs.sync_pir_to_irr ) >> + alternative_vcall(hvm_funcs.sync_pir_to_irr, v); > > Nit: for consistency the wrappers are usually named hvm_<hook_name>, > so in this case it would be hvm_sync_pir_to_irr(), or the hvm_funcs > field should be renamed to vlapic_sync_pir_to_irr. Funny you should mention that: See my earlier comment as well as what was committed. Jan
On Thu, May 23, 2024 at 04:40:06PM +0200, Jan Beulich wrote: > On 23.05.2024 16:37, Roger Pau Monné wrote: > > On Wed, May 08, 2024 at 01:39:21PM +0100, Alejandro Vallejo wrote: > >> --- a/xen/arch/x86/include/asm/hvm/hvm.h > >> +++ b/xen/arch/x86/include/asm/hvm/hvm.h > >> @@ -798,6 +798,12 @@ static inline void hvm_update_vlapic_mode(struct vcpu *v) > >> alternative_vcall(hvm_funcs.update_vlapic_mode, v); > >> } > >> > >> +static inline void hvm_vlapic_sync_pir_to_irr(struct vcpu *v) > >> +{ > >> + if ( hvm_funcs.sync_pir_to_irr ) > >> + alternative_vcall(hvm_funcs.sync_pir_to_irr, v); > > > > Nit: for consistency the wrappers are usually named hvm_<hook_name>, > > so in this case it would be hvm_sync_pir_to_irr(), or the hvm_funcs > > field should be renamed to vlapic_sync_pir_to_irr. > > Funny you should mention that: See my earlier comment as well as what > was committed. Oh, sorry, didn't realize you already replied, adjusted and committed. Thanks, Roger.
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c index 4a9fe82cbd8d..4f5479b12c98 100644 --- a/xen/arch/x86/hvm/irq.c +++ b/xen/arch/x86/hvm/irq.c @@ -512,13 +512,13 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v) int vector; /* - * Always call vlapic_sync_pir_to_irr so that PIR is synced into IRR when - * using posted interrupts. Note this is also done by + * Always call hvm_vlapic_sync_pir_to_irr so that PIR is synced into IRR + * when using posted interrupts. Note this is also done by * vlapic_has_pending_irq but depending on which interrupts are pending * hvm_vcpu_has_pending_irq will return early without calling * vlapic_has_pending_irq. */ - vlapic_sync_pir_to_irr(v); + hvm_vlapic_sync_pir_to_irr(v); if ( unlikely(v->arch.nmi_pending) ) return hvm_intack_nmi; diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 61a96474006b..8a244100009c 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -98,7 +98,7 @@ static void vlapic_clear_irr(int vector, struct vlapic *vlapic) static int vlapic_find_highest_irr(struct vlapic *vlapic) { - vlapic_sync_pir_to_irr(vlapic_vcpu(vlapic)); + hvm_vlapic_sync_pir_to_irr(vlapic_vcpu(vlapic)); return vlapic_find_highest_vector(&vlapic->regs->data[APIC_IRR]); } @@ -1516,7 +1516,7 @@ static int cf_check lapic_save_regs(struct vcpu *v, hvm_domain_context_t *h) if ( !has_vlapic(v->domain) ) return 0; - vlapic_sync_pir_to_irr(v); + hvm_vlapic_sync_pir_to_irr(v); return hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, vcpu_vlapic(v)->regs); } diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h index e1f0585d75a9..84911f3ebcb4 100644 --- a/xen/arch/x86/include/asm/hvm/hvm.h +++ b/xen/arch/x86/include/asm/hvm/hvm.h @@ -798,6 +798,12 @@ static inline void hvm_update_vlapic_mode(struct vcpu *v) alternative_vcall(hvm_funcs.update_vlapic_mode, v); } +static inline void hvm_vlapic_sync_pir_to_irr(struct vcpu *v) +{ + if ( hvm_funcs.sync_pir_to_irr ) + alternative_vcall(hvm_funcs.sync_pir_to_irr, v); +} + #else /* CONFIG_HVM */ #define hvm_enabled false diff --git a/xen/arch/x86/include/asm/hvm/vlapic.h b/xen/arch/x86/include/asm/hvm/vlapic.h index e8d41313abd3..34f23cd38a20 100644 --- a/xen/arch/x86/include/asm/hvm/vlapic.h +++ b/xen/arch/x86/include/asm/hvm/vlapic.h @@ -139,10 +139,4 @@ bool vlapic_match_dest( const struct vlapic *target, const struct vlapic *source, int short_hand, uint32_t dest, bool dest_mode); -static inline void vlapic_sync_pir_to_irr(struct vcpu *v) -{ - if ( hvm_funcs.sync_pir_to_irr ) - alternative_vcall(hvm_funcs.sync_pir_to_irr, v); -} - #endif /* __ASM_X86_HVM_VLAPIC_H__ */ diff --git a/xen/arch/x86/include/asm/hvm/vpt.h b/xen/arch/x86/include/asm/hvm/vpt.h index feb0bf43f14b..0b92b286252d 100644 --- a/xen/arch/x86/include/asm/hvm/vpt.h +++ b/xen/arch/x86/include/asm/hvm/vpt.h @@ -11,7 +11,6 @@ #include <xen/timer.h> #include <xen/list.h> #include <xen/rwlock.h> -#include <asm/hvm/hvm.h> /* * Abstract layer of periodic time, one short time.
Otherwise it's not possible to call functions described in hvm/vlapic.h from the inline functions of hvm/hvm.h. This is because a static inline in vlapic.h depends on hvm.h, and pulls it transitively through vpt.h. The ultimate cause is having hvm.h included in any of the "v*.h" headers, so break the cycle moving the guilty inline into hvm.h. No functional change. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- v2: * New patch. Prereq to moving vlapic_cpu_policy_changed() onto hvm.h --- xen/arch/x86/hvm/irq.c | 6 +++--- xen/arch/x86/hvm/vlapic.c | 4 ++-- xen/arch/x86/include/asm/hvm/hvm.h | 6 ++++++ xen/arch/x86/include/asm/hvm/vlapic.h | 6 ------ xen/arch/x86/include/asm/hvm/vpt.h | 1 - 5 files changed, 11 insertions(+), 12 deletions(-)