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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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?

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


Thanks,
Razvan

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