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:36, Razvan Cojocaru wrote:
> On 09/06/2016 02:20 PM, 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...
> Yes, but in that case the code would actually have been tested, because
> the common code runs (or should run) in the same manner for both ARM and
> x86. So x86 testing would have implicitly tested the common code.
That's quite a big assumption. I have seen common code patch touching 
memory subsystem breaking ARM because the way memory is handled is 
different.
> For our case, it is unfortunately impossible to implement this in the
> common part of the code. The main reason for this is that it's important
> to avoid a TLB flush until the very end, so we can't just iterate over
> the arrays and call the old p2m_set_mem_access() for each element, since
> p2m_set_mem_access() flushes the TLB on each call (the common code
> approach was my first thought going in).
>
> I certainly want everyone to be happy, but even assuming I "blindly"
> write some passable ARM code, it's always possible that a problem will
> pop up, at which point it will be rightly expected of me to remedy the
> situation as quickly as possible - but I may not even be able to
> investigate the issue.
There is a free ARM model [1] available that can boot Xen easily.

Regards,

[1] http://www.arm.com/products/tools/models/foundation-model.php

--
Julien Grall

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