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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

>> > +                                      a.u.set_mem_access_multi.vie
>> > w);
>> > +        if ( rc > 0 )
>> > +        {
>> > +            a.u.set_mem_access_multi.opaque = rc;
>> > +            if ( __copy_field_to_guest(guest_handle_cast(arg,
>> > xen_hvm_altp2m_op_t), &a, u.set_mem_access_multi.opaque) )
>> Long line.
>> 
>> Also please consider adding a prereq patch changing the function's
>> parameter to a properly typed handle, which will then make
>> unnecessary using the not obviously correct cast here. Other
>> copying back of individual fields in this function could then also be
>> switched to __copy_field_to_guest().
> 
> Actually, changing the function's parameter to a typed handle could
> complicate things a little for compat_altp2m_op. It should be modified
> also to use a typed handle (compat_hvm_altp2m_op_t), which in case of
> commands other than HVMOP_altp2m_set_mem_access_multi should be casted
> to xen_hvm_altp2m_op_t before calling do_altp2m_op.
> If the problem was only related to the code clarity, I think the new
> implementation will be a little more difficult to follow.

Hmm, likely true - it would help do_altp2m_op(), but would indeed
look to harm the new ad hoc compat wrapper you add.

>> > @@ -4586,6 +4613,57 @@ static int do_altp2m_op(
>> >      return rc;
>> >  }
>> >  
>> > +static int compat_altp2m_op(
>> > +    XEN_GUEST_HANDLE_PARAM(void) arg)
>> > +{
>> > +    struct compat_hvm_altp2m_op a;
>> > +    union
>> > +    {
>> > +        XEN_GUEST_HANDLE_PARAM(void) hnd;
>> > +        struct xen_hvm_altp2m_op *altp2m_op;
>> > +    } nat;
>> > +
>> > +    if ( !hvm_altp2m_supported() )
>> > +        return -EOPNOTSUPP;
>> > +
>> > +    if ( copy_from_gueWill change in the next patch iteration.
>> 
> st(&a, arg, 1) )

This isn't the first time I see such a misplaced comment. Once
again I can't make sense of it when being placed this way.

Jan


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