Re: [Xen-devel] [PATCH V2] common/mem_access: merged mem_access setting interfaces

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Mar 20, 2017 at 2:20 PM, Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx> wrote:

On 03/20/2017 06:54 PM, Jan Beulich wrote:
>>>> On 20.03.17 at 17:48, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 03/20/2017 06:40 PM, Jan Beulich wrote:
>>>>>> On 20.03.17 at 17:16, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> On 03/20/2017 06:14 PM, Razvan Cojocaru wrote:
>>>>> In any case, back when I've added xc_set_mem_access_multi() I've also
>>>>> modified struct xen_mem_access_op in the same manner:
>>>>>
>>>>>
>>>> http://xenbits.xenproject.org/gitweb/?p=xen.git;a=commitdiff;h=1ef5056bd6274e
>>>> cbe065387b6cf45657d6d700cd
>>>>
>>>> Oh, nevermind, I think you're referring to the fact that I had back then
>>>> added members to the end of the structure, and so the old layout had
>>>> remained compatible. Point taken.
>>>
>>> Not just that - there you've also introduced a new sub-op.
>>
>> Yes, but by modifying the structure for use with
>> XENMEM_access_op_set_access_multi, it's now also changed for, e.g.,
>> XENMEM_access_op_set_access - since it's common to them. Other than the
>> place where the new data has been added, I believe that the same
>> critique would apply to the old patch, unless I'm misunderstanding
>> something.
>
> Indeed, strictly speaking that change wasn't really okay either,
> as someone passing the smaller structure near the end of a page
> might get -EFAULT back. So we're trying to do better this time ...

Two question remain to be answered then:

1. How should this proceed? Andrew's comment, taken together with Tamas'
last patch ("altp2m: Allow specifying external-only use-case") would
seem to imply that the best way forward is to revert to the previous
patch which duplicates the xc_set_mem_access_multi()'s mem-op with
xc_altp2m_set_mem_access_multi()'s HVMOP (with the compat code to be
worked out). Is there a consensus on this?

2. What is the best way to avoid incompatibilities such as the one
mentioned? For example, in this case, should I have deprecated
XENMEM_access_op_set_access and XENMEM_access_op_set_access_multi and
added XENMEM_access_op_set_access2 and XENMEM_access_op_set_access2? Or
do you have something different in mind?

IMHO deprecating the old memops shouldn't be needed. The code on the hypervisor side all lands in the same spot, so keeping both memops (and the hvmop) in place should add no extra work from a maintenance perspective.

Tamas 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel