[PATCH] xen: don't compile pv-specific parts if XEN_PV isn't configured

[PATCH] xen: don't compile pv-specific parts if XEN_PV isn't configured

[Juergen Gross]

xenbus_client.c contains some functions specific for pv guests.
Enclose them with #ifdef CONFIG_XEN_PV to avoid compiling them when
they are not needed (e.g. on ARM).

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/xenbus/xenbus_client.c | 130 +++++++++++++++++++------------------
 1 file changed, 67 insertions(+), 63 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index 82a8866758ee..a1c17000129b 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -519,64 +519,6 @@ static int __xenbus_map_ring(struct xenbus_device *dev,
 	return err;
 }
 
-static int xenbus_map_ring_valloc_pv(struct xenbus_device *dev,
-				     grant_ref_t *gnt_refs,
-				     unsigned int nr_grefs,
-				     void **vaddr)
-{
-	struct xenbus_map_node *node;
-	struct vm_struct *area;
-	pte_t *ptes[XENBUS_MAX_RING_GRANTS];
-	phys_addr_t phys_addrs[XENBUS_MAX_RING_GRANTS];
-	int err = GNTST_okay;
-	int i;
-	bool leaked;
-
-	*vaddr = NULL;
-
-	if (nr_grefs > XENBUS_MAX_RING_GRANTS)
-		return -EINVAL;
-
-	node = kzalloc(sizeof(*node), GFP_KERNEL);
-	if (!node)
-		return -ENOMEM;
-
-	area = alloc_vm_area(XEN_PAGE_SIZE * nr_grefs, ptes);
-	if (!area) {
-		kfree(node);
-		return -ENOMEM;
-	}
-
-	for (i = 0; i < nr_grefs; i++)
-		phys_addrs[i] = arbitrary_virt_to_machine(ptes[i]).maddr;
-
-	err = __xenbus_map_ring(dev, gnt_refs, nr_grefs, node->handles,
-				phys_addrs,
-				GNTMAP_host_map | GNTMAP_contains_pte,
-				&leaked);
-	if (err)
-		goto failed;
-
-	node->nr_handles = nr_grefs;
-	node->pv.area = area;
-
-	spin_lock(&xenbus_valloc_lock);
-	list_add(&node->next, &xenbus_valloc_pages);
-	spin_unlock(&xenbus_valloc_lock);
-
-	*vaddr = area->addr;
-	return 0;
-
-failed:
-	if (!leaked)
-		free_vm_area(area);
-	else
-		pr_alert("leaking VM area %p size %u page(s)", area, nr_grefs);
-
-	kfree(node);
-	return err;
-}
-
 struct map_ring_valloc_hvm
 {
 	unsigned int idx;
@@ -725,6 +667,65 @@ int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
 }
 EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
 
+#ifdef CONFIG_XEN_PV
+static int xenbus_map_ring_valloc_pv(struct xenbus_device *dev,
+				     grant_ref_t *gnt_refs,
+				     unsigned int nr_grefs,
+				     void **vaddr)
+{
+	struct xenbus_map_node *node;
+	struct vm_struct *area;
+	pte_t *ptes[XENBUS_MAX_RING_GRANTS];
+	phys_addr_t phys_addrs[XENBUS_MAX_RING_GRANTS];
+	int err = GNTST_okay;
+	int i;
+	bool leaked;
+
+	*vaddr = NULL;
+
+	if (nr_grefs > XENBUS_MAX_RING_GRANTS)
+		return -EINVAL;
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return -ENOMEM;
+
+	area = alloc_vm_area(XEN_PAGE_SIZE * nr_grefs, ptes);
+	if (!area) {
+		kfree(node);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < nr_grefs; i++)
+		phys_addrs[i] = arbitrary_virt_to_machine(ptes[i]).maddr;
+
+	err = __xenbus_map_ring(dev, gnt_refs, nr_grefs, node->handles,
+				phys_addrs,
+				GNTMAP_host_map | GNTMAP_contains_pte,
+				&leaked);
+	if (err)
+		goto failed;
+
+	node->nr_handles = nr_grefs;
+	node->pv.area = area;
+
+	spin_lock(&xenbus_valloc_lock);
+	list_add(&node->next, &xenbus_valloc_pages);
+	spin_unlock(&xenbus_valloc_lock);
+
+	*vaddr = area->addr;
+	return 0;
+
+failed:
+	if (!leaked)
+		free_vm_area(area);
+	else
+		pr_alert("leaking VM area %p size %u page(s)", area, nr_grefs);
+
+	kfree(node);
+	return err;
+}
+
 static int xenbus_unmap_ring_vfree_pv(struct xenbus_device *dev, void *vaddr)
 {
 	struct xenbus_map_node *node;
@@ -788,6 +789,12 @@ static int xenbus_unmap_ring_vfree_pv(struct xenbus_device *dev, void *vaddr)
 	return err;
 }
 
