diff mbox series

[V2,1/3] xen: Introduce "gpaddr_bits" field to XEN_SYSCTL_physinfo

Message ID 1631297924-8658-2-git-send-email-olekstysh@gmail.com (mailing list archive)
State Superseded
Headers show
Series Add handling of extended regions (safe ranges) on Arm (Was "xen/memory: Introduce a hypercall to provide unallocated space") | expand

Commit Message

Oleksandr Tyshchenko Sept. 10, 2021, 6:18 p.m. UTC 
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

We need to pass info about maximum supported guest address
space size to the toolstack on Arm in order to properly
calculate the base and size of the extended region (safe range)
for the guest. The extended region is unused address space which
could be safely used by domain for foreign/grant mappings on Arm.
The extended region itself will be handled by the subsequents
patch.

Use p2m_ipa_bits variable on Arm, the x86 equivalent is
hap_paddr_bits.

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>
---
Please note, that recent review comments [1] haven't been addressed yet.
It is not forgotten, some clarification is needed. It will be addressed for the next version.

[1] https://lore.kernel.org/xen-devel/973f5344-aa10-3ad6-ff02-ad5f358ad279@citrix.com/

Changes since RFC:
   - update patch subject/description
   - replace arch-specific sub-struct with common gpaddr_bits
     field and update code to reflect that
---
 tools/include/libxl.h            | 7 +++++++
 tools/libs/light/libxl.c         | 2 ++
 tools/libs/light/libxl_types.idl | 2 ++
 xen/arch/arm/sysctl.c            | 2 ++
 xen/arch/x86/sysctl.c            | 2 ++
 xen/include/public/sysctl.h      | 3 ++-
 6 files changed, 17 insertions(+), 1 deletion(-)

Comments

Jan Beulich Sept. 16, 2021, 2:49 p.m. UTC | #1 
On 10.09.2021 20:18, Oleksandr Tyshchenko wrote:
> --- 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_GPADDR_BITS
> +  *
> +  * If this is defined, libxl_physinfo has a "gpaddr_bits" field.
> +  */
> + #define LIBXL_HAVE_PHYSINFO_GPADDR_BITS 1

Nit: I don't think you mean to have leading blanks here?

> @@ -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];
> +    uint32_t gpaddr_bits;
>  };

Please make trailing padding explicit. I wonder whether this needs
to be a 32-bit field: I expect we would need a full new ABI by the
time we might reach 256 address bits. Otoh e.g. threads_per_core is
pretty certainly oversized as well ...

Jan
Oleksandr Tyshchenko Sept. 16, 2021, 3:43 p.m. UTC | #2 
On 16.09.21 17:49, Jan Beulich wrote:

Hi Jan

> On 10.09.2021 20:18, Oleksandr Tyshchenko wrote:
>> --- 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_GPADDR_BITS
>> +  *
>> +  * If this is defined, libxl_physinfo has a "gpaddr_bits" field.
>> +  */
>> + #define LIBXL_HAVE_PHYSINFO_GPADDR_BITS 1
> Nit: I don't think you mean to have leading blanks here?

Yes, will remove.


>
>> @@ -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];
>> +    uint32_t gpaddr_bits;
>>   };
> Please make trailing padding explicit. I wonder whether this needs
> to be a 32-bit field: I expect we would need a full new ABI by the
> time we might reach 256 address bits. Otoh e.g. threads_per_core is
> pretty certainly oversized as well ...

I take it, this is a suggestion to make the field uint8_t and add 
uint8_t pad[7] after?


Please note, these are still unaddressed review comments for the last 
version [1], with a suggestion to use domctl (new?).
Also it is not entirely clear to me regarding whether this field will 
even remain gpaddr_bits or became maxphysaddr for example.

[1] 
https://lore.kernel.org/xen-devel/973f5344-aa10-3ad6-ff02-ad5f358ad279@citrix.com/


>
> Jan
>
Jan Beulich Sept. 16, 2021, 3:47 p.m. UTC | #3 
On 16.09.2021 17:43, Oleksandr wrote:
> On 16.09.21 17:49, Jan Beulich wrote:
>> On 10.09.2021 20:18, Oleksandr Tyshchenko wrote:
>>> @@ -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];
>>> +    uint32_t gpaddr_bits;
>>>   };
>> Please make trailing padding explicit. I wonder whether this needs
>> to be a 32-bit field: I expect we would need a full new ABI by the
>> time we might reach 256 address bits. Otoh e.g. threads_per_core is
>> pretty certainly oversized as well ...
> 
> I take it, this is a suggestion to make the field uint8_t and add 
> uint8_t pad[7] after?

I view this as a viable option at least.

Jan
Oleksandr Tyshchenko Sept. 16, 2021, 4:05 p.m. UTC | #4 
On 16.09.21 18:47, Jan Beulich wrote:

Hi Jan

> On 16.09.2021 17:43, Oleksandr wrote:
>> On 16.09.21 17:49, Jan Beulich wrote:
>>> On 10.09.2021 20:18, Oleksandr Tyshchenko wrote:
>>>> @@ -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];
>>>> +    uint32_t gpaddr_bits;
>>>>    };
>>> Please make trailing padding explicit. I wonder whether this needs
>>> to be a 32-bit field: I expect we would need a full new ABI by the
>>> time we might reach 256 address bits. Otoh e.g. threads_per_core is
>>> pretty certainly oversized as well ...
>> I take it, this is a suggestion to make the field uint8_t and add
>> uint8_t pad[7] after?
> I view this as a viable option at least.

I got it, sounds reasonable.


>
> Jan
>
diff mbox series

Patch

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index b9ba16d..da44944 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_GPADDR_BITS
+  *
+  * If this is defined, libxl_physinfo has a "gpaddr_bits" field.
+  */
+ #define LIBXL_HAVE_PHYSINFO_GPADDR_BITS 1
+
 /*
  * LIBXL_HAVE_DOMINFO_OUTSTANDING_MEMKB 1
  *
diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
index 204eb0b..c86624f 100644
--- a/tools/libs/light/libxl.c
+++ b/tools/libs/light/libxl.c
@@ -405,6 +405,8 @@  int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
     physinfo->cap_vmtrace =
         !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_vmtrace);
 
+    physinfo->gpaddr_bits = xcphysinfo.gpaddr_bits;
+
     GC_FREE;
     return 0;
 }
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 3f9fff6..f7437e4 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -1061,6 +1061,8 @@  libxl_physinfo = Struct("physinfo", [
     ("cap_shadow", bool),
     ("cap_iommu_hap_pt_share", bool),
     ("cap_vmtrace", bool),
+
+    ("gpaddr_bits", uint32),
     ], dir=DIR_OUT)
 
 libxl_connectorinfo = Struct("connectorinfo", [
diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
index f87944e..91dca4f 100644
--- a/xen/arch/arm/sysctl.c
+++ b/xen/arch/arm/sysctl.c
@@ -15,6 +15,8 @@ 
 void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
 {
     pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
+
+    pi->gpaddr_bits = p2m_ipa_bits;
 }
 
 long arch_do_sysctl(struct xen_sysctl *sysctl,
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index aff52a1..7b14865 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -135,6 +135,8 @@  void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap;
     if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_shadow;
+
+    pi->gpaddr_bits = hap_paddr_bits;
 }
 
 long arch_do_sysctl(
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 039ccf8..f53b42e 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];
+    uint32_t gpaddr_bits;
 };
 
 /*