Subject: Re: [PATCH 2/4] xen: limit grant v2 interface to the v1 functionality
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-3-jgross@suse.com>
 <f9cc6652-c524-bf4f-73f0-494d16deedf0@oracle.com>
 <8c87dc06-bbc8-5279-d135-d3351032913d@suse.com>
 <2777c0f8-7555-fbf3-6e18-81cea2fd1a39@oracle.com>
 <7dd99241-f771-9fa2-8a6a-ce019dbe08b6@suse.com>
 <46a17191-916a-e039-7e2f-a197e6783fe4@oracle.com>
 <258bc8dd-c475-bd5d-dffc-8c77fbe3cca3@suse.com>
 <9e8e497a-49d1-6299-8101-dd1123f81fee@oracle.com>
 <ccea56f5-bfbe-6003-f7bd-d2477831b9e3@suse.com>
 <0ea7c145-9a86-f002-75d0-bc6b8be72867@oracle.com>
 <ebdfb0b3-599e-e92c-87e3-09fff2d7b523@suse.com>
 <83489c35-778d-e8f7-a3bf-ff2d1eeba8d3@oracle.com>
From: Juergen Gross <jgross@suse.com>
Message-ID: <f2881fa5-d053-0bad-db64-f6f99935bd17@suse.com>
Date: Thu, 14 Sep 2017 10:13:18 +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: <83489c35-778d-e8f7-a3bf-ff2d1eeba8d3@oracle.com>
Content-Type: text/plain; charset=windows-1252
Content-Language: de-DE
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3750
Lines: 78

On 13/09/17 18:20, Boris Ostrovsky wrote:
> On 09/13/2017 10:45 AM, Juergen Gross wrote:
>> On 13/09/17 15:50, Boris Ostrovsky wrote:
>>> On 09/13/2017 09:38 AM, Juergen Gross wrote:
>>>> On 13/09/17 15:22, Boris Ostrovsky wrote:
>>>>> On 09/12/2017 02:18 PM, Juergen Gross wrote:
>>>>>> On 12/09/17 18:21, Boris Ostrovsky wrote:
>>>>>>> On 09/12/2017 12:09 PM, Juergen Gross wrote:
>>>>>>>> On 12/09/17 18:05, Boris Ostrovsky wrote:
>>>>>>>>> On 09/12/2017 11:50 AM, Juergen Gross wrote:
>>>>>>>>>> On 12/09/17 17:44, Boris Ostrovsky wrote:
>>>>>>>>>>> On 09/08/2017 10:48 AM, Juergen Gross wrote:
>>>>>>>>>>>> As there is currently no user for sub-page grants or transient grants
>>>>>>>>>>>> remove that functionality. This at once makes it possible to switch
>>>>>>>>>>>> from grant v2 to grant v1 without restrictions, as there is no loss of
>>>>>>>>>>>> functionality other than the limited frame number width related to
>>>>>>>>>>>> the switch.
>>>>>>>>>>> But isn't that ABI violation? v2 is expected to support this (XSAs
>>>>>>>>>>> notwithstanding)
>>>>>>>>>> No, I don't think so.
>>>>>>>>>>
>>>>>>>>>> The hypervisor still supports it, but the domU (or dom0) isn't required
>>>>>>>>>> to make use of all the features IMHO. Or are you aware of any backend
>>>>>>>>>> querying the grant version of a frontend and acting in another way if v2
>>>>>>>>>> is detected?
>>>>>>>>> I am not aware of any but that doesn't mean that they don't (or won't)
>>>>>>>>> exist.
>>>>>>>> But isn't the frontend the one which is defining what is granted in
>>>>>>>> which way? How should there be an ABI breakage when the frontend just
>>>>>>>> isn't using sub-page or transitive grants?
>>>>>>> People may provide both front and backend drivers and frontends, knowing
>>>>>>> that v2 is available, could decide to use those features.
>>>>>> No, without the functions to use them it will be impossible.
>>>>> I don't follow this. Which functions? The ones this patch is removing?
>>>> Yes, just after having been added one patch earlier.
>>>>
>>>> Right now the Linux kernel doesn't support grant V2 at all. So there
>>>> surely is no driver making use of V2 features right now.
>>>>
>>>> Ican merge patches 1 and 2 in case you want. I just thought a pure
>>>> revert of the former V2 remove patch would be easier to review,
>>>> taking into account that the former V2 support was working in
>>>> production environments (and even back then there was no user of
>>>> sub-page or transient grants).
>>> No, I don't have problems with *how* you are doing this (revert fully
>>> first and then remove).
>>>
>>> I am just not sure that removing these functions is the way to go
>>> because we are ending up with partial implementation of v2. The fact
>>> that noone is/has been using these features is IMO not particularly
>>> relevant.
>>>
>>> If these two were optional features then it would have been reasonable
>>> to drop them.
>> Why does the kernel need to support all features of an interface?
>>
>> I'm quite sure there are lots of interfaces supported only partially in
>> the kernel.
> 
> I don't think partially supported interface is a supported interface.
> It's just something that has been working until now.
> 
>> Having support for functionality in the kernel not being used at all is
>> just adding dead code with a high potential to bitrot. I can't even test
>> this functionality right now, as there are no users of it. So I'd risk
>> adding something which is broken from the beginning. 
> 
> OK. That I haven't considered that.
> 
> BTW, why are you not removing (*update_trans_entry) definition from
> gnttab_ops? You are taking (*update_subpage_entry) out.

Just for having a reason to send V2. ;-)

Thanks for catching it.


Juergen