Re: [Xen-devel] [PATCH V2 1/2] x86/altp2m: Add hypercall to set a range of sve bits

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Nov 12, 2019 at 4:54 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
> > @@ -4681,7 +4682,7 @@ static int do_altp2m_op(
> >          break;
> >
> >      case HVMOP_altp2m_set_suppress_ve:
> > -        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
> > +        if ( a.u.suppress_ve.pad1 )
>
> Just because the field changes its name doesn't mean you can
> drop the check. You even add a new field not used (yet) by
> this sub-function, which then also would need checking here.
>
> > @@ -4693,8 +4694,23 @@ static int do_altp2m_op(
> >          }
> >          break;
> >
> > +    case HVMOP_altp2m_set_suppress_ve_multi:
> > +        if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr )
>
> A count of zero typically is taken as a no-op, not an error.
>
> > +            rc = -EINVAL;
> > +        else
> > +        {
> > +            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve);
> > +
> > +            if ( rc == -ERESTART )
> > +                if ( __copy_field_to_guest(guest_handle_cast(arg,
> > +                                           xen_hvm_altp2m_op_t),
> > +                                           &a, u.suppress_ve.opaque) )
> > +                    rc = -EFAULT;
>
> If the operation is best effort, _some_ indication of failure should
> still be handed back to the caller. Whether that's through the opaque
> field or by some other means is secondary. If not via that field
> (which would make the outer of the two if()-s disappear), please fold
> the if()-s.

At least for mem_sharing_range_op we also do a best-effort and don't
return an error for pages where it wasn't possible to share. So I
don't think it's absolutely necessary to do that, especially if the
caller can't do anything about those errors anyway.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel