RE: [PATCH RESEND v10 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
> From: Jane Malalane
> Sent: Wednesday, June 29, 2022 11:17 PM
>
> On 29/06/2022 15:26, Jan Beulich wrote:
> > On 29.06.2022 15:55, Jane Malalane wrote:
> >> Add XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC and
> >> XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC to report accelerated xAPIC
> and
> >> x2APIC, on x86 hardware. This is so that xAPIC and x2APIC virtualization
> >> can subsequently be enabled on a per-domain basis.
> >> No such features are currently implemented on AMD hardware.
> >>
> >> HW assisted xAPIC virtualization will be reported if HW, at the
> >> minimum, supports virtualize_apic_accesses as this feature alone means
> >> that an access to the APIC page will cause an APIC-access VM exit. An
> >> APIC-access VM exit provides a VMM with information about the access
> >> causing the VM exit, unlike a regular EPT fault, thus simplifying some
> >> internal handling.
> >>
> >> HW assisted x2APIC virtualization will be reported if HW supports
> >> virtualize_x2apic_mode and, at least, either apic_reg_virt or
> >> virtual_intr_delivery. This also means that
> >> sysctl follows the conditionals in vmx_vlapic_msr_changed().
> >>
> >> For that purpose, also add an arch-specific "capabilities" parameter
> >> to struct xen_sysctl_physinfo.
> >>
> >> Note that this interface is intended to be compatible with AMD so that
> >> AVIC support can be introduced in a future patch. Unlike Intel that
> >> has multiple controls for APIC Virtualization, AMD has one global
> >> 'AVIC Enable' control bit, so fine-graining of APIC virtualization
> >> control cannot be done on a common interface.
> >>
> >> Suggested-by: Andrew Cooper
> >> Signed-off-by: Jane Malalane
> >> Reviewed-by: "Roger Pau Monné"
> >> Reviewed-by: Jan Beulich
> >> Reviewed-by: Anthony PERARD
> >
> > Could you please clarify whether you did drop Kevin's R-b (which, a
> > little unhelpfully, he provided in reply to v9 a week after you had
> > posted v10) because of ...
> >
> >> v10:
> >> * Make assisted_x{2}apic_available conditional upon _vmx_cpu_up()
> >
> > ... this, requiring him to re-offer the tag? Until told otherwise I
> > will assume so.
>
> It wasn't intentional but yes, that is right. There was a change, albeit
> minor, in vmx from v9 to v10 so I do require Kevin Tian or Jun Nakajima
> to review it.
>
Reviewed-by: Kevin Tian
Re: [PATCH RESEND v10 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
On 29/06/2022 15:49, Christian Lindig wrote: > > > On 29 Jun 2022, at 14:55, Jane Malalane > mailto:jane.malal...@citrix.com>> wrote: > > + physinfo = caml_alloc_tuple(11); > Store_field(physinfo, 0, Val_int(c_physinfo.threads_per_core)); > Store_field(physinfo, 1, Val_int(c_physinfo.cores_per_socket)); > Store_field(physinfo, 2, Val_int(c_physinfo.nr_cpus)); > @@ -749,6 +749,17 @@ CAMLprim value stub_xc_physinfo(value xch) > Store_field(physinfo, 8, cap_list); > Store_field(physinfo, 9, Val_int(c_physinfo.max_cpu_id + 1)); > > +#if defined(__i386__) || defined(__x86_64__) > + /* > + * arch_capabilities: physinfo_arch_cap_flag list; > + */ > + arch_cap_list = c_bitmap_to_ocaml_list > + /* ! physinfo_arch_cap_flag CAP_ none */ > + /* ! XEN_SYSCTL_PHYSCAP_ XEN_SYSCTL_PHYSCAP_X86_MAX max */ > + (c_physinfo.arch_capabilities); > + Store_field(physinfo, 10, arch_cap_list); > +#endif > + > CAMLreturn(physinfo); > } > > I this extending the tuple but only defining a value on x86? Does this not > lead to undefined fields on other architectures? You're right, it's missing a definition, I will send a new version - will just give some time for more eventual comments from others on the series overall. Thank you, Jane
Re: [PATCH RESEND v10 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
On 29/06/2022 15:26, Jan Beulich wrote:
> On 29.06.2022 15:55, Jane Malalane wrote:
>> Add XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC and
>> XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC to report accelerated xAPIC and
>> x2APIC, on x86 hardware. This is so that xAPIC and x2APIC virtualization
>> can subsequently be enabled on a per-domain basis.
>> No such features are currently implemented on AMD hardware.
>>
>> HW assisted xAPIC virtualization will be reported if HW, at the
>> minimum, supports virtualize_apic_accesses as this feature alone means
>> that an access to the APIC page will cause an APIC-access VM exit. An
>> APIC-access VM exit provides a VMM with information about the access
>> causing the VM exit, unlike a regular EPT fault, thus simplifying some
>> internal handling.
>>
>> HW assisted x2APIC virtualization will be reported if HW supports
>> virtualize_x2apic_mode and, at least, either apic_reg_virt or
>> virtual_intr_delivery. This also means that
>> sysctl follows the conditionals in vmx_vlapic_msr_changed().
>>
>> For that purpose, also add an arch-specific "capabilities" parameter
>> to struct xen_sysctl_physinfo.
>>
>> Note that this interface is intended to be compatible with AMD so that
>> AVIC support can be introduced in a future patch. Unlike Intel that
>> has multiple controls for APIC Virtualization, AMD has one global
>> 'AVIC Enable' control bit, so fine-graining of APIC virtualization
>> control cannot be done on a common interface.
>>
>> Suggested-by: Andrew Cooper
>> Signed-off-by: Jane Malalane
>> Reviewed-by: "Roger Pau Monné"
>> Reviewed-by: Jan Beulich
>> Reviewed-by: Anthony PERARD
>
> Could you please clarify whether you did drop Kevin's R-b (which, a
> little unhelpfully, he provided in reply to v9 a week after you had
> posted v10) because of ...
>
>> v10:
>> * Make assisted_x{2}apic_available conditional upon _vmx_cpu_up()
>
> ... this, requiring him to re-offer the tag? Until told otherwise I
> will assume so.
It wasn't intentional but yes, that is right. There was a change, albeit
minor, in vmx from v9 to v10 so I do require Kevin Tian or Jun Nakajima
to review it.
Thank you,
Jane.<>
Re: [PATCH RESEND v10 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
On 29 Jun 2022, at 14:55, Jane Malalane
mailto:jane.malal...@citrix.com>> wrote:
+ physinfo = caml_alloc_tuple(11);
Store_field(physinfo, 0, Val_int(c_physinfo.threads_per_core));
Store_field(physinfo, 1, Val_int(c_physinfo.cores_per_socket));
Store_field(physinfo, 2, Val_int(c_physinfo.nr_cpus));
@@ -749,6 +749,17 @@ CAMLprim value stub_xc_physinfo(value xch)
Store_field(physinfo, 8, cap_list);
Store_field(physinfo, 9, Val_int(c_physinfo.max_cpu_id + 1));
+#if defined(__i386__) || defined(__x86_64__)
+ /*
+ * arch_capabilities: physinfo_arch_cap_flag list;
+ */
+ arch_cap_list = c_bitmap_to_ocaml_list
+ /* ! physinfo_arch_cap_flag CAP_ none */
+ /* ! XEN_SYSCTL_PHYSCAP_ XEN_SYSCTL_PHYSCAP_X86_MAX max */
+ (c_physinfo.arch_capabilities);
+ Store_field(physinfo, 10, arch_cap_list);
+#endif
+
CAMLreturn(physinfo);
}
I this extending the tuple but only defining a value on x86? Does this not lead
to undefined fields on other architectures?
type physinfo = {
threads_per_core : int;
cores_per_socket : int;
@@ -124,6 +128,7 @@ type physinfo = {
scrub_pages : nativeint;
capabilities : physinfo_cap_flag list;
max_nr_cpus : int; (** compile-time max possible number of nr_cpus *)
+ arch_capabilities : physinfo_arch_cap_flag list;
}
type version = { major : int; minor : int; extra : string
Here the record is extended but it looks to me like the new field is kept
undefined on non x86 architectures. If the field still has a defined content,
it would be good to explain why.
— C
Re: [PATCH RESEND v10 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
On 29.06.2022 15:55, Jane Malalane wrote:
> Add XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC and
> XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC to report accelerated xAPIC and
> x2APIC, on x86 hardware. This is so that xAPIC and x2APIC virtualization
> can subsequently be enabled on a per-domain basis.
> No such features are currently implemented on AMD hardware.
>
> HW assisted xAPIC virtualization will be reported if HW, at the
> minimum, supports virtualize_apic_accesses as this feature alone means
> that an access to the APIC page will cause an APIC-access VM exit. An
> APIC-access VM exit provides a VMM with information about the access
> causing the VM exit, unlike a regular EPT fault, thus simplifying some
> internal handling.
>
> HW assisted x2APIC virtualization will be reported if HW supports
> virtualize_x2apic_mode and, at least, either apic_reg_virt or
> virtual_intr_delivery. This also means that
> sysctl follows the conditionals in vmx_vlapic_msr_changed().
>
> For that purpose, also add an arch-specific "capabilities" parameter
> to struct xen_sysctl_physinfo.
>
> Note that this interface is intended to be compatible with AMD so that
> AVIC support can be introduced in a future patch. Unlike Intel that
> has multiple controls for APIC Virtualization, AMD has one global
> 'AVIC Enable' control bit, so fine-graining of APIC virtualization
> control cannot be done on a common interface.
>
> Suggested-by: Andrew Cooper
> Signed-off-by: Jane Malalane
> Reviewed-by: "Roger Pau Monné"
> Reviewed-by: Jan Beulich
> Reviewed-by: Anthony PERARD
Could you please clarify whether you did drop Kevin's R-b (which, a
little unhelpfully, he provided in reply to v9 a week after you had
posted v10) because of ...
> v10:
> * Make assisted_x{2}apic_available conditional upon _vmx_cpu_up()
... this, requiring him to re-offer the tag? Until told otherwise I
will assume so.
Jan
