Re: [Xen-devel] [PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()

[Julien Grall <julien.grall@xxxxxxx>]

On 06/09/16 12:34, George Dunlap wrote:
> On 06/09/16 12:20, Julien Grall wrote:
> > On 06/09/16 12:05, George Dunlap wrote:
> > > On 06/09/16 11:51, Julien Grall wrote:
> > > > On 06/09/16 11:45, Razvan Cojocaru wrote:
> > > > > Hello,
> > > > >
> > > > > > On 06/09/16 11:00, Razvan Cojocaru wrote:
> > > > > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > > > > > index b648a9d..e65a9b8 100644
> > > > > > > --- a/xen/arch/arm/p2m.c
> > > > > > > +++ b/xen/arch/arm/p2m.c
> > > > > > > @@ -1836,6 +1836,15 @@ long p2m_set_mem_access(struct domain *d,
> > > > > > > gfn_t
> > > > > > > gfn, uint32_t nr,
> > > > > > >      return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +long p2m_set_mem_access_multi(struct domain *d,
> > > > > > > +                              const XEN_GUEST_HANDLE(const_uint64)
> > > > > > > pfn_list,
> > > > > > > +                              const XEN_GUEST_HANDLE(const_uint8)
> > > > > > > access_list,
> > > > > > > +                              uint32_t nr, uint32_t start, uint32_t
> > > > > > > mask,
> > > > > > > +                              unsigned int altp2m_idx)
> > > > > > > +{
> > > > > > > +    return -ENOTSUP;
> > > > > > Why didn't you implement this function for ARM?
> > > > > Because unfortunately I don't have an ARM setup to test it on and I
> > > > > thought it would be unfair to publish the patch with untestable ARM
> > > > > support.
> > > > So what's the plan? Who will implement the ARM solution?
> > > >
> > > > I don't think there is a technical challenge to implement the ARM one.
> > > Are we going to require that all new functionality be implemented both
> > > on x86 and on ARM?  That seems like a bit of a lot to ask of someone
> > > who's not going to use it (and as Razvan points out, can't even test
> > > it).  The mem_access functionality is still fairly technical -- anyone
> > > who decides they want to use it and runs across the same problem Razvan
> > > did should have no trouble implementing this hypercall for ARM when they
> > > need it.
> > I am not saying we should every time implement the ARM version when the
> > x86 version is added and vice-versa.
> >
> > However, I don't think this is a lot to ask when an hypercall benefits
> > both architecture. Note that nobody would have complained that the code
> > was not tested on ARM if the hypercall was implemented in the common
> > code. Even if it could break the platform...
> So you really don't mind if Razvan submits a patch for code that he
> hasn't tested and isn't sure works?
To be fair, I don't mind as long as it is marked untested. I already saw 
this kind of patch on the mailing list, and I always tested the patch 
myself.
It is much better than waiting another couple of release to have a 
simple hypercall implemented.
> For someone coming along later wanting to use this functionality, I
> would think "It's not implemented yet, but here is the x86 version you
> can easily copy" would be better than "It's been implemented but never
> tested -- we're not sure if it actually works or not".
>
> Having common code not tested on ARM is slightly different.  At least in
> that case you know that the basic algorithm is correct because the code
> works on one platform.  The only things that might break are
> x86-specific assumptions.  Having the code not tested *at all*, other
> than compile-tested, seems a lot worse to me.
ARM is providing free model [1] which can boot Xen. It is not difficult 
to setup and does not take much time to test a patch series.
Regards,

--
Julien Grall

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