xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] xen: remove some checks for always present Xen features
@ 2021-04-22 15:10 Juergen Gross
  2021-04-22 15:10 ` [PATCH 1/3] xen: check required " Juergen Gross
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Juergen Gross @ 2021-04-22 15:10 UTC (permalink / raw)
  To: xen-devel, linux-kernel, x86
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	Peter Zijlstra

Some features of Xen can be assumed to be always present, so add a
central check to verify this being true and remove the other checks.

Juergen Gross (3):
  xen: check required Xen features
  xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests
  xen: assume XENFEAT_gnttab_map_avail_bits being set for pv guests

 arch/x86/xen/enlighten_pv.c | 12 ++----------
 arch/x86/xen/mmu_pv.c       |  4 ++--
 drivers/xen/features.c      | 18 ++++++++++++++++++
 drivers/xen/gntdev.c        | 36 ++----------------------------------
 4 files changed, 24 insertions(+), 46 deletions(-)

-- 
2.26.2



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/3] xen: check required Xen features
  2021-04-22 15:10 [PATCH 0/3] xen: remove some checks for always present Xen features Juergen Gross
@ 2021-04-22 15:10 ` Juergen Gross
  2021-04-22 15:26   ` Stefano Stabellini
  2021-05-10 12:11   ` Boris Ostrovsky
  2021-04-22 15:10 ` [PATCH 2/3] xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests Juergen Gross
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 17+ messages in thread
From: Juergen Gross @ 2021-04-22 15:10 UTC (permalink / raw)
  To: xen-devel, linux-kernel
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, Peter Zijlstra

Linux kernel is not supported to run on Xen versions older than 4.0.

Add tests for required Xen features always being present in Xen 4.0
and newer.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/features.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/xen/features.c b/drivers/xen/features.c
index 25c053b09605..60503299c9bc 100644
--- a/drivers/xen/features.c
+++ b/drivers/xen/features.c
@@ -9,13 +9,26 @@
 #include <linux/types.h>
 #include <linux/cache.h>
 #include <linux/export.h>
+#include <linux/printk.h>
 
 #include <asm/xen/hypercall.h>
 
+#include <xen/xen.h>
 #include <xen/interface/xen.h>
 #include <xen/interface/version.h>
 #include <xen/features.h>
 
+/*
+ * Linux kernel expects at least Xen 4.0.
+ *
+ * Assume some features to be available for that reason (depending on guest
+ * mode, of course).
+ */
+#define chk_feature(f) {						\
+		if (!xen_feature(f))					\
+			pr_err("Xen: feature %s not available!\n", #f);	\
+	}
+
 u8 xen_features[XENFEAT_NR_SUBMAPS * 32] __read_mostly;
 EXPORT_SYMBOL_GPL(xen_features);
 
@@ -31,4 +44,9 @@ void xen_setup_features(void)
 		for (j = 0; j < 32; j++)
 			xen_features[i * 32 + j] = !!(fi.submap & 1<<j);
 	}
+
+	if (xen_pv_domain()) {
+		chk_feature(XENFEAT_mmu_pt_update_preserve_ad);
+		chk_feature(XENFEAT_gnttab_map_avail_bits);
+	}
 }
-- 
2.26.2



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 2/3] xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests
  2021-04-22 15:10 [PATCH 0/3] xen: remove some checks for always present Xen features Juergen Gross
  2021-04-22 15:10 ` [PATCH 1/3] xen: check required " Juergen Gross
@ 2021-04-22 15:10 ` Juergen Gross
  2021-04-22 15:10 ` [PATCH 3/3] xen: assume XENFEAT_gnttab_map_avail_bits " Juergen Gross
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 17+ messages in thread
From: Juergen Gross @ 2021-04-22 15:10 UTC (permalink / raw)
  To: xen-devel, x86, linux-kernel
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
	Peter Zijlstra

XENFEAT_mmu_pt_update_preserve_ad is always set in Xen 4.0 and newer.
Remove coding assuming it might be zero.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/enlighten_pv.c | 12 ++----------
 arch/x86/xen/mmu_pv.c       |  4 ++--
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index dc0a337f985b..e754927feac7 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -116,9 +116,8 @@ static void __init xen_banner(void)
 	HYPERVISOR_xen_version(XENVER_extraversion, &extra);
 
 	pr_info("Booting paravirtualized kernel on %s\n", pv_info.name);
-	printk(KERN_INFO "Xen version: %d.%d%s%s\n",
-	       version >> 16, version & 0xffff, extra.extraversion,
-	       xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? " (preserve-AD)" : "");
+	pr_info("Xen version: %d.%d%s (preserve-AD)\n",
+		version >> 16, version & 0xffff, extra.extraversion);
 }
 
 static void __init xen_pv_init_platform(void)
@@ -1303,13 +1302,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
 	xen_init_apic();
 #endif
 
-	if (xen_feature(XENFEAT_mmu_pt_update_preserve_ad)) {
-		pv_ops.mmu.ptep_modify_prot_start =
-			xen_ptep_modify_prot_start;
-		pv_ops.mmu.ptep_modify_prot_commit =
-			xen_ptep_modify_prot_commit;
-	}
-
 	machine_ops = xen_machine_ops;
 
 	/*
diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index cf2ade864c30..359c711336a8 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -2100,8 +2100,8 @@ static const struct pv_mmu_ops xen_mmu_ops __initconst = {
 	.set_pte = xen_set_pte_init,
 	.set_pmd = xen_set_pmd_hyper,
 
-	.ptep_modify_prot_start = __ptep_modify_prot_start,
-	.ptep_modify_prot_commit = __ptep_modify_prot_commit,
+	.ptep_modify_prot_start = xen_ptep_modify_prot_start,
+	.ptep_modify_prot_commit = xen_ptep_modify_prot_commit,
 
 	.pte_val = PV_CALLEE_SAVE(xen_pte_val),
 	.pgd_val = PV_CALLEE_SAVE(xen_pgd_val),
-- 
2.26.2



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 3/3] xen: assume XENFEAT_gnttab_map_avail_bits being set for pv guests
  2021-04-22 15:10 [PATCH 0/3] xen: remove some checks for always present Xen features Juergen Gross
  2021-04-22 15:10 ` [PATCH 1/3] xen: check required " Juergen Gross
  2021-04-22 15:10 ` [PATCH 2/3] xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests Juergen Gross
@ 2021-04-22 15:10 ` Juergen Gross
  2021-04-22 15:16 ` [PATCH 0/3] xen: remove some checks for always present Xen features Jan Beulich
  2021-05-10  7:34 ` Juergen Gross
  4 siblings, 0 replies; 17+ messages in thread
From: Juergen Gross @ 2021-04-22 15:10 UTC (permalink / raw)
  To: xen-devel, linux-kernel
  Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini, Peter Zijlstra

XENFEAT_gnttab_map_avail_bits is always set in Xen 4.0 and newer.
Remove coding assuming it might be zero.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/gntdev.c | 36 ++----------------------------------
 1 file changed, 2 insertions(+), 34 deletions(-)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index f01d58c7a042..93818816e137 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -266,20 +266,13 @@ static int find_grant_ptes(pte_t *pte, unsigned long addr, void *data)
 {
 	struct gntdev_grant_map *map = data;
 	unsigned int pgnr = (addr - map->vma->vm_start) >> PAGE_SHIFT;
-	int flags = map->flags | GNTMAP_application_map | GNTMAP_contains_pte;
+	int flags = map->flags | GNTMAP_application_map | GNTMAP_contains_pte |
+		    (1 << _GNTMAP_guest_avail0);
 	u64 pte_maddr;
 
 	BUG_ON(pgnr >= map->count);
 	pte_maddr = arbitrary_virt_to_machine(pte).maddr;
 
-	/*
-	 * Set the PTE as special to force get_user_pages_fast() fall
-	 * back to the slow path.  If this is not supported as part of
-	 * the grant map, it will be done afterwards.
-	 */
-	if (xen_feature(XENFEAT_gnttab_map_avail_bits))
-		flags |= (1 << _GNTMAP_guest_avail0);
-
 	gnttab_set_map_op(&map->map_ops[pgnr], pte_maddr, flags,
 			  map->grants[pgnr].ref,
 			  map->grants[pgnr].domid);
@@ -288,14 +281,6 @@ static int find_grant_ptes(pte_t *pte, unsigned long addr, void *data)
 	return 0;
 }
 
-#ifdef CONFIG_X86
-static int set_grant_ptes_as_special(pte_t *pte, unsigned long addr, void *data)
-{
-	set_pte_at(current->mm, addr, pte, pte_mkspecial(*pte));
-	return 0;
-}
-#endif
-
 int gntdev_map_grant_pages(struct gntdev_grant_map *map)
 {
 	int i, err = 0;
@@ -1053,23 +1038,6 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma)
 		err = vm_map_pages_zero(vma, map->pages, map->count);
 		if (err)
 			goto out_put_map;
-	} else {
-#ifdef CONFIG_X86
-		/*
-		 * If the PTEs were not made special by the grant map
-		 * hypercall, do so here.
-		 *
-		 * This is racy since the mapping is already visible
-		 * to userspace but userspace should be well-behaved
-		 * enough to not touch it until the mmap() call
-		 * returns.
-		 */
-		if (!xen_feature(XENFEAT_gnttab_map_avail_bits)) {
-			apply_to_page_range(vma->vm_mm, vma->vm_start,
-					    vma->vm_end - vma->vm_start,
-					    set_grant_ptes_as_special, NULL);
-		}
-#endif
 	}
 
 	return 0;
-- 
2.26.2



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] xen: remove some checks for always present Xen features
  2021-04-22 15:10 [PATCH 0/3] xen: remove some checks for always present Xen features Juergen Gross
                   ` (2 preceding siblings ...)
  2021-04-22 15:10 ` [PATCH 3/3] xen: assume XENFEAT_gnttab_map_avail_bits " Juergen Gross
@ 2021-04-22 15:16 ` Jan Beulich
  2021-04-22 15:17   ` Juergen Gross
  2021-05-10  7:34 ` Juergen Gross
  4 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2021-04-22 15:16 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, Peter Zijlstra,
	linux-kernel, x86, xen-devel

On 22.04.2021 17:10, Juergen Gross wrote:
> Some features of Xen can be assumed to be always present, so add a
> central check to verify this being true and remove the other checks.
> 
> Juergen Gross (3):
>   xen: check required Xen features
>   xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests
>   xen: assume XENFEAT_gnttab_map_avail_bits being set for pv guests

I wonder whether it's a good idea to infer feature presence from
version numbers. If (at some point in the past) you had inferred
gnttab v2 being available by version, this would have been broken
by its availability becoming controllable by a command line option
in Xen.

Jan


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] xen: remove some checks for always present Xen features
  2021-04-22 15:16 ` [PATCH 0/3] xen: remove some checks for always present Xen features Jan Beulich
@ 2021-04-22 15:17   ` Juergen Gross
  2021-04-22 15:23     ` Jan Beulich
  0 siblings, 1 reply; 17+ messages in thread
From: Juergen Gross @ 2021-04-22 15:17 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, Peter Zijlstra,
	linux-kernel, x86, xen-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 848 bytes --]

On 22.04.21 17:16, Jan Beulich wrote:
> On 22.04.2021 17:10, Juergen Gross wrote:
>> Some features of Xen can be assumed to be always present, so add a
>> central check to verify this being true and remove the other checks.
>>
>> Juergen Gross (3):
>>    xen: check required Xen features
>>    xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests
>>    xen: assume XENFEAT_gnttab_map_avail_bits being set for pv guests
> 
> I wonder whether it's a good idea to infer feature presence from
> version numbers. If (at some point in the past) you had inferred
> gnttab v2 being available by version, this would have been broken
> by its availability becoming controllable by a command line option
> in Xen.

I'm testing the feature to be really present when booting and issue a
message if it is not there.

Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] xen: remove some checks for always present Xen features
  2021-04-22 15:17   ` Juergen Gross
@ 2021-04-22 15:23     ` Jan Beulich
  2021-04-22 15:28       ` Juergen Gross
  0 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2021-04-22 15:23 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, Peter Zijlstra,
	linux-kernel, x86, xen-devel

On 22.04.2021 17:17, Juergen Gross wrote:
> On 22.04.21 17:16, Jan Beulich wrote:
>> On 22.04.2021 17:10, Juergen Gross wrote:
>>> Some features of Xen can be assumed to be always present, so add a
>>> central check to verify this being true and remove the other checks.
>>>
>>> Juergen Gross (3):
>>>    xen: check required Xen features
>>>    xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests
>>>    xen: assume XENFEAT_gnttab_map_avail_bits being set for pv guests
>>
>> I wonder whether it's a good idea to infer feature presence from
>> version numbers. If (at some point in the past) you had inferred
>> gnttab v2 being available by version, this would have been broken
>> by its availability becoming controllable by a command line option
>> in Xen.
> 
> I'm testing the feature to be really present when booting and issue a
> message if it is not there.

And how does this help if the feature really isn't there yet other code
assumes it is?

Jan


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/3] xen: check required Xen features
  2021-04-22 15:10 ` [PATCH 1/3] xen: check required " Juergen Gross
@ 2021-04-22 15:26   ` Stefano Stabellini
  2021-04-22 15:32     ` Juergen Gross
  2021-05-10 12:11   ` Boris Ostrovsky
  1 sibling, 1 reply; 17+ messages in thread
From: Stefano Stabellini @ 2021-04-22 15:26 UTC (permalink / raw)
  To: Juergen Gross
  Cc: xen-devel, linux-kernel, Boris Ostrovsky, Stefano Stabellini,
	Peter Zijlstra

On Thu, 22 Apr 2021, Juergen Gross wrote:
> Linux kernel is not supported to run on Xen versions older than 4.0.
> 
> Add tests for required Xen features always being present in Xen 4.0
> and newer.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/features.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/xen/features.c b/drivers/xen/features.c
> index 25c053b09605..60503299c9bc 100644
> --- a/drivers/xen/features.c
> +++ b/drivers/xen/features.c
> @@ -9,13 +9,26 @@
>  #include <linux/types.h>
>  #include <linux/cache.h>
>  #include <linux/export.h>
> +#include <linux/printk.h>
>  
>  #include <asm/xen/hypercall.h>
>  
> +#include <xen/xen.h>
>  #include <xen/interface/xen.h>
>  #include <xen/interface/version.h>
>  #include <xen/features.h>
>  
> +/*
> + * Linux kernel expects at least Xen 4.0.
> + *
> + * Assume some features to be available for that reason (depending on guest
> + * mode, of course).
> + */
> +#define chk_feature(f) {						\
> +		if (!xen_feature(f))					\
> +			pr_err("Xen: feature %s not available!\n", #f);	\
> +	}

I think this could be done as a static inline function in
include/xen/features.h. That way it would be available everywhere. Also,
static inlines are better than macro when it is possible to use them in
terms of code safety.


>  u8 xen_features[XENFEAT_NR_SUBMAPS * 32] __read_mostly;
>  EXPORT_SYMBOL_GPL(xen_features);
>  
> @@ -31,4 +44,9 @@ void xen_setup_features(void)
>  		for (j = 0; j < 32; j++)
>  			xen_features[i * 32 + j] = !!(fi.submap & 1<<j);
>  	}
> +
> +	if (xen_pv_domain()) {
> +		chk_feature(XENFEAT_mmu_pt_update_preserve_ad);
> +		chk_feature(XENFEAT_gnttab_map_avail_bits);
> +	}
>  }
> -- 
> 2.26.2
> 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] xen: remove some checks for always present Xen features
  2021-04-22 15:23     ` Jan Beulich
@ 2021-04-22 15:28       ` Juergen Gross
  2021-04-22 15:42         ` Jan Beulich
  0 siblings, 1 reply; 17+ messages in thread
From: Juergen Gross @ 2021-04-22 15:28 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, Peter Zijlstra,
	linux-kernel, x86, xen-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 1204 bytes --]

On 22.04.21 17:23, Jan Beulich wrote:
> On 22.04.2021 17:17, Juergen Gross wrote:
>> On 22.04.21 17:16, Jan Beulich wrote:
>>> On 22.04.2021 17:10, Juergen Gross wrote:
>>>> Some features of Xen can be assumed to be always present, so add a
>>>> central check to verify this being true and remove the other checks.
>>>>
>>>> Juergen Gross (3):
>>>>     xen: check required Xen features
>>>>     xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests
>>>>     xen: assume XENFEAT_gnttab_map_avail_bits being set for pv guests
>>>
>>> I wonder whether it's a good idea to infer feature presence from
>>> version numbers. If (at some point in the past) you had inferred
>>> gnttab v2 being available by version, this would have been broken
>>> by its availability becoming controllable by a command line option
>>> in Xen.
>>
>> I'm testing the feature to be really present when booting and issue a
>> message if it is not there.
> 
> And how does this help if the feature really isn't there yet other code
> assumes it is?

Did you look at the features I'm testing? Those are really just low
level additions I can't imagine will ever be removed again.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/3] xen: check required Xen features
  2021-04-22 15:26   ` Stefano Stabellini
@ 2021-04-22 15:32     ` Juergen Gross
  0 siblings, 0 replies; 17+ messages in thread
From: Juergen Gross @ 2021-04-22 15:32 UTC (permalink / raw)
  To: Stefano Stabellini
  Cc: xen-devel, linux-kernel, Boris Ostrovsky, Peter Zijlstra


[-- Attachment #1.1.1: Type: text/plain, Size: 1746 bytes --]

On 22.04.21 17:26, Stefano Stabellini wrote:
> On Thu, 22 Apr 2021, Juergen Gross wrote:
>> Linux kernel is not supported to run on Xen versions older than 4.0.
>>
>> Add tests for required Xen features always being present in Xen 4.0
>> and newer.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   drivers/xen/features.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/xen/features.c b/drivers/xen/features.c
>> index 25c053b09605..60503299c9bc 100644
>> --- a/drivers/xen/features.c
>> +++ b/drivers/xen/features.c
>> @@ -9,13 +9,26 @@
>>   #include <linux/types.h>
>>   #include <linux/cache.h>
>>   #include <linux/export.h>
>> +#include <linux/printk.h>
>>   
>>   #include <asm/xen/hypercall.h>
>>   
>> +#include <xen/xen.h>
>>   #include <xen/interface/xen.h>
>>   #include <xen/interface/version.h>
>>   #include <xen/features.h>
>>   
>> +/*
>> + * Linux kernel expects at least Xen 4.0.
>> + *
>> + * Assume some features to be available for that reason (depending on guest
>> + * mode, of course).
>> + */
>> +#define chk_feature(f) {						\
>> +		if (!xen_feature(f))					\
>> +			pr_err("Xen: feature %s not available!\n", #f);	\
>> +	}
> 
> I think this could be done as a static inline function in
> include/xen/features.h. That way it would be available everywhere. Also,
> static inlines are better than macro when it is possible to use them in
> terms of code safety.

It is a macro in order to have only one parameter.

And being a local macro is rendering the code safety reasoning moot.

Additionally I don't want this testing to be scattered all over the
code base. It should be done in one place only.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] xen: remove some checks for always present Xen features
  2021-04-22 15:28       ` Juergen Gross
@ 2021-04-22 15:42         ` Jan Beulich
  2021-04-22 15:49           ` Juergen Gross
  2021-04-22 15:51           ` Andrew Cooper
  0 siblings, 2 replies; 17+ messages in thread
From: Jan Beulich @ 2021-04-22 15:42 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, Peter Zijlstra,
	linux-kernel, x86, xen-devel

On 22.04.2021 17:28, Juergen Gross wrote:
> On 22.04.21 17:23, Jan Beulich wrote:
>> On 22.04.2021 17:17, Juergen Gross wrote:
>>> On 22.04.21 17:16, Jan Beulich wrote:
>>>> On 22.04.2021 17:10, Juergen Gross wrote:
>>>>> Some features of Xen can be assumed to be always present, so add a
>>>>> central check to verify this being true and remove the other checks.
>>>>>
>>>>> Juergen Gross (3):
>>>>>     xen: check required Xen features
>>>>>     xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests
>>>>>     xen: assume XENFEAT_gnttab_map_avail_bits being set for pv guests
>>>>
>>>> I wonder whether it's a good idea to infer feature presence from
>>>> version numbers. If (at some point in the past) you had inferred
>>>> gnttab v2 being available by version, this would have been broken
>>>> by its availability becoming controllable by a command line option
>>>> in Xen.
>>>
>>> I'm testing the feature to be really present when booting and issue a
>>> message if it is not there.
>>
>> And how does this help if the feature really isn't there yet other code
>> assumes it is?
> 
> Did you look at the features I'm testing?

I did, yes.

> Those are really just low
> level additions I can't imagine will ever be removed again.

I don't expect them to be removed. But I don't think the people having
contributed gnttab v2 expected any such for it, either.

Jan


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] xen: remove some checks for always present Xen features
  2021-04-22 15:42         ` Jan Beulich
@ 2021-04-22 15:49           ` Juergen Gross
  2021-04-22 15:51           ` Andrew Cooper
  1 sibling, 0 replies; 17+ messages in thread
From: Juergen Gross @ 2021-04-22 15:49 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, Peter Zijlstra,
	linux-kernel, x86, xen-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 2038 bytes --]

On 22.04.21 17:42, Jan Beulich wrote:
> On 22.04.2021 17:28, Juergen Gross wrote:
>> On 22.04.21 17:23, Jan Beulich wrote:
>>> On 22.04.2021 17:17, Juergen Gross wrote:
>>>> On 22.04.21 17:16, Jan Beulich wrote:
>>>>> On 22.04.2021 17:10, Juergen Gross wrote:
>>>>>> Some features of Xen can be assumed to be always present, so add a
>>>>>> central check to verify this being true and remove the other checks.
>>>>>>
>>>>>> Juergen Gross (3):
>>>>>>      xen: check required Xen features
>>>>>>      xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests
>>>>>>      xen: assume XENFEAT_gnttab_map_avail_bits being set for pv guests
>>>>>
>>>>> I wonder whether it's a good idea to infer feature presence from
>>>>> version numbers. If (at some point in the past) you had inferred
>>>>> gnttab v2 being available by version, this would have been broken
>>>>> by its availability becoming controllable by a command line option
>>>>> in Xen.
>>>>
>>>> I'm testing the feature to be really present when booting and issue a
>>>> message if it is not there.
>>>
>>> And how does this help if the feature really isn't there yet other code
>>> assumes it is?
>>
>> Did you look at the features I'm testing?
> 
> I did, yes.
> 
>> Those are really just low
>> level additions I can't imagine will ever be removed again.
> 
> I don't expect them to be removed. But I don't think the people having
> contributed gnttab v2 expected any such for it, either.

There is a major difference here.

gnttab v2 was replacing an existing functionality by a more scalable,
but more complex solution.

The features I'm assuming to be present are basically repairing issues
which have been present due to omissions in the initial implementation.

Especially the XENFEAT_gnttab_map_avail_bits causes a racy workaround in
the kernel when not present. The race is only avoided in case the user
code is well-behaved. It is dom0 user code, yes, but nevertheless such
issues are never nice.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] xen: remove some checks for always present Xen features
  2021-04-22 15:42         ` Jan Beulich
  2021-04-22 15:49           ` Juergen Gross
@ 2021-04-22 15:51           ` Andrew Cooper
  1 sibling, 0 replies; 17+ messages in thread
From: Andrew Cooper @ 2021-04-22 15:51 UTC (permalink / raw)
  To: Jan Beulich, Juergen Gross
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, Peter Zijlstra,
	linux-kernel, x86, xen-devel

On 22/04/2021 16:42, Jan Beulich wrote:
> On 22.04.2021 17:28, Juergen Gross wrote:
>> On 22.04.21 17:23, Jan Beulich wrote:
>>> On 22.04.2021 17:17, Juergen Gross wrote:
>>>> On 22.04.21 17:16, Jan Beulich wrote:
>>>>> On 22.04.2021 17:10, Juergen Gross wrote:
>>>>>> Some features of Xen can be assumed to be always present, so add a
>>>>>> central check to verify this being true and remove the other checks.
>>>>>>
>>>>>> Juergen Gross (3):
>>>>>>     xen: check required Xen features
>>>>>>     xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests
>>>>>>     xen: assume XENFEAT_gnttab_map_avail_bits being set for pv guests
>>>>> I wonder whether it's a good idea to infer feature presence from
>>>>> version numbers. If (at some point in the past) you had inferred
>>>>> gnttab v2 being available by version, this would have been broken
>>>>> by its availability becoming controllable by a command line option
>>>>> in Xen.
>>>> I'm testing the feature to be really present when booting and issue a
>>>> message if it is not there.
>>> And how does this help if the feature really isn't there yet other code
>>> assumes it is?
>> Did you look at the features I'm testing?
> I did, yes.
>
>> Those are really just low
>> level additions I can't imagine will ever be removed again.
> I don't expect them to be removed. But I don't think the people having
> contributed gnttab v2 expected any such for it, either.

