Re: [PATCH v13 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 24 November 2017 09:58
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; StefanoStabellini <sstabellini@kernel.org>; xen-
> devel@lists.xenproject.org; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; Tim
> (Xen.org) <tim@xen.org>
> Subject: RE: [PATCH v13 05/11] x86/mm: add HYPERVISOR_memory_op to
> acquire guest resources
> 
> >>> On 24.11.17 at 10:36, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 23 November 2017 16:42
> >> >>> On 30.10.17 at 18:48, <paul.durrant@citrix.com> wrote:
> >> > +    if ( !paging_mode_translate(currd) )
> >> > +    {
> >> > +        if ( copy_to_guest(xmar.frame_list, mfn_list, xmar.nr_frames) )
> >> > +            rc = -EFAULT;
> >> > +    }
> >> > +    else
> >> > +    {
> >> > +        xen_pfn_t gfn_list[ARRAY_SIZE(mfn_list)];
> >> > +        unsigned int i;
> >> > +
> >> > +        rc = -EFAULT;
> >> > +        if ( copy_from_guest(gfn_list, xmar.frame_list, xmar.nr_frames) )
> >> > +            goto out;
> >>
> >> This will result in requests with nr_frames being zero to fail with
> >> -EFAULT afaict. Let's please have such no-op requests succeed.
> >
> > Oh, I was labouring under the assumption that a copy_from_guest() with a
> > zero count would succeed... I should have checked.
> 
> I don't think it would fail. The problem is that you won't enter ...
> 
> >> > +        for ( i = 0; i < xmar.nr_frames; i++ )
> >> > +        {
> 
> ... the body of this loop then, and hence rc will remain at -EFAULT.
> 
> >> > +            rc = set_foreign_p2m_entry(currd, gfn_list[i],
> >> > +                                       _mfn(mfn_list[i]));
> >> > +            if ( rc )
> >> > +            {
> >> > +                /*
> >> > +                 * Make sure rc is -EIO for any iteration other than
> >> > +                 * the first.
> >> > +                 */
> >> > +                rc = (i != 0) ? -EIO : rc;
> >>
> >> Along the lines of what I've said above, "!=0" could be dropped
> >> here, too.
> >
> > The reason for the != 0 here is make the failure explicitly EIO for the case
> > where at least one iteration has successfully set_foreign_p2m_entry() but
> a
> > subsequent one has failed, which matches what is stated in the header.
> > (Specifically it means that, on ARM, the -EOPNOTSUPP from
> > set_foreign_p2m_entry() will get back to the caller).
> 
> I'm not asking for the condition to be removed, just for it to be
> simplified to
> 
>                 rc = i ? -EIO : rc;
> 

Ah, ok. That's fine then.

Thanks,

   Paul

> Jan


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