diff mbox series

[RFC] xen: Introduce arch specific field to XEN_SYSCTL_physinfo

Message ID 1628897304-20793-1-git-send-email-olekstysh@gmail.com (mailing list archive)
State New, archived
Headers show
Series [RFC] xen: Introduce arch specific field to XEN_SYSCTL_physinfo | expand

Commit Message

Oleksandr Tyshchenko Aug. 13, 2021, 11:28 p.m. UTC 
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

We need to pass info about maximum supported address space size
to the toolstack on Arm in order to properly calculate the base
and size of the safe range for the guest. Use p2m_ipa_bits variable
which purpose is to hold the bit size of IPAs in P2M tables.

As we change the size of structure bump the interface version.

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
You can find the discussion at:
https://lore.kernel.org/xen-devel/cb1c8fd4-a4c5-c18e-c8db-f8e317d95526@xen.org/

Another possible option could be to introduce new Arm specific SYSCTL
to pass such info. But, it was initially decided to not expand the SYSCTL
range and reuse existing which context would fit.
---
 tools/include/libxl.h             | 7 +++++++
 tools/libs/light/libxl.c          | 3 +++
 tools/libs/light/libxl_arch.h     | 5 +++++
 tools/libs/light/libxl_arm.c      | 7 +++++++
 tools/libs/light/libxl_types.idl  | 5 +++++
 tools/libs/light/libxl_x86.c      | 6 ++++++
 xen/arch/arm/sysctl.c             | 1 +
 xen/include/public/arch-arm.h     | 5 +++++
 xen/include/public/arch-x86/xen.h | 2 ++
 xen/include/public/sysctl.h       | 3 ++-
 10 files changed, 43 insertions(+), 1 deletion(-)

Comments

Jan Beulich Aug. 16, 2021, 7:33 a.m. UTC | #1 
On 14.08.2021 01:28, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> We need to pass info about maximum supported address space size
> to the toolstack on Arm in order to properly calculate the base
> and size of the safe range for the guest. Use p2m_ipa_bits variable
> which purpose is to hold the bit size of IPAs in P2M tables.

What is "the safe range"?

> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -332,6 +332,11 @@ struct xen_arch_domainconfig {
>       */
>      uint32_t clock_frequency;
>  };
> +
> +struct arch_physinfo {
> +    /* Holds the bit size of IPAs in p2m tables. */
> +    uint32_t p2m_ipa_bits;
> +};
>  #endif /* __XEN__ || __XEN_TOOLS__ */
>  
>  struct arch_vcpu_info {
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -346,6 +346,8 @@ typedef struct xen_msr_entry {
>  } xen_msr_entry_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_msr_entry_t);
>  
> +struct arch_physinfo {
> +};
>  #endif /* !__ASSEMBLY__ */

While the term "p2m_ipa_bits" surely isn't arch-agnostic, I wonder
whether the expressed information is (the x86 equivalent being
hap_paddr_bits, at a guess), and hence whether this really ought
to live in an arch-specific sub-struct. If indeed so, please name
the struct in a name space clean way, i.e. add xen_ as prefix.

Also please retain a blank line before the #endif. I wonder whether
on Arm you wouldn't want to add one at this occasion.

Jan
Oleksandr Tyshchenko Aug. 29, 2021, 8:28 p.m. UTC | #2 
On 16.08.21 10:33, Jan Beulich wrote:

Hi Jan

Sorry for the late response.

> On 14.08.2021 01:28, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> We need to pass info about maximum supported address space size
>> to the toolstack on Arm in order to properly calculate the base
>> and size of the safe range for the guest. Use p2m_ipa_bits variable
>> which purpose is to hold the bit size of IPAs in P2M tables.
> What is "the safe range"?

It is unallocated (unused) address space which could be safely used by 
domain for foreign/grant mappings on Arm, I will update description.