+static const struct xenbus_ring_ops ring_ops_pv = {
+	.map = xenbus_map_ring_valloc_pv,
+	.unmap = xenbus_unmap_ring_vfree_pv,
+};
+#endif
+
 struct unmap_ring_vfree_hvm
 {
 	unsigned int idx;
@@ -916,11 +923,6 @@ enum xenbus_state xenbus_read_driver_state(const char *path)
 }
 EXPORT_SYMBOL_GPL(xenbus_read_driver_state);
 
-static const struct xenbus_ring_ops ring_ops_pv = {
-	.map = xenbus_map_ring_valloc_pv,
-	.unmap = xenbus_unmap_ring_vfree_pv,
-};
-
 static const struct xenbus_ring_ops ring_ops_hvm = {
 	.map = xenbus_map_ring_valloc_hvm,
 	.unmap = xenbus_unmap_ring_vfree_hvm,
@@ -928,8 +930,10 @@ static const struct xenbus_ring_ops ring_ops_hvm = {
 
 void __init xenbus_ring_ops_init(void)
 {
+#ifdef CONFIG_XEN_PV
 	if (!xen_feature(XENFEAT_auto_translated_physmap))
 		ring_ops = &ring_ops_pv;
 	else
+#endif
 		ring_ops = &ring_ops_hvm;
 }
-- 
2.12.3

Re: [PATCH] xen: don't compile pv-specific parts if XEN_PV isn't configured

[Boris Ostrovsky]

On 09/14/2017 08:38 AM, Juergen Gross wrote:
> xenbus_client.c contains some functions specific for pv guests.
> Enclose them with #ifdef CONFIG_XEN_PV to avoid compiling them when
> they are not needed (e.g. on ARM).
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  drivers/xen/xenbus/xenbus_client.c | 130 +++++++++++++++++++------------------
>  1 file changed, 67 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index 82a8866758ee..a1c17000129b 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -519,64 +519,6 @@ static int __xenbus_map_ring(struct xenbus_device *dev,
>  	return err;
>  }
>  
> -static int xenbus_map_ring_valloc_pv(struct xenbus_device *dev,
> -				     grant_ref_t *gnt_refs,
> -				     unsigned int nr_grefs,
> -				     void **vaddr)
> -{
> -	struct xenbus_map_node *node;
> -	struct vm_struct *area;
> -	pte_t *ptes[XENBUS_MAX_RING_GRANTS];
> -	phys_addr_t phys_addrs[XENBUS_MAX_RING_GRANTS];
> -	int err = GNTST_okay;
> -	int i;
> -	bool leaked;
> -
> -	*vaddr = NULL;
> -
> -	if (nr_grefs > XENBUS_MAX_RING_GRANTS)
> -		return -EINVAL;
> -
> -	node = kzalloc(sizeof(*node), GFP_KERNEL);
> -	if (!node)
> -		return -ENOMEM;
> -
> -	area = alloc_vm_area(XEN_PAGE_SIZE * nr_grefs, ptes);
> -	if (!area) {
> -		kfree(node);
> -		return -ENOMEM;
> -	}
> -
> -	for (i = 0; i < nr_grefs; i++)
> -		phys_addrs[i] = arbitrary_virt_to_machine(ptes[i]).maddr;
> -
> -	err = __xenbus_map_ring(dev, gnt_refs, nr_grefs, node->handles,
> -				phys_addrs,
> -				GNTMAP_host_map | GNTMAP_contains_pte,
> -				&leaked);
> -	if (err)
> -		goto failed;
> -
> -	node->nr_handles = nr_grefs;
> -	node->pv.area = area;
> -
> -	spin_lock(&xenbus_valloc_lock);
> -	list_add(&node->next, &xenbus_valloc_pages);
> -	spin_unlock(&xenbus_valloc_lock);
> -
> -	*vaddr = area->addr;
> -	return 0;
> -
> -failed:
> -	if (!leaked)
> -		free_vm_area(area);
> -	else
> -		pr_alert("leaking VM area %p size %u page(s)", area, nr_grefs);
> -
> -	kfree(node);
> -	return err;
> -}
> -
>  struct map_ring_valloc_hvm
>  {
>  	unsigned int idx;
> @@ -725,6 +667,65 @@ int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
>  }
>  EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
>  
> +#ifdef CONFIG_XEN_PV
> +static int xenbus_map_ring_valloc_pv(struct xenbus_device *dev,
> +				     grant_ref_t *gnt_refs,
> +				     unsigned int nr_grefs,
> +				     void **vaddr)
> +{
> +	struct xenbus_map_node *node;
> +	struct vm_struct *area;
> +	pte_t *ptes[XENBUS_MAX_RING_GRANTS];
> +	phys_addr_t phys_addrs[XENBUS_MAX_RING_GRANTS];
> +	int err = GNTST_okay;
> +	int i;
> +	bool leaked;
> +
> +	*vaddr = NULL;
> +
> +	if (nr_grefs > XENBUS_MAX_RING_GRANTS)
> +		return -EINVAL;
> +
> +	node = kzalloc(sizeof(*node), GFP_KERNEL);
> +	if (!node)
> +		return -ENOMEM;
> +
> +	area = alloc_vm_area(XEN_PAGE_SIZE * nr_grefs, ptes);
> +	if (!area) {
> +		kfree(node);
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < nr_grefs; i++)
> +		phys_addrs[i] = arbitrary_virt_to_machine(ptes[i]).maddr;
> +
> +	err = __xenbus_map_ring(dev, gnt_refs, nr_grefs, node->handles,
> +				phys_addrs,
> +				GNTMAP_host_map | GNTMAP_contains_pte,
> +				&leaked);
> +	if (err)
> +		goto failed;
> +
> +	node->nr_handles = nr_grefs;
> +	node->pv.area = area;
> +
> +	spin_lock(&xenbus_valloc_lock);
> +	list_add(&node->next, &xenbus_valloc_pages);
> +	spin_unlock(&xenbus_valloc_lock);
> +
> +	*vaddr = area->addr;
> +	return 0;
> +
> +failed:
> +	if (!leaked)
> +		free_vm_area(area);
> +	else
> +		pr_alert("leaking VM area %p size %u page(s)", area, nr_grefs);
> +
> +	kfree(node);
> +	return err;
> +}
> +

Did you make any changes in xenbus_map_ring_valloc_pv()? I don't see any
but the diff looks pretty big --- I'd expect only the preprocessor
directives to show up.

-boris

Re: [PATCH] xen: don't compile pv-specific parts if XEN_PV isn't configured

[Juergen Gross]

On 14/09/17 16:00, Boris Ostrovsky wrote:
> On 09/14/2017 08:38 AM, Juergen Gross wrote:
>> xenbus_client.c contains some functions specific for pv guests.
>> Enclose them with #ifdef CONFIG_XEN_PV to avoid compiling them when
>> they are not needed (e.g. on ARM).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  drivers/xen/xenbus/xenbus_client.c | 130 +++++++++++++++++++------------------
>>  1 file changed, 67 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
>> index 82a8866758ee..a1c17000129b 100644
>> --- a/drivers/xen/xenbus/xenbus_client.c
>> +++ b/drivers/xen/xenbus/xenbus_client.c
>> @@ -519,64 +519,6 @@ static int __xenbus_map_ring(struct xenbus_device *dev,
>>  	return err;
>>  }
>>  
>> -static int xenbus_map_ring_valloc_pv(struct xenbus_device *dev,
>> -				     grant_ref_t *gnt_refs,
>> -				     unsigned int nr_grefs,
>> -				     void **vaddr)
>> -{
>> -	struct xenbus_map_node *node;
>> -	struct vm_struct *area;
>> -	pte_t *ptes[XENBUS_MAX_RING_GRANTS];
>> -	phys_addr_t phys_addrs[XENBUS_MAX_RING_GRANTS];
>> -	int err = GNTST_okay;
>> -	int i;
>> -	bool leaked;
>> -
>> -	*vaddr = NULL;
>> -
>> -	if (nr_grefs > XENBUS_MAX_RING_GRANTS)
>> -		return -EINVAL;
>> -
>> -	node = kzalloc(sizeof(*node), GFP_KERNEL);
>> -	if (!node)
>> -		return -ENOMEM;
>> -
>> -	area = alloc_vm_area(XEN_PAGE_SIZE * nr_grefs, ptes);
>> -	if (!area) {
>> -		kfree(node);
>> -		return -ENOMEM;
>> -	}
>> -
>> -	for (i = 0; i < nr_grefs; i++)
>> -		phys_addrs[i] = arbitrary_virt_to_machine(ptes[i]).maddr;
>> -
>> -	err = __xenbus_map_ring(dev, gnt_refs, nr_grefs, node->handles,
>> -				phys_addrs,
>> -				GNTMAP_host_map | GNTMAP_contains_pte,
>> -				&leaked);
>> -	if (err)
>> -		goto failed;
>> -
>> -	node->nr_handles = nr_grefs;
>> -	node->pv.area = area;
>> -
>> -	spin_lock(&xenbus_valloc_lock);
>> -	list_add(&node->next, &xenbus_valloc_pages);
>> -	spin_unlock(&xenbus_valloc_lock);
>> -
>> -	*vaddr = area->addr;
>> -	return 0;
>> -
>> -failed:
>> -	if (!leaked)
>> -		free_vm_area(area);
>> -	else
>> -		pr_alert("leaking VM area %p size %u page(s)", area, nr_grefs);
>> -
>> -	kfree(node);
>> -	return err;
>> -}
>> -
>>  struct map_ring_valloc_hvm
>>  {
>>  	unsigned int idx;
>> @@ -725,6 +667,65 @@ int xenbus_unmap_ring_vfree(struct xenbus_device *dev, void *vaddr)
>>  }
>>  EXPORT_SYMBOL_GPL(xenbus_unmap_ring_vfree);
>>  
>> +#ifdef CONFIG_XEN_PV
>> +static int xenbus_map_ring_valloc_pv(struct xenbus_device *dev,
>> +				     grant_ref_t *gnt_refs,
>> +				     unsigned int nr_grefs,
>> +				     void **vaddr)
>> +{
>> +	struct xenbus_map_node *node;
>> +	struct vm_struct *area;
>> +	pte_t *ptes[XENBUS_MAX_RING_GRANTS];
>> +	phys_addr_t phys_addrs[XENBUS_MAX_RING_GRANTS];
>> +	int err = GNTST_okay;
>> +	int i;
>> +	bool leaked;
>> +
>> +	*vaddr = NULL;
>> +
>> +	if (nr_grefs > XENBUS_MAX_RING_GRANTS)
>> +		return -EINVAL;
>> +
>> +	node = kzalloc(sizeof(*node), GFP_KERNEL);
>> +	if (!node)
>> +		return -ENOMEM;
>> +
>> +	area = alloc_vm_area(XEN_PAGE_SIZE * nr_grefs, ptes);
>> +	if (!area) {
>> +		kfree(node);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	for (i = 0; i < nr_grefs; i++)
>> +		phys_addrs[i] = arbitrary_virt_to_machine(ptes[i]).maddr;
>> +
>> +	err = __xenbus_map_ring(dev, gnt_refs, nr_grefs, node->handles,
>> +				phys_addrs,
>> +				GNTMAP_host_map | GNTMAP_contains_pte,
>> +				&leaked);
>> +	if (err)
>> +		goto failed;
>> +
>> +	node->nr_handles = nr_grefs;
>> +	node->pv.area = area;
>> +
>> +	spin_lock(&xenbus_valloc_lock);
>> +	list_add(&node->next, &xenbus_valloc_pages);
>> +	spin_unlock(&xenbus_valloc_lock);
>> +
>> +	*vaddr = area->addr;
>> +	return 0;
>> +
>> +failed:
>> +	if (!leaked)
>> +		free_vm_area(area);
>> +	else
>> +		pr_alert("leaking VM area %p size %u page(s)", area, nr_grefs);
>> +
>> +	kfree(node);
>> +	return err;
>> +}
>> +
> 
> Did you make any changes in xenbus_map_ring_valloc_pv()? I don't see any
> but the diff looks pretty big --- I'd expect only the preprocessor
> directives to show up.

I moved the functions to require only one #ifdef (plus 1 for setting
the pv variants).


Juergen

Re: [PATCH] xen: don't compile pv-specific parts if XEN_PV isn't configured

[Boris Ostrovsky]

>> Did you make any changes in xenbus_map_ring_valloc_pv()? I don't see any
>> but the diff looks pretty big --- I'd expect only the preprocessor
>> directives to show up.
> I moved the functions to require only one #ifdef (plus 1 for setting
> the pv variants).

Oh, OK, I didn't notice that.

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Re: [PATCH] xen: don't compile pv-specific parts if XEN_PV isn't configured

[Tycho Andersen]

Hi Juergen,

On Thu, Sep 14, 2017 at 02:38:58PM +0200, Juergen Gross wrote:
> xenbus_client.c contains some functions specific for pv guests.
> Enclose them with #ifdef CONFIG_XEN_PV to avoid compiling them when
> they are not needed (e.g. on ARM).
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Thanks for this! I think we also need to drop the old definition,
something like the below. Can you fold this in or should I send it
separately?

Cheers,

Tycho

>From 410a0c15c354f1ba387bdac6837d0a2031744c56 Mon Sep 17 00:00:00 2001
From: Tycho Andersen <tycho@docker.com>
Date: Thu, 14 Sep 2017 08:57:30 -0700
Subject: [PATCH] xen, arm64: drop dummy lookup_address()

This is unused, and conflicts with the definition that we'll add for XPFO.

Signed-off-by: Tycho Andersen <tycho@docker.com>
---
 include/xen/arm/page.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
index 415dbc6e43fd..6adc2a955340 100644
--- a/include/xen/arm/page.h
+++ b/include/xen/arm/page.h
@@ -84,16 +84,6 @@ static inline xmaddr_t arbitrary_virt_to_machine(void *vaddr)
 	BUG();
 }
 
-/* TODO: this shouldn't be here but it is because the frontend drivers
- * are using it (its rolled in headers) even though we won't hit the code path.
- * So for right now just punt with this.
- */
-static inline pte_t *lookup_address(unsigned long address, unsigned int *level)
-{
-	BUG();
-	return NULL;
-}
-
 extern int set_foreign_p2m_mapping(struct gnttab_map_grant_ref *map_ops,
 				   struct gnttab_map_grant_ref *kmap_ops,
 				   struct page **pages, unsigned int count);
-- 
2.11.0

Re: [PATCH] xen: don't compile pv-specific parts if XEN_PV isn't configured

[Juergen Gross]

On 14/09/17 18:31, Tycho Andersen wrote:
> Hi Juergen,
> 
> On Thu, Sep 14, 2017 at 02:38:58PM +0200, Juergen Gross wrote:
>> xenbus_client.c contains some functions specific for pv guests.
>> Enclose them with #ifdef CONFIG_XEN_PV to avoid compiling them when
>> they are not needed (e.g. on ARM).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Thanks for this! I think we also need to drop the old definition,
> something like the below. Can you fold this in or should I send it
> separately?

Please send it separately, as it touches ARM code only and thus should
be Acked by the ARM Xen maintainer.


Juergen

> 
> Cheers,
> 
> Tycho
> 
> From 410a0c15c354f1ba387bdac6837d0a2031744c56 Mon Sep 17 00:00:00 2001
> From: Tycho Andersen <tycho@docker.com>
> Date: Thu, 14 Sep 2017 08:57:30 -0700
> Subject: [PATCH] xen, arm64: drop dummy lookup_address()
> 
> This is unused, and conflicts with the definition that we'll add for XPFO.
> 
> Signed-off-by: Tycho Andersen <tycho@docker.com>
> ---
>  include/xen/arm/page.h | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
> index 415dbc6e43fd..6adc2a955340 100644
> --- a/include/xen/arm/page.h
> +++ b/include/xen/arm/page.h
> @@ -84,16 +84,6 @@ static inline xmaddr_t arbitrary_virt_to_machine(void *vaddr)
>  	BUG();
>  }
>  
> -/* TODO: this shouldn't be here but it is because the frontend drivers
> - * are using it (its rolled in headers) even though we won't hit the code path.
> - * So for right now just punt with this.
> - */
> -static inline pte_t *lookup_address(unsigned long address, unsigned int *level)
> -{
> -	BUG();
> -	return NULL;
> -}
> -
>  extern int set_foreign_p2m_mapping(struct gnttab_map_grant_ref *map_ops,
>  				   struct gnttab_map_grant_ref *kmap_ops,
>  				   struct page **pages, unsigned int count);
> 

Re: [PATCH] xen: don't compile pv-specific parts if XEN_PV isn't configured

[Boris Ostrovsky]

On 09/14/2017 08:38 AM, Juergen Gross wrote:
> xenbus_client.c contains some functions specific for pv guests.
> Enclose them with #ifdef CONFIG_XEN_PV to avoid compiling them when
> they are not needed (e.g. on ARM).
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Applied to for-linus-14b.

-boris