Re: [Xen-devel] [PATCH v4] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.10.17 at 20:08, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 10/10/2017 05:24 PM, Jan Beulich wrote:
>>>>> On 10.10.17 at 16:13, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
>>> On Ma, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:> > 
>>>>>>
>>>>>>>
>>>>>>> a.u.set_mem_access_multi.pfn_list,
>>>>> +                                      a.u.set_mem_access_multi.acc
>>>>> ess_list,
>>>>> +                                      a.u.set_mem_access_multi.nr,
>>>>> +                                      a.u.set_mem_access_multi.opa
>>>>> que,
>>>>> +                                      MEMOP_CMD_MASK,
>>>> This not being a memop, re-using this value looks arbitrary.
>>>> Unless it absolutely has to be this value, please either add a brief
>>>> comment saying this just happens to fit your need or use a literal
>>>> constant.
>>> I will add a comment to clarify the reason for using MEMOP_CMD_MASK:
>>> e.g.
>>> /* MEMOP_CMD_MASK is used here to mirror the way
>>> p2m_set_mem_access_multi() is called for the
>>> XENMEM_access_op_set_access_multi case. */
>> 
>> But that's neither brief nor an actual reason (if at all it is a cosmetic
>> one). A wider or more narrow mask would do, afaict.
> 
> Revisiting the p2m_set_mem_access_multi() code, the mask is used here:
> 
> 447         /* Check for continuation if it's not the last iteration. */
> 448         if ( nr > ++start && !(start & mask) && hypercall_preempt_check() 
> )
> 449         {
> 450             rc = start;
> 451             break;
> 452         }
> 
> If I'm reading the code correctly, MEMOP_CMD_MASK happens to be
> 0000000000111111, which allows an interruption in the processing loop
> every 64th time we go through it.
> 
> Now, it does indeed make more sense to see MEMOP_CMD_MASK being used
> with XENMEM_access_op_set_access_multi than it does with altp2m (where
> we're not dealing with memops). However, indeed almost any mask would do
> here (0x1f, for example, or 0x7f, or something else entirely).
> 
> The reason I've initially picked MEMOP_CMD_MASK for this patch is that
> it had seemed like a reasonable default (currenly made use of with
> XENMEM_access_op_set_access_multi). I'm quite likely missing something,
> but I don't know what criteria we should use for picking a good value
> here, and how to express it. And if we go with the tried and true
> MEMOP_CMD_MASK (because it seems to work well), is it not appropriate to
> express that intent by using its name in the code?

My point is that MEMOP_CMD_MASK can't be changed, i.e. other
than the value to be used here it is not arbitrary.

> Should we just use it's value directly (i.e. 0x3f)?

That's certainly an option.

Jan


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