Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 15, 2022 at 04:33:15PM +0000, Jane Malalane wrote:
> On 15/02/2022 15:21, Jan Beulich wrote:
> > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
> > unless you have verified the sender and know the content is safe.
> > 
> > On 15.02.2022 16:10, Jane Malalane wrote:
> >> On 15/02/2022 10:19, Jan Beulich wrote:
> >>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
> >>> unless you have verified the sender and know the content is safe.
> >>>
> >>> On 15.02.2022 11:14, Jane Malalane wrote:
> >>>> On 15/02/2022 07:09, Jan Beulich wrote:
> >>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open 
> >>>>> attachments unless you have verified the sender and know the content is 
> >>>>> safe.
> >>>>>
> >>>>> On 14.02.2022 18:09, Jane Malalane wrote:
> >>>>>> On 14/02/2022 13:18, Jan Beulich wrote:
> >>>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open 
> >>>>>>> attachments unless you have verified the sender and know the content 
> >>>>>>> is safe.
> >>>>>>>
> >>>>>>> On 14.02.2022 14:11, Jane Malalane wrote:
> >>>>>>>> On 11/02/2022 11:46, Jan Beulich wrote:
> >>>>>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open 
> >>>>>>>>> attachments unless you have verified the sender and know the 
> >>>>>>>>> content is safe.
> >>>>>>>>>
> >>>>>>>>> On 11.02.2022 12:29, Roger Pau Monné wrote:
> >>>>>>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
> >>>>>>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote:
> >>>>>>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
> >>>>>>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c 
> >>>>>>>>>>>>> b/xen/arch/x86/hvm/vmx/vmcs.c
> >>>>>>>>>>>>> index 7ab15e07a0..4060aef1bd 100644
> >>>>>>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> >>>>>>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> >>>>>>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
> >>>>>>>>>>>>>                    MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
> >>>>>>>>>>>>>            }
> >>>>>>>>>>>>>        
> >>>>>>>>>>>>> +    /* Check whether hardware supports accelerated xapic and 
> >>>>>>>>>>>>> x2apic. */
> >>>>>>>>>>>>> +    if ( bsp )
> >>>>>>>>>>>>> +    {
> >>>>>>>>>>>>> +        assisted_xapic_available = 
> >>>>>>>>>>>>> cpu_has_vmx_virtualize_apic_accesses;
> >>>>>>>>>>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt 
> >>>>>>>>>>>>> ||
> >>>>>>>>>>>>> +                                     
> >>>>>>>>>>>>> cpu_has_vmx_virtual_intr_delivery) &&
> >>>>>>>>>>>>> +                                    
> >>>>>>>>>>>>> cpu_has_vmx_virtualize_x2apic_mode;
> >>>>>>>>>>>>
> >>>>>>>>>>>> I've been think about this, and it seems kind of asymmetric that 
> >>>>>>>>>>>> for
> >>>>>>>>>>>> xAPIC mode we report hw assisted support only with
> >>>>>>>>>>>> virtualize_apic_accesses available, while for x2APIC we require
> >>>>>>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or
> >>>>>>>>>>>> virtual_intr_delivery.
> >>>>>>>>>>>>
> >>>>>>>>>>>> I think we likely need to be more consistent here, and report hw
> >>>>>>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is
> >>>>>>>>>>>> available.
> >>>>>>>>>>>>
> >>>>>>>>>>>> This will likely have some effect on patch 2 also, as you will 
> >>>>>>>>>>>> have to
> >>>>>>>>>>>> adjust vmx_vlapic_msr_changed.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Thanks, Roger.
> >>>>>>>>>>>
> >>>>>>>>>>> Any other thoughts on this? As on one hand it is asymmetric but 
> >>>>>>>>>>> also
> >>>>>>>>>>> there isn't much assistance with only virtualize_x2apic_mode set 
> >>>>>>>>>>> as, in
> >>>>>>>>>>> this case, a VM exit will be avoided only when trying to access 
> >>>>>>>>>>> the TPR
> >>>>>>>>>>> register.
> >>>>>>>>>>
> >>>>>>>>>> I've been thinking about this, and reporting hardware assisted
> >>>>>>>>>> x{2}APIC virtualization with just
> >>>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
> >>>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. 
> >>>>>>>>>> While
> >>>>>>>>>> those provide some assistance to the VMM in order to handle APIC
> >>>>>>>>>> accesses, it will still require a trap into the hypervisor to 
> >>>>>>>>>> handle
> >>>>>>>>>> most of the accesses.
> >>>>>>>>>>
> >>>>>>>>>> So maybe we should only report hardware assisted support when the
> >>>>>>>>>> mentioned features are present together with
> >>>>>>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT?
> >>>>>>>>>
> >>>>>>>>> Not sure - "some assistance" seems still a little better than none 
> >>>>>>>>> at all.
> >>>>>>>>> Which route to go depends on what exactly we intend the bit to be 
> >>>>>>>>> used for.
> >>>>>>>>>
> >>>>>>>> True. I intended this bit to be specifically for enabling
> >>>>>>>> assisted_x{2}apic. So, would it be inconsistent to report hardware
> >>>>>>>> assistance with just VIRTUALIZE_APIC_ACCESSES or 
> >>>>>>>> VIRTUALIZE_X2APIC_MODE
> >>>>>>>> but still claim that x{2}apic is virtualized if no MSR accesses are
> >>>>>>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as 
> >>>>>>>> you
> >>>>>>>> say, the guest gets at least "some assistance" instead of none but we
> >>>>>>>> still claim x{2}apic virtualization when it is actually complete? 
> >>>>>>>> Maybe
> >>>>>>>> I could also add a comment alluding to this in the xl documentation.
> >>>>>>>
> >>>>>>> To rephrase my earlier point: Which kind of decisions are the 
> >>>>>>> consumer(s)
> >>>>>>> of us reporting hardware assistance going to take? In how far is 
> >>>>>>> there a
> >>>>>>> risk that "some assistance" is overall going to lead to a loss of
> >>>>>>> performance? I guess I'd need to see comment and actual code all in 
> >>>>>>> one
> >>>>>>> place ...
> >>>>>>>
> >>>>>> So, I was thinking of adding something along the lines of:
> >>>>>>
> >>>>>> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
> >>>>>> +Enables or disables hardware assisted virtualization for xAPIC. This
> >>>>>> +allows accessing APIC registers without a VM-exit. Notice enabling
> >>>>>> +this does not guarantee full virtualization for xAPIC, as this can
> >>>>>> +only be achieved if hardware supports “APIC-register virtualization”
> >>>>>> +and “virtual-interrupt delivery”. The default is settable via
> >>>>>> +L<xl.conf(5)>.
> >>>>>
> >>>>> But isn't this contradictory? Doesn't lack of APIC-register 
> >>>>> virtualization
> >>>>> mean VM exits upon (most) accesses?
> >>>>
> >>>> Yes, it does mean. I guess the alternative wouuld be then to require
> >>>> APIC-register virtualization for enabling xAPIC. But also, although this
> >>>> doesn't provide much acceleration, even getting a VM exit is some
> >>>> assistance if compared to instead getting an EPT fault and having to
> >>>> decode the access.
> >>>
> >>> I agree here, albeit I'd like to mention that EPT faults are also VM
> >>> exits. All my earlier comment was about is that this piece of doc
> >>> wants to express reality, whichever way it is that things end up
> >>> being implemented.
> >>
> >> Oh yes. Right, I see how this info could be misleading.
> >>
> >> How about this?...
> > 
> > Getting close. The thing I can't judge is whether this level of technical
> > detail is suitable for this doc. Just one further remark:
> 
> Unsure too.
> 
> >> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
> >> +
> >> +B<(x86 only)> Enables or disables hardware assisted virtualization for
> >> +xAPIC. With this option enabled, a memory-mapped APIC access will be
> >> +decoded by hardware and either issue a VM exit with an exit reason
> >> +instead of an EPT fault or altogether avoid a VM exit. Notice
> > 
> > As said before, EPT faults also are VM exits and also provide an exit
> > reason. Therefore maybe "... and either issue a VM exit with a more
> > specific exit reason than an EPT fault would provide, or altogether
> > avoid a VM exit" or "... and either issue a more specific VM exit than
> > just an EPT fault, or altogether avoid a VM exit"?
> 
> Yes, that's better.

I would avoid mentioning EPT, as that's an Intel specific technology.
Could we instead use 'p2m fault'?

Thanks, Roger.