>
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -332,6 +332,11 @@ struct xen_arch_domainconfig {
>>        */
>>       uint32_t clock_frequency;
>>   };
>> +
>> +struct arch_physinfo {
>> +    /* Holds the bit size of IPAs in p2m tables. */
>> +    uint32_t p2m_ipa_bits;
>> +};
>>   #endif /* __XEN__ || __XEN_TOOLS__ */
>>   
>>   struct arch_vcpu_info {
>> --- a/xen/include/public/arch-x86/xen.h
>> +++ b/xen/include/public/arch-x86/xen.h
>> @@ -346,6 +346,8 @@ typedef struct xen_msr_entry {
>>   } xen_msr_entry_t;
>>   DEFINE_XEN_GUEST_HANDLE(xen_msr_entry_t);
>>   
>> +struct arch_physinfo {
>> +};
>>   #endif /* !__ASSEMBLY__ */
> While the term "p2m_ipa_bits" surely isn't arch-agnostic, I wonder
> whether the expressed information is (the x86 equivalent being
> hap_paddr_bits, at a guess), and hence whether this really ought
> to live in an arch-specific sub-struct.

Well, on Arm we calculate the number of the IPA bits based on the number 
of PA bits when setting Stage 2 address translation.
I might mistake, but what we currently have on Arm is "ipa_bits == 
pa_bits". So, this means that information we need at the toolstack side 
isn't really arch-specific and
we could probably avoid introducing arch-specific sub-struct by suitable 
renaming the field (pa_bits, paddr_bits, whatever).

We could even name the field as hap_paddr_bits. Although, I don't know 
whether the hap_* is purely x86-ism, but I see there are several 
mentions in the common code (hap_pt_share, use_hap_pt, etc). Any 
suggestions?


> If indeed so, please name
> the struct in a name space clean way, i.e. add xen_ as prefix.

ok


>
> Also please retain a blank line before the #endif. I wonder whether
> on Arm you wouldn't want to add one at this occasion.

ok


>
> Jan
>
Jan Beulich Aug. 30, 2021, 1:16 p.m. UTC | #3 
On 29.08.2021 22:28, Oleksandr wrote:
> On 16.08.21 10:33, Jan Beulich wrote:
>> On 14.08.2021 01:28, Oleksandr Tyshchenko wrote:
>>> --- a/xen/include/public/arch-arm.h
>>> +++ b/xen/include/public/arch-arm.h
>>> @@ -332,6 +332,11 @@ struct xen_arch_domainconfig {
>>>        */
>>>       uint32_t clock_frequency;
>>>   };
>>> +
>>> +struct arch_physinfo {
>>> +    /* Holds the bit size of IPAs in p2m tables. */
>>> +    uint32_t p2m_ipa_bits;
>>> +};
>>>   #endif /* __XEN__ || __XEN_TOOLS__ */
>>>   
>>>   struct arch_vcpu_info {
>>> --- a/xen/include/public/arch-x86/xen.h
>>> +++ b/xen/include/public/arch-x86/xen.h
>>> @@ -346,6 +346,8 @@ typedef struct xen_msr_entry {
>>>   } xen_msr_entry_t;
>>>   DEFINE_XEN_GUEST_HANDLE(xen_msr_entry_t);
>>>   
>>> +struct arch_physinfo {
>>> +};
>>>   #endif /* !__ASSEMBLY__ */
>> While the term "p2m_ipa_bits" surely isn't arch-agnostic, I wonder
>> whether the expressed information is (the x86 equivalent being
>> hap_paddr_bits, at a guess), and hence whether this really ought
>> to live in an arch-specific sub-struct.
> 
> Well, on Arm we calculate the number of the IPA bits based on the number 
> of PA bits when setting Stage 2 address translation.
> I might mistake, but what we currently have on Arm is "ipa_bits == 
> pa_bits". So, this means that information we need at the toolstack side 
> isn't really arch-specific and
> we could probably avoid introducing arch-specific sub-struct by suitable 
> renaming the field (pa_bits, paddr_bits, whatever).
> 
> We could even name the field as hap_paddr_bits. Although, I don't know 
> whether the hap_* is purely x86-ism, but I see there are several 
> mentions in the common code (hap_pt_share, use_hap_pt, etc). Any 
> suggestions?

Well, HAP or not - there is going to be a limit to a guest's
address bits. So I don't see why it would matter whether HAP is
an x86-specific term. If you wanted to express the guest nature
and distinguish it from "paddr_bits", how about "gaddr_bits" or
"gpaddr_bits"?

Jan
Oleksandr Tyshchenko Aug. 30, 2021, 4:14 p.m. UTC | #4 
On 30.08.21 16:16, Jan Beulich wrote:

Hi Jan

> On 29.08.2021 22:28, Oleksandr wrote:
>> On 16.08.21 10:33, Jan Beulich wrote:
>>> On 14.08.2021 01:28, Oleksandr Tyshchenko wrote:
>>>> --- a/xen/include/public/arch-arm.h
>>>> +++ b/xen/include/public/arch-arm.h
>>>> @@ -332,6 +332,11 @@ struct xen_arch_domainconfig {
>>>>         */
>>>>        uint32_t clock_frequency;
>>>>    };
>>>> +
>>>> +struct arch_physinfo {
>>>> +    /* Holds the bit size of IPAs in p2m tables. */
>>>> +    uint32_t p2m_ipa_bits;
>>>> +};
>>>>    #endif /* __XEN__ || __XEN_TOOLS__ */
>>>>    
>>>>    struct arch_vcpu_info {
>>>> --- a/xen/include/public/arch-x86/xen.h
>>>> +++ b/xen/include/public/arch-x86/xen.h
>>>> @@ -346,6 +346,8 @@ typedef struct xen_msr_entry {
>>>>    } xen_msr_entry_t;
>>>>    DEFINE_XEN_GUEST_HANDLE(xen_msr_entry_t);
>>>>    
>>>> +struct arch_physinfo {
>>>> +};
>>>>    #endif /* !__ASSEMBLY__ */
>>> While the term "p2m_ipa_bits" surely isn't arch-agnostic, I wonder
>>> whether the expressed information is (the x86 equivalent being
>>> hap_paddr_bits, at a guess), and hence whether this really ought
>>> to live in an arch-specific sub-struct.
>> Well, on Arm we calculate the number of the IPA bits based on the number
>> of PA bits when setting Stage 2 address translation.
>> I might mistake, but what we currently have on Arm is "ipa_bits ==
>> pa_bits". So, this means that information we need at the toolstack side
>> isn't really arch-specific and
>> we could probably avoid introducing arch-specific sub-struct by suitable
>> renaming the field (pa_bits, paddr_bits, whatever).
>>
>> We could even name the field as hap_paddr_bits. Although, I don't know
>> whether the hap_* is purely x86-ism, but I see there are several
>> mentions in the common code (hap_pt_share, use_hap_pt, etc). Any
>> suggestions?
> Well, HAP or not - there is going to be a limit to a guest's
> address bits. So I don't see why it would matter whether HAP is
> an x86-specific term.

agree


>   If you wanted to express the guest nature
> and distinguish it from "paddr_bits", how about "gaddr_bits" or
> "gpaddr_bits"?

ok, both sound fine to me, with a little preference for gpaddr_bits. So, 
will drop arch-specific sub-struct for the next version.

Thank you.


>
> Jan
>
diff mbox series

Patch

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index b9ba16d..fabd7fb 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -855,6 +855,13 @@  typedef struct libxl__ctx libxl_ctx;
  */
 #define LIBXL_HAVE_PHYSINFO_MAX_POSSIBLE_MFN 1
 
+ /*
+  * LIBXL_HAVE_PHYSINFO_ARCH
+  *
+  * If this is defined, libxl_physinfo has a "arch" field.
+  */
+ #define LIBXL_HAVE_PHYSINFO_ARCH 1
+
 /*
  * LIBXL_HAVE_DOMINFO_OUTSTANDING_MEMKB 1
  *
diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
index 204eb0b..5387d50 100644
--- a/tools/libs/light/libxl.c
+++ b/tools/libs/light/libxl.c
@@ -15,6 +15,7 @@ 
 #include "libxl_osdeps.h"
 
 #include "libxl_internal.h"
+#include "libxl_arch.h"
 
 int libxl_ctx_alloc(libxl_ctx **pctx, int version,
                     unsigned flags, xentoollog_logger * lg)
@@ -405,6 +406,8 @@  int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
     physinfo->cap_vmtrace =
         !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_vmtrace);
 
+    libxl__arch_get_physinfo(gc, physinfo, &xcphysinfo);
+
     GC_FREE;
     return 0;
 }
diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
index 8527fc5..f3c6e75 100644
--- a/tools/libs/light/libxl_arch.h
+++ b/tools/libs/light/libxl_arch.h
@@ -90,6 +90,11 @@  void libxl__arch_update_domain_config(libxl__gc *gc,
                                       libxl_domain_config *dst,
                                       const libxl_domain_config *src);
 
+_hidden
+void libxl__arch_get_physinfo(libxl__gc *gc,
+                              libxl_physinfo *to,
+                              xc_physinfo_t *from);
+
 #if defined(__i386__) || defined(__x86_64__)
 
 #define LAPIC_BASE_ADDRESS  0xfee00000
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6..7304e25 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -1236,6 +1236,13 @@  void libxl__arch_update_domain_config(libxl__gc *gc,
 {
 }
 
+void libxl__arch_get_physinfo(libxl__gc *gc,
+                              libxl_physinfo *to,
+                              xc_physinfo_t *from)
+{
+    to->arch_arm.p2m_ipa_bits = from->arch.p2m_ipa_bits;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 3f9fff6..519e787 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -1061,6 +1061,11 @@  libxl_physinfo = Struct("physinfo", [
     ("cap_shadow", bool),
     ("cap_iommu_hap_pt_share", bool),
     ("cap_vmtrace", bool),
+
+    ("arch_arm", Struct(None, [("p2m_ipa_bits", uint32),
+                              ])),
+    ("arch_x86", Struct(None, [
+                              ])),
     ], dir=DIR_OUT)
 
 libxl_connectorinfo = Struct("connectorinfo", [
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index 18c3c77..0fb13ee 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -882,6 +882,12 @@  void libxl__arch_update_domain_config(libxl__gc *gc,
                     libxl_defbool_val(src->b_info.arch_x86.msr_relaxed));
 }
 
+void libxl__arch_get_physinfo(libxl__gc *gc,
+                              libxl_physinfo *to,
+                              xc_physinfo_t *from)
+{
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
index f87944e..4e7e209 100644
--- a/xen/arch/arm/sysctl.c
+++ b/xen/arch/arm/sysctl.c
@@ -15,6 +15,7 @@ 
 void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
 {
     pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
+    pi->arch.p2m_ipa_bits = p2m_ipa_bits;
 }
 
 long arch_do_sysctl(struct xen_sysctl *sysctl,
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 64a2ca3..36b1eef 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -332,6 +332,11 @@  struct xen_arch_domainconfig {
      */
     uint32_t clock_frequency;
 };
+
+struct arch_physinfo {
+    /* Holds the bit size of IPAs in p2m tables. */
+    uint32_t p2m_ipa_bits;
+};
 #endif /* __XEN__ || __XEN_TOOLS__ */
 
 struct arch_vcpu_info {
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index 7acd94c..8d2c05e 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -346,6 +346,8 @@  typedef struct xen_msr_entry {
 } xen_msr_entry_t;
 DEFINE_XEN_GUEST_HANDLE(xen_msr_entry_t);
 
+struct arch_physinfo {
+};
 #endif /* !__ASSEMBLY__ */
 
 /*
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 039ccf8..2727f21 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -35,7 +35,7 @@ 
 #include "domctl.h"
 #include "physdev.h"
 
-#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013
+#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014
 
 /*
  * Read console content from Xen buffer ring.
@@ -120,6 +120,7 @@  struct xen_sysctl_physinfo {
     uint64_aligned_t outstanding_pages;
     uint64_aligned_t max_mfn; /* Largest possible MFN on this host */
     uint32_t hw_cap[8];
+    struct arch_physinfo arch;
 };
 
 /*