The trainwreck around gnttab v2 is a mistake I hope we're never going to
make again.  I don't think it's a useful argument here.

The logic is fine.  It's checking for the actual features in the ABI
upon which Linux depends.

Sure - someone could modify Xen to take the feature out, but they'd get
a red wall in CI as they break every Linux kernel released in the past
decade.

~Andrew



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] xen: remove some checks for always present Xen features
  2021-04-22 15:10 [PATCH 0/3] xen: remove some checks for always present Xen features Juergen Gross
                   ` (3 preceding siblings ...)
  2021-04-22 15:16 ` [PATCH 0/3] xen: remove some checks for always present Xen features Jan Beulich
@ 2021-05-10  7:34 ` Juergen Gross
  2021-05-10 11:31   ` Peter Zijlstra
  4 siblings, 1 reply; 17+ messages in thread
From: Juergen Gross @ 2021-05-10  7:34 UTC (permalink / raw)
  To: xen-devel, linux-kernel, x86
  Cc: Boris Ostrovsky, Stefano Stabellini, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, Peter Zijlstra


[-- Attachment #1.1.1: Type: text/plain, Size: 751 bytes --]

On 22.04.21 17:10, Juergen Gross wrote:
> Some features of Xen can be assumed to be always present, so add a
> central check to verify this being true and remove the other checks.
> 
> Juergen Gross (3):
>    xen: check required Xen features
>    xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests
>    xen: assume XENFEAT_gnttab_map_avail_bits being set for pv guests
> 
>   arch/x86/xen/enlighten_pv.c | 12 ++----------
>   arch/x86/xen/mmu_pv.c       |  4 ++--
>   drivers/xen/features.c      | 18 ++++++++++++++++++
>   drivers/xen/gntdev.c        | 36 ++----------------------------------
>   4 files changed, 24 insertions(+), 46 deletions(-)
> 

Could I please get some feedback on this series?


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 0/3] xen: remove some checks for always present Xen features
  2021-05-10  7:34 ` Juergen Gross
@ 2021-05-10 11:31   ` Peter Zijlstra
  0 siblings, 0 replies; 17+ messages in thread
From: Peter Zijlstra @ 2021-05-10 11:31 UTC (permalink / raw)
  To: Juergen Gross
  Cc: xen-devel, linux-kernel, x86, Boris Ostrovsky,
	Stefano Stabellini, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, H. Peter Anvin

On Mon, May 10, 2021 at 09:34:18AM +0200, Juergen Gross wrote:
> On 22.04.21 17:10, Juergen Gross wrote:
> > Some features of Xen can be assumed to be always present, so add a
> > central check to verify this being true and remove the other checks.
> > 
> > Juergen Gross (3):
> >    xen: check required Xen features
> >    xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests
> >    xen: assume XENFEAT_gnttab_map_avail_bits being set for pv guests
> > 
> >   arch/x86/xen/enlighten_pv.c | 12 ++----------
> >   arch/x86/xen/mmu_pv.c       |  4 ++--
> >   drivers/xen/features.c      | 18 ++++++++++++++++++
> >   drivers/xen/gntdev.c        | 36 ++----------------------------------
> >   4 files changed, 24 insertions(+), 46 deletions(-)
> > 
> 
> Could I please get some feedback on this series?

I'm obviously in favour, but given I'm not an actual Xen user that might
not be worth much, still:

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/3] xen: check required Xen features
  2021-04-22 15:10 ` [PATCH 1/3] xen: check required " Juergen Gross
  2021-04-22 15:26   ` Stefano Stabellini
@ 2021-05-10 12:11   ` Boris Ostrovsky
  2021-05-10 13:21     ` Juergen Gross
  1 sibling, 1 reply; 17+ messages in thread
From: Boris Ostrovsky @ 2021-05-10 12:11 UTC (permalink / raw)
  To: Juergen Gross, xen-devel, linux-kernel; +Cc: Stefano Stabellini, Peter Zijlstra


On 4/22/21 11:10 AM, Juergen Gross wrote:
>  
> +/*
> + * Linux kernel expects at least Xen 4.0.
> + *
> + * Assume some features to be available for that reason (depending on guest
> + * mode, of course).
> + */
> +#define chk_feature(f) {						\
> +		if (!xen_feature(f))					\
> +			pr_err("Xen: feature %s not available!\n", #f);	\
> +	}


With your changes in the subsequent patches, are we still going to function properly without those features? (i.e. maybe we should just panic)


(Also, chk_required_features() perhaps?)


-boris


> +
>  u8 xen_features[XENFEAT_NR_SUBMAPS * 32] __read_mostly;
>  EXPORT_SYMBOL_GPL(xen_features);
>  
> @@ -31,4 +44,9 @@ void xen_setup_features(void)
>  		for (j = 0; j < 32; j++)
>  			xen_features[i * 32 + j] = !!(fi.submap & 1<<j);
>  	}
> +
> +	if (xen_pv_domain()) {
> +		chk_feature(XENFEAT_mmu_pt_update_preserve_ad);
> +		chk_feature(XENFEAT_gnttab_map_avail_bits);
> +	}
>  }


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/3] xen: check required Xen features
  2021-05-10 12:11   ` Boris Ostrovsky
@ 2021-05-10 13:21     ` Juergen Gross
  0 siblings, 0 replies; 17+ messages in thread
From: Juergen Gross @ 2021-05-10 13:21 UTC (permalink / raw)
  To: Boris Ostrovsky, xen-devel, linux-kernel
  Cc: Stefano Stabellini, Peter Zijlstra


[-- Attachment #1.1.1: Type: text/plain, Size: 1087 bytes --]

On 10.05.21 14:11, Boris Ostrovsky wrote:
> 
> On 4/22/21 11:10 AM, Juergen Gross wrote:
>>   
>> +/*
>> + * Linux kernel expects at least Xen 4.0.
>> + *
>> + * Assume some features to be available for that reason (depending on guest
>> + * mode, of course).
>> + */
>> +#define chk_feature(f) {						\
>> +		if (!xen_feature(f))					\
>> +			pr_err("Xen: feature %s not available!\n", #f);	\
>> +	}
> 
> 
> With your changes in the subsequent patches, are we still going to function properly without those features? (i.e. maybe we should just panic)

Depends on the use case.

XENFEAT_gnttab_map_avail_bits is relevant for driver domains using
user space backends only. In case it is not available "interesting"
things might happen.

XENFEAT_mmu_pt_update_preserve_ad not being present would result in
a subsequent mmu-update function using that feature returning -ENOSYS,
so this wouldn't be unrecognized.

So panic() might be a good idea in case the features are not available.

> (Also, chk_required_features() perhaps?)

Fine with me.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-05-10 13:22 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-22 15:10 [PATCH 0/3] xen: remove some checks for always present Xen features Juergen Gross
2021-04-22 15:10 ` [PATCH 1/3] xen: check required " Juergen Gross
2021-04-22 15:26   ` Stefano Stabellini
2021-04-22 15:32     ` Juergen Gross
2021-05-10 12:11   ` Boris Ostrovsky
2021-05-10 13:21     ` Juergen Gross
2021-04-22 15:10 ` [PATCH 2/3] xen: assume XENFEAT_mmu_pt_update_preserve_ad being set for pv guests Juergen Gross
2021-04-22 15:10 ` [PATCH 3/3] xen: assume XENFEAT_gnttab_map_avail_bits " Juergen Gross
2021-04-22 15:16 ` [PATCH 0/3] xen: remove some checks for always present Xen features Jan Beulich
2021-04-22 15:17   ` Juergen Gross
2021-04-22 15:23     ` Jan Beulich
2021-04-22 15:28       ` Juergen Gross
2021-04-22 15:42         ` Jan Beulich
2021-04-22 15:49           ` Juergen Gross
2021-04-22 15:51           ` Andrew Cooper
2021-05-10  7:34 ` Juergen Gross
2021-05-10 11:31   ` Peter Zijlstra

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).