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@xxxxxxxx>
>> ---
>>  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