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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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?


Thanks,
Razvan

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