| Message ID | 20221104142235.36556-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vapic: followup from assisted APIC series | expand |
On 04/11/2022 14:22, Roger Pau Monne wrote: > The current reporting of the hardware assisted APIC options is done by > checking "virtualize APIC accesses" which is not very helpful, as that > feature doesn't avoid a vmexit, instead it does provide some help in > order to detect APIC MMIO accesses in vmexit processing. > > Repurpose the current reporting of xAPIC assistance to instead report > such feature as present when there's support for "TPR shadow" and > "APIC register virtualization" because in that case some xAPIC MMIO > register accesses are handled directly by the hardware, without > requiring a vmexit. > > For symetry also change assisted x2APIC reporting to require > "virtualize x2APIC mode" and "APIC register virtualization", dropping > the option to also be reported when "virtual interrupt delivery" is > available. Presence of the "virtual interrupt delivery" feature will > be reported using a different option. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > I find the logic in vmx_vlapic_msr_changed() hard to follow, but I > don't want to rewrite the function logic at this point. > --- > xen/arch/x86/hvm/viridian/viridian.c | 2 +- > xen/arch/x86/hvm/vmx/vmcs.c | 8 ++++---- > xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++++++++------- > xen/arch/x86/traps.c | 4 +--- > 4 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c > index c4fa0a8b32..bafd8e90de 100644 > --- a/xen/arch/x86/hvm/viridian/viridian.c > +++ b/xen/arch/x86/hvm/viridian/viridian.c > @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf, > * Suggest x2APIC mode by default, unless xAPIC registers are hardware > * virtualized and x2APIC ones aren't. > */ > - if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode ) > + if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) ) So, not sure why this is separated from patch 1 but stated this way it seems counterintuitive. We only want to use the viridian MSRs if they are going to be more efficient.. which I think is only in the case where we have neither an x2apic not an assisted xapic (hence we would trap for MMIO). Paul > res->a |= CPUID4A_MSR_BASED_APIC; > if ( viridian_feature_mask(d) & HVMPV_hcall_ipi ) > res->a |= CPUID4A_SYNTHETIC_CLUSTER_IPI;
On Fri, Nov 04, 2022 at 03:55:54PM +0000, Paul Durrant wrote: > On 04/11/2022 14:22, Roger Pau Monne wrote: > > The current reporting of the hardware assisted APIC options is done by > > checking "virtualize APIC accesses" which is not very helpful, as that > > feature doesn't avoid a vmexit, instead it does provide some help in > > order to detect APIC MMIO accesses in vmexit processing. > > > > Repurpose the current reporting of xAPIC assistance to instead report > > such feature as present when there's support for "TPR shadow" and > > "APIC register virtualization" because in that case some xAPIC MMIO > > register accesses are handled directly by the hardware, without > > requiring a vmexit. > > > > For symetry also change assisted x2APIC reporting to require > > "virtualize x2APIC mode" and "APIC register virtualization", dropping > > the option to also be reported when "virtual interrupt delivery" is > > available. Presence of the "virtual interrupt delivery" feature will > > be reported using a different option. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > I find the logic in vmx_vlapic_msr_changed() hard to follow, but I > > don't want to rewrite the function logic at this point. > > --- > > xen/arch/x86/hvm/viridian/viridian.c | 2 +- > > xen/arch/x86/hvm/vmx/vmcs.c | 8 ++++---- > > xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++++++++------- > > xen/arch/x86/traps.c | 4 +--- > > 4 files changed, 24 insertions(+), 15 deletions(-) > > > > diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c > > index c4fa0a8b32..bafd8e90de 100644 > > --- a/xen/arch/x86/hvm/viridian/viridian.c > > +++ b/xen/arch/x86/hvm/viridian/viridian.c > > @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf, > > * Suggest x2APIC mode by default, unless xAPIC registers are hardware > > * virtualized and x2APIC ones aren't. > > */ > > - if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode ) > > + if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) ) > > So, not sure why this is separated from patch 1 but stated this way it seems > counterintuitive. We only want to use the viridian MSRs if they are going to > be more efficient.. which I think is only in the case where we have neither > an x2apic not an assisted xapic (hence we would trap for MMIO). I've read the MS HTLFS and I guess I got confused, the section about this CPUID bit states: "Bit 3: Recommend using MSRs for accessing APIC registers EOI, ICR and TPR rather than their memory-mapped" So I've (wrongly) understood that MSRs for accessing APIC registers was meant to be a recommendation to use x2APIC mode in order to access those registers. Didn't realize Viridian had a way to expose certain APIC registers using MSRs when the APIC is in xAPIC mode. I withdraw patch 1 and adjust patch 2 accordingly then. Thanks, Roger.
On 04/11/2022 16:01, Roger Pau Monné wrote: > On Fri, Nov 04, 2022 at 03:55:54PM +0000, Paul Durrant wrote: >> On 04/11/2022 14:22, Roger Pau Monne wrote: >>> The current reporting of the hardware assisted APIC options is done by >>> checking "virtualize APIC accesses" which is not very helpful, as that >>> feature doesn't avoid a vmexit, instead it does provide some help in >>> order to detect APIC MMIO accesses in vmexit processing. >>> >>> Repurpose the current reporting of xAPIC assistance to instead report >>> such feature as present when there's support for "TPR shadow" and >>> "APIC register virtualization" because in that case some xAPIC MMIO >>> register accesses are handled directly by the hardware, without >>> requiring a vmexit. >>> >>> For symetry also change assisted x2APIC reporting to require >>> "virtualize x2APIC mode" and "APIC register virtualization", dropping >>> the option to also be reported when "virtual interrupt delivery" is >>> available. Presence of the "virtual interrupt delivery" feature will >>> be reported using a different option. >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> --- >>> I find the logic in vmx_vlapic_msr_changed() hard to follow, but I >>> don't want to rewrite the function logic at this point. >>> --- >>> xen/arch/x86/hvm/viridian/viridian.c | 2 +- >>> xen/arch/x86/hvm/vmx/vmcs.c | 8 ++++---- >>> xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++++++++------- >>> xen/arch/x86/traps.c | 4 +--- >>> 4 files changed, 24 insertions(+), 15 deletions(-) >>> >>> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c >>> index c4fa0a8b32..bafd8e90de 100644 >>> --- a/xen/arch/x86/hvm/viridian/viridian.c >>> +++ b/xen/arch/x86/hvm/viridian/viridian.c >>> @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf, >>> * Suggest x2APIC mode by default, unless xAPIC registers are hardware >>> * virtualized and x2APIC ones aren't. >>> */ >>> - if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode ) >>> + if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) ) >> >> So, not sure why this is separated from patch 1 but stated this way it seems >> counterintuitive. We only want to use the viridian MSRs if they are going to >> be more efficient.. which I think is only in the case where we have neither >> an x2apic not an assisted xapic (hence we would trap for MMIO). > > I've read the MS HTLFS and I guess I got confused, the section about > this CPUID bit states: > > "Bit 3: Recommend using MSRs for accessing APIC registers EOI, ICR and > TPR rather than their memory-mapped" > > So I've (wrongly) understood that MSRs for accessing APIC registers > was meant to be a recommendation to use x2APIC mode in order to access > those registers. Didn't realize Viridian had a way to expose certain > APIC registers using MSRs when the APIC is in xAPIC mode. > Yeah, I think they predate the existence of x2apic. > I withdraw patch 1 and adjust patch 2 accordingly then. > Cool. Thanks, Paul
On Fri, Nov 04, 2022 at 04:05:05PM +0000, Paul Durrant wrote: > On 04/11/2022 16:01, Roger Pau Monné wrote: > > On Fri, Nov 04, 2022 at 03:55:54PM +0000, Paul Durrant wrote: > > > On 04/11/2022 14:22, Roger Pau Monne wrote: > > > > The current reporting of the hardware assisted APIC options is done by > > > > checking "virtualize APIC accesses" which is not very helpful, as that > > > > feature doesn't avoid a vmexit, instead it does provide some help in > > > > order to detect APIC MMIO accesses in vmexit processing. > > > > > > > > Repurpose the current reporting of xAPIC assistance to instead report > > > > such feature as present when there's support for "TPR shadow" and > > > > "APIC register virtualization" because in that case some xAPIC MMIO > > > > register accesses are handled directly by the hardware, without > > > > requiring a vmexit. > > > > > > > > For symetry also change assisted x2APIC reporting to require > > > > "virtualize x2APIC mode" and "APIC register virtualization", dropping > > > > the option to also be reported when "virtual interrupt delivery" is > > > > available. Presence of the "virtual interrupt delivery" feature will > > > > be reported using a different option. > > > > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > > --- > > > > I find the logic in vmx_vlapic_msr_changed() hard to follow, but I > > > > don't want to rewrite the function logic at this point. > > > > --- > > > > xen/arch/x86/hvm/viridian/viridian.c | 2 +- > > > > xen/arch/x86/hvm/vmx/vmcs.c | 8 ++++---- > > > > xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++++++++------- > > > > xen/arch/x86/traps.c | 4 +--- > > > > 4 files changed, 24 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c > > > > index c4fa0a8b32..bafd8e90de 100644 > > > > --- a/xen/arch/x86/hvm/viridian/viridian.c > > > > +++ b/xen/arch/x86/hvm/viridian/viridian.c > > > > @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf, > > > > * Suggest x2APIC mode by default, unless xAPIC registers are hardware > > > > * virtualized and x2APIC ones aren't. > > > > */ > > > > - if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode ) > > > > + if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) ) > > > > > > So, not sure why this is separated from patch 1 but stated this way it seems > > > counterintuitive. We only want to use the viridian MSRs if they are going to > > > be more efficient.. which I think is only in the case where we have neither > > > an x2apic not an assisted xapic (hence we would trap for MMIO). > > > > I've read the MS HTLFS and I guess I got confused, the section about > > this CPUID bit states: > > > > "Bit 3: Recommend using MSRs for accessing APIC registers EOI, ICR and > > TPR rather than their memory-mapped" > > > > So I've (wrongly) understood that MSRs for accessing APIC registers > > was meant to be a recommendation to use x2APIC mode in order to access > > those registers. Didn't realize Viridian had a way to expose certain > > APIC registers using MSRs when the APIC is in xAPIC mode. > > > > Yeah, I think they predate the existence of x2apic. > > > I withdraw patch 1 and adjust patch 2 accordingly then. > > > Cool. Thanks, How does Windows know whether to use xAPIC or x2APIC? I would assume CPUID4A_MSR_BASED_APIC only makes sense when in xAPIC mode, as otherwise the registers are already accesses using MSRs. Thanks, Roger.
On 04/11/2022 16:10, Roger Pau Monné wrote: > On Fri, Nov 04, 2022 at 04:05:05PM +0000, Paul Durrant wrote: >> On 04/11/2022 16:01, Roger Pau Monné wrote: >>> On Fri, Nov 04, 2022 at 03:55:54PM +0000, Paul Durrant wrote: >>>> On 04/11/2022 14:22, Roger Pau Monne wrote: >>>>> The current reporting of the hardware assisted APIC options is done by >>>>> checking "virtualize APIC accesses" which is not very helpful, as that >>>>> feature doesn't avoid a vmexit, instead it does provide some help in >>>>> order to detect APIC MMIO accesses in vmexit processing. >>>>> >>>>> Repurpose the current reporting of xAPIC assistance to instead report >>>>> such feature as present when there's support for "TPR shadow" and >>>>> "APIC register virtualization" because in that case some xAPIC MMIO >>>>> register accesses are handled directly by the hardware, without >>>>> requiring a vmexit. >>>>> >>>>> For symetry also change assisted x2APIC reporting to require >>>>> "virtualize x2APIC mode" and "APIC register virtualization", dropping >>>>> the option to also be reported when "virtual interrupt delivery" is >>>>> available. Presence of the "virtual interrupt delivery" feature will >>>>> be reported using a different option. >>>>> >>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>>>> --- >>>>> I find the logic in vmx_vlapic_msr_changed() hard to follow, but I >>>>> don't want to rewrite the function logic at this point. >>>>> --- >>>>> xen/arch/x86/hvm/viridian/viridian.c | 2 +- >>>>> xen/arch/x86/hvm/vmx/vmcs.c | 8 ++++---- >>>>> xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++++++++------- >>>>> xen/arch/x86/traps.c | 4 +--- >>>>> 4 files changed, 24 insertions(+), 15 deletions(-) >>>>> >>>>> diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c >>>>> index c4fa0a8b32..bafd8e90de 100644 >>>>> --- a/xen/arch/x86/hvm/viridian/viridian.c >>>>> +++ b/xen/arch/x86/hvm/viridian/viridian.c >>>>> @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf, >>>>> * Suggest x2APIC mode by default, unless xAPIC registers are hardware >>>>> * virtualized and x2APIC ones aren't. >>>>> */ >>>>> - if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode ) >>>>> + if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) ) >>>> >>>> So, not sure why this is separated from patch 1 but stated this way it seems >>>> counterintuitive. We only want to use the viridian MSRs if they are going to >>>> be more efficient.. which I think is only in the case where we have neither >>>> an x2apic not an assisted xapic (hence we would trap for MMIO). >>> >>> I've read the MS HTLFS and I guess I got confused, the section about >>> this CPUID bit states: >>> >>> "Bit 3: Recommend using MSRs for accessing APIC registers EOI, ICR and >>> TPR rather than their memory-mapped" >>> >>> So I've (wrongly) understood that MSRs for accessing APIC registers >>> was meant to be a recommendation to use x2APIC mode in order to access >>> those registers. Didn't realize Viridian had a way to expose certain >>> APIC registers using MSRs when the APIC is in xAPIC mode. >>> >> >> Yeah, I think they predate the existence of x2apic. >> >>> I withdraw patch 1 and adjust patch 2 accordingly then. >>> >> Cool. Thanks, > > How does Windows know whether to use xAPIC or x2APIC? > cpuid? TBH I'm not sure why this recommendation would ever trump x2apic anyway. > I would assume CPUID4A_MSR_BASED_APIC only makes sense when in xAPIC > mode, as otherwise the registers are already accesses using MSRs. > > Thanks, Roger.
On Fri, Nov 04, 2022 at 05:35:23PM +0000, Paul Durrant wrote: > On 04/11/2022 16:10, Roger Pau Monné wrote: > > On Fri, Nov 04, 2022 at 04:05:05PM +0000, Paul Durrant wrote: > > > On 04/11/2022 16:01, Roger Pau Monné wrote: > > > > On Fri, Nov 04, 2022 at 03:55:54PM +0000, Paul Durrant wrote: > > > > > On 04/11/2022 14:22, Roger Pau Monne wrote: > > > > > > The current reporting of the hardware assisted APIC options is done by > > > > > > checking "virtualize APIC accesses" which is not very helpful, as that > > > > > > feature doesn't avoid a vmexit, instead it does provide some help in > > > > > > order to detect APIC MMIO accesses in vmexit processing. > > > > > > > > > > > > Repurpose the current reporting of xAPIC assistance to instead report > > > > > > such feature as present when there's support for "TPR shadow" and > > > > > > "APIC register virtualization" because in that case some xAPIC MMIO > > > > > > register accesses are handled directly by the hardware, without > > > > > > requiring a vmexit. > > > > > > > > > > > > For symetry also change assisted x2APIC reporting to require > > > > > > "virtualize x2APIC mode" and "APIC register virtualization", dropping > > > > > > the option to also be reported when "virtual interrupt delivery" is > > > > > > available. Presence of the "virtual interrupt delivery" feature will > > > > > > be reported using a different option. > > > > > > > > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > > > > --- > > > > > > I find the logic in vmx_vlapic_msr_changed() hard to follow, but I > > > > > > don't want to rewrite the function logic at this point. > > > > > > --- > > > > > > xen/arch/x86/hvm/viridian/viridian.c | 2 +- > > > > > > xen/arch/x86/hvm/vmx/vmcs.c | 8 ++++---- > > > > > > xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++++++++------- > > > > > > xen/arch/x86/traps.c | 4 +--- > > > > > > 4 files changed, 24 insertions(+), 15 deletions(-) > > > > > > > > > > > > diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c > > > > > > index c4fa0a8b32..bafd8e90de 100644 > > > > > > --- a/xen/arch/x86/hvm/viridian/viridian.c > > > > > > +++ b/xen/arch/x86/hvm/viridian/viridian.c > > > > > > @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf, > > > > > > * Suggest x2APIC mode by default, unless xAPIC registers are hardware > > > > > > * virtualized and x2APIC ones aren't. > > > > > > */ > > > > > > - if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode ) > > > > > > + if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) ) > > > > > > > > > > So, not sure why this is separated from patch 1 but stated this way it seems > > > > > counterintuitive. We only want to use the viridian MSRs if they are going to > > > > > be more efficient.. which I think is only in the case where we have neither > > > > > an x2apic not an assisted xapic (hence we would trap for MMIO). > > > > > > > > I've read the MS HTLFS and I guess I got confused, the section about > > > > this CPUID bit states: > > > > > > > > "Bit 3: Recommend using MSRs for accessing APIC registers EOI, ICR and > > > > TPR rather than their memory-mapped" > > > > > > > > So I've (wrongly) understood that MSRs for accessing APIC registers > > > > was meant to be a recommendation to use x2APIC mode in order to access > > > > those registers. Didn't realize Viridian had a way to expose certain > > > > APIC registers using MSRs when the APIC is in xAPIC mode. > > > > > > > > > > Yeah, I think they predate the existence of x2apic. > > > > > > > I withdraw patch 1 and adjust patch 2 accordingly then. > > > > > > > Cool. Thanks, > > > > How does Windows know whether to use xAPIC or x2APIC? > > > > cpuid? TBH I'm not sure why this recommendation would ever trump x2apic > anyway. OK, so the recommendation is ignored when running in x2APIC mode, which should be the default since Xen does always expose x2APIC by default to guests. Thanks, Roger.
diff --git a/xen/arch/x86/hvm/viridian/viridian.c b/xen/arch/x86/hvm/viridian/viridian.c index c4fa0a8b32..bafd8e90de 100644 --- a/xen/arch/x86/hvm/viridian/viridian.c +++ b/xen/arch/x86/hvm/viridian/viridian.c @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf, * Suggest x2APIC mode by default, unless xAPIC registers are hardware * virtualized and x2APIC ones aren't. */ - if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode ) + if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) ) res->a |= CPUID4A_MSR_BASED_APIC; if ( viridian_feature_mask(d) & HVMPV_hcall_ipi ) res->a |= CPUID4A_SYNTHETIC_CLUSTER_IPI; diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index a1aca1ec04..7bb96e1a8e 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -1136,7 +1136,7 @@ static int construct_vmcs(struct vcpu *v) if ( !has_assisted_xapic(d) ) v->arch.hvm.vmx.secondary_exec_control &= - ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES; + ~SECONDARY_EXEC_APIC_REGISTER_VIRT; if ( cpu_has_vmx_secondary_exec_control ) __vmwrite(SECONDARY_VM_EXEC_CONTROL, @@ -2156,10 +2156,10 @@ int __init vmx_vmcs_init(void) if ( !ret ) { /* Check whether hardware supports accelerated xapic and x2apic. */ - assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; + assisted_xapic_available = cpu_has_vmx_tpr_shadow && + cpu_has_vmx_apic_reg_virt; assisted_x2apic_available = cpu_has_vmx_virtualize_x2apic_mode && - (cpu_has_vmx_apic_reg_virt || - cpu_has_vmx_virtual_intr_delivery); + cpu_has_vmx_apic_reg_virt; register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1); } diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index e624b415c9..bf0fe3355c 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3405,25 +3405,29 @@ static void vmx_install_vlapic_mapping(struct vcpu *v) void vmx_vlapic_msr_changed(struct vcpu *v) { + bool virtualize_x2apic_mode = has_assisted_x2apic(v->domain) || + (cpu_has_vmx_virtualize_x2apic_mode && + cpu_has_vmx_virtual_intr_delivery); struct vlapic *vlapic = vcpu_vlapic(v); unsigned int msr; - if ( !has_assisted_xapic(v->domain) && - !has_assisted_x2apic(v->domain) ) + if ( !cpu_has_vmx_virtualize_apic_accesses && + !virtualize_x2apic_mode ) return; vmx_vmcs_enter(v); v->arch.hvm.vmx.secondary_exec_control &= ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES | - SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE); + SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE | + SECONDARY_EXEC_APIC_REGISTER_VIRT); if ( !vlapic_hw_disabled(vlapic) && (vlapic_base_address(vlapic) == APIC_DEFAULT_PHYS_BASE) ) { - if ( has_assisted_x2apic(v->domain) && vlapic_x2apic_mode(vlapic) ) + if ( virtualize_x2apic_mode && vlapic_x2apic_mode(vlapic) ) { v->arch.hvm.vmx.secondary_exec_control |= SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE; - if ( cpu_has_vmx_apic_reg_virt ) + if ( has_assisted_x2apic(v->domain) ) { for ( msr = MSR_X2APIC_FIRST; msr <= MSR_X2APIC_LAST; msr++ ) @@ -3432,6 +3436,10 @@ void vmx_vlapic_msr_changed(struct vcpu *v) vmx_set_msr_intercept(v, MSR_X2APIC_PPR, VMX_MSR_R); vmx_set_msr_intercept(v, MSR_X2APIC_TMICT, VMX_MSR_R); vmx_set_msr_intercept(v, MSR_X2APIC_TMCCT, VMX_MSR_R); + + v->arch.hvm.vmx.secondary_exec_control |= + SECONDARY_EXEC_APIC_REGISTER_VIRT; + } if ( cpu_has_vmx_virtual_intr_delivery ) { @@ -3440,9 +3448,12 @@ void vmx_vlapic_msr_changed(struct vcpu *v) vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W); } } - else if ( has_assisted_xapic(v->domain) ) + else if ( vlapic_xapic_mode(vlapic) ) v->arch.hvm.vmx.secondary_exec_control |= - SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES; + (cpu_has_vmx_virtualize_apic_accesses ? + SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES : 0) | + (has_assisted_xapic(v->domain) ? + SECONDARY_EXEC_APIC_REGISTER_VIRT : 0); } if ( !(v->arch.hvm.vmx.secondary_exec_control & SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE) ) diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 7207390118..5c0aabe8a3 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -1124,8 +1124,7 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, if ( !is_hvm_domain(d) || subleaf != 0 ) break; - if ( cpu_has_vmx_apic_reg_virt && - has_assisted_xapic(d) ) + if ( has_assisted_xapic(d) ) res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT; /* @@ -1135,7 +1134,6 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, * vmx_vlapic_msr_changed()). */ if ( has_assisted_x2apic(d) && - cpu_has_vmx_apic_reg_virt && cpu_has_vmx_virtual_intr_delivery ) res->a |= XEN_HVM_CPUID_X2APIC_VIRT;
The current reporting of the hardware assisted APIC options is done by checking "virtualize APIC accesses" which is not very helpful, as that feature doesn't avoid a vmexit, instead it does provide some help in order to detect APIC MMIO accesses in vmexit processing. Repurpose the current reporting of xAPIC assistance to instead report such feature as present when there's support for "TPR shadow" and "APIC register virtualization" because in that case some xAPIC MMIO register accesses are handled directly by the hardware, without requiring a vmexit. For symetry also change assisted x2APIC reporting to require "virtualize x2APIC mode" and "APIC register virtualization", dropping the option to also be reported when "virtual interrupt delivery" is available. Presence of the "virtual interrupt delivery" feature will be reported using a different option. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- I find the logic in vmx_vlapic_msr_changed() hard to follow, but I don't want to rewrite the function logic at this point. --- xen/arch/x86/hvm/viridian/viridian.c | 2 +- xen/arch/x86/hvm/vmx/vmcs.c | 8 ++++---- xen/arch/x86/hvm/vmx/vmx.c | 25 ++++++++++++++++++------- xen/arch/x86/traps.c | 4 +--- 4 files changed, 24 insertions(+), 15 deletions(-)