Subject: Re: [Xen-devel] [PATCH 1/4] xen: re-introduce support for grant v2
 interface
To: Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
References: <20170908144849.2958-1-jgross@suse.com>
 <20170908144849.2958-2-jgross@suse.com>
 <67503f2a-6d93-59c6-5a49-40fcb487859c@oracle.com>
From: Juergen Gross <jgross@suse.com>
Message-ID: <12021f54-9404-c92b-5368-440d14814dbd@suse.com>
Date: Tue, 12 Sep 2017 17:45:25 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <67503f2a-6d93-59c6-5a49-40fcb487859c@oracle.com>
Content-Type: text/plain; charset=utf-8
Content-Language: de-DE
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2138
Lines: 75

On 12/09/17 17:41, Boris Ostrovsky wrote:
> 
>> +int arch_gnttab_map_status(uint64_t *frames, unsigned long nr_gframes,
>> +			   unsigned long max_nr_gframes,
>> +			   grant_status_t **__shared)
>> +{
>> +	grant_status_t *shared = *__shared;
>> +	unsigned long addr;
>> +	unsigned long i;
>> +
>> +	if (shared == NULL)
>> +		*__shared = shared = gnttab_status_vm_area.area->addr;
>> +
>> +	addr = (unsigned long)shared;
>> +
>> +	for (i = 0; i < nr_gframes; i++) {
>> +		set_pte_at(&init_mm, addr, gnttab_status_vm_area.ptes[i],
>> +			   mfn_pte(frames[i], PAGE_KERNEL));
>> +		addr += PAGE_SIZE;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> This looks pretty much identical to arch_gnttab_map_shared() except for
> gnttab_shared_vm_area vs. gnttab_status_vm_area,which can be passed in
> as a parameter.
> 
>> +
>>  void arch_gnttab_unmap(void *shared, unsigned long nr_gframes)
>>  {
>> +	pte_t **ptes;
>>  	unsigned long addr;
>>  	unsigned long i;
>>  
>> +	if (shared == gnttab_status_vm_area.area->addr)
>> +		ptes = gnttab_status_vm_area.ptes;
>> +	else
>> +		ptes = gnttab_shared_vm_area.ptes;
>> +
>>  	addr = (unsigned long)shared;
>>  
>>  	for (i = 0; i < nr_gframes; i++) {
>> -		set_pte_at(&init_mm, addr, gnttab_shared_vm_area.ptes[i],
>> -			   __pte(0));
>> +		set_pte_at(&init_mm, addr, ptes[i], __pte(0));
>>  		addr += PAGE_SIZE;
>>  	}
> 
> And this too looks like can be factored out (to something like
> arch_update_gnttab()). But perhaps not in this patch.
> 
>>  }
>> @@ -102,12 +129,35 @@ static int arch_gnttab_valloc(struct gnttab_vm_area *area, unsigned nr_frames)
>>  	return 0;
>>  }
>>  
>> -int arch_gnttab_init(unsigned long nr_shared)
>> +static void arch_gnttab_vfree(struct gnttab_vm_area *area)
>>  {
>> +	free_vm_area(area->area);
>> +	kfree(area->ptes);
>> +}
> 
> Not sure there is need for this routine. It is used only once.
> 
> Also, as an overall comment -- It feels like there may be too many
> BUG_ON()s.

This patch is mostly a revert of David's patch removing grant v2 support
with just very few adaptions. I wanted to keep it as similar to the
former grant v2 support as possible.


Juergen