| Message ID | 1473156038-3758-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Razvan Cojocaru writes ("[PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()"):
> Currently it is only possible to set mem_access restrictions only for
> a contiguous range of GFNs (or, as a particular case, for a single GFN).
> This patch introduces a new libxc function taking an array of GFNs.
> The alternative would be to set each page in turn, using a userspace-HV
> roundtrip for each call, and triggering a TLB flush per page set.
>
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
I have no objection with my tools maintainer hat on. But I have a
question for you and/or the hypervisor maintainers:
Could this aim be achieved with a multicall ? (Can multicalls defer
the TLB flush?)
Ian.
>>> On 06.09.16 at 12:16, <ian.jackson@eu.citrix.com> wrote: > Razvan Cojocaru writes ("[PATCH V3] tools/libxc, xen/x86: Added > xc_set_mem_access_multi()"): >> Currently it is only possible to set mem_access restrictions only for >> a contiguous range of GFNs (or, as a particular case, for a single GFN). >> This patch introduces a new libxc function taking an array of GFNs. >> The alternative would be to set each page in turn, using a userspace-HV >> roundtrip for each call, and triggering a TLB flush per page set. >> >> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> >> Acked-by: Wei Liu <wei.liu2@citrix.com> > > I have no objection with my tools maintainer hat on. But I have a > question for you and/or the hypervisor maintainers: > > Could this aim be achieved with a multicall ? (Can multicalls defer > the TLB flush?) No, they can't, but it's not entirely excluded to make them do so. And then iirc there are no multicalls available to HVM (and hence PVHv2) guests right now. Jan
On 09/06/2016 01:16 PM, Ian Jackson wrote: > Razvan Cojocaru writes ("[PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()"): >> Currently it is only possible to set mem_access restrictions only for >> a contiguous range of GFNs (or, as a particular case, for a single GFN). >> This patch introduces a new libxc function taking an array of GFNs. >> The alternative would be to set each page in turn, using a userspace-HV >> roundtrip for each call, and triggering a TLB flush per page set. >> >> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> >> Acked-by: Wei Liu <wei.liu2@citrix.com> > > I have no objection with my tools maintainer hat on. But I have a > question for you and/or the hypervisor maintainers: > > Could this aim be achieved with a multicall ? (Can multicalls defer > the TLB flush?) I assume your question is: could we do multiple xc_set_mem_access() calls and then call something like xc_tlb_flush() at the end of it, instead of a singke xc_set_mem_access_multi() call? If that's the question, the answer (to the best of my knowledge) is that yes, that would be achievable (again, to the best of my knowledge that would still require a patch). But that would still be suboptimal performance-wise. Each xc_set_mem_access() call is a userspace <-> hypervisor round-trip. A typical introspection application follows the basic xen-access.c model, where you receive an event, do things in response to it, then reply (at which point, usually the VCPU is allowed to resume running). If, as part of processing the event, you'd like to set access restrictions on hundreds of pages, that would require hundreds of xc_set_mem_access() calls, and would definitely incur noticeable overhead. In short, I believe that there's a very strong case to be made for this approach vs. multiple calls. I hope I've understood and addressed your question. Thanks, Razvan
On Tue, Sep 06, 2016 at 01:27:21PM +0300, Razvan Cojocaru wrote: > On 09/06/2016 01:16 PM, Ian Jackson wrote: > > Razvan Cojocaru writes ("[PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()"): > >> Currently it is only possible to set mem_access restrictions only for > >> a contiguous range of GFNs (or, as a particular case, for a single GFN). > >> This patch introduces a new libxc function taking an array of GFNs. > >> The alternative would be to set each page in turn, using a userspace-HV > >> roundtrip for each call, and triggering a TLB flush per page set. > >> > >> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > >> Acked-by: Wei Liu <wei.liu2@citrix.com> > > > > I have no objection with my tools maintainer hat on. But I have a > > question for you and/or the hypervisor maintainers: > > > > Could this aim be achieved with a multicall ? (Can multicalls defer > > the TLB flush?) > > I assume your question is: could we do multiple xc_set_mem_access() > calls and then call something like xc_tlb_flush() at the end of it, > instead of a singke xc_set_mem_access_multi() call? > FWIW: I think you misunderstood, multicall is a Xen hypercall to batch multiple hypercalls into one. But as Jan said in the other reply, there is no multicall for HVM guests (yet) -- see hvm/hvm.c:hvm_hypercall_table. Wei.
Razvan Cojocaru writes ("Re: [PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()"):
> On 09/06/2016 01:16 PM, Ian Jackson wrote:
> > Could this aim be achieved with a multicall ? (Can multicalls defer
> > the TLB flush?)
>
> I assume your question is: could we do multiple xc_set_mem_access()
> calls and then call something like xc_tlb_flush() at the end of it,
> instead of a singke xc_set_mem_access_multi() call?
No, I was talking about the guest<->hypervisor interface.
Xen has a feature called "multicalls" which allows a single hypercall
to actually contain several hypercalls, so that a single pair of
guest<->hypervisor transitions can do more work.
Jan Beulich writes ("Re: [PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()"):
> On 06.09.16 at 12:16, <ian.jackson@eu.citrix.com> wrote:
> > Could this aim be achieved with a multicall ? (Can multicalls defer
> > the TLB flush?)
>
> No, they can't, but it's not entirely excluded to make them do so.
AIUI from reading the patch the TLB flush is implicit, so extra code
complexity (and fragility) would be needed to implement that.
> And then iirc there are no multicalls available to HVM (and hence
> PVHv2) guests right now.
This is another problem with my suggestion.
So, thanks for the reply. Carry on :-).
Ian.
On 09/06/2016 01:26 PM, Jan Beulich wrote: >>>> On 06.09.16 at 12:16, <ian.jackson@eu.citrix.com> wrote: >> Razvan Cojocaru writes ("[PATCH V3] tools/libxc, xen/x86: Added >> xc_set_mem_access_multi()"): >>> Currently it is only possible to set mem_access restrictions only for >>> a contiguous range of GFNs (or, as a particular case, for a single GFN). >>> This patch introduces a new libxc function taking an array of GFNs. >>> The alternative would be to set each page in turn, using a userspace-HV >>> roundtrip for each call, and triggering a TLB flush per page set. >>> >>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> >>> Acked-by: Wei Liu <wei.liu2@citrix.com> >> >> I have no objection with my tools maintainer hat on. But I have a >> question for you and/or the hypervisor maintainers: >> >> Could this aim be achieved with a multicall ? (Can multicalls defer >> the TLB flush?) > > No, they can't, but it's not entirely excluded to make them do so. > And then iirc there are no multicalls available to HVM (and hence > PVHv2) guests right now. Oh, right, Ian was talking about the Xen multicall mechanism, not simply chaining userspace xc_set_mem_access() calls. In any case, any single xc_set_mem_access() call does a TLB flush hypervisor-side, and AFAIK no flush hypercall is currently available. Other than that, I've never used the multicall mechanism so I'm not sure what else it would imply in this case. Thanks, Razvan
Hi Ravzan, 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? Regards,
On 06/09/16 11:16, Ian Jackson wrote: > Razvan Cojocaru writes ("[PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()"): >> Currently it is only possible to set mem_access restrictions only for >> a contiguous range of GFNs (or, as a particular case, for a single GFN). >> This patch introduces a new libxc function taking an array of GFNs. >> The alternative would be to set each page in turn, using a userspace-HV >> roundtrip for each call, and triggering a TLB flush per page set. >> >> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> >> Acked-by: Wei Liu <wei.liu2@citrix.com> > > I have no objection with my tools maintainer hat on. But I have a > question for you and/or the hypervisor maintainers: > > Could this aim be achieved with a multicall ? (Can multicalls defer > the TLB flush?) Making people go through the process of constructing a multicall consisting of a huge array of hypercalls which each set restrictions for a single gfn seems a bit tedious. We already have other hypercalls which take arrays of gfns, so even if it could be done in theory, I don't see a good reason why not to accept something like this. -George
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. Thanks, Razvan
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. Regards,
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. -George
On 09/06/2016 01:51 PM, 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? My assumption has been that whoever first needs this functionality and is working on an ARM project would implement it. Touching the ARM code only occured because it became necessary in order to compile. The patch subject mentions that these are mainly x86 changes ("xen/x86"). > I don't think there is a technical challenge to implement the ARM one. apply_p2m_changes(), which would require modification, doesn't look trivial, and I would unfortunately not be able to test the changes beyond making sure the code still compiles. Thanks, Razvan
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... Regards,
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? 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. Anyone else have an opinion? Tamas and company have already implemented a fair amount of the mem_access stuff for ARM -- it seems likely that this is the sort of thing they would probably end up implementing at some point once they get around to it. -George
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. 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. Thanks, Razvan
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
George Dunlap writes ("Re: [PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()"):
> So you really don't mind if Razvan submits a patch for code that he
> hasn't tested and isn't sure works?
I don't think that's a very good idea. (Are there possible security
implications? It's not entirely clear to me...)
Under the circumstances I don't think it's reasonable to expect Razvan
to write the patch, unless some ARM maintainer is willing to test it.
Ian.
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,
On 06/09/16 12:51, Ian Jackson wrote: > George Dunlap writes ("Re: [PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()"): >> So you really don't mind if Razvan submits a patch for code that he >> hasn't tested and isn't sure works? > > I don't think that's a very good idea. (Are there possible security > implications? It's not entirely clear to me...) > > Under the circumstances I don't think it's reasonable to expect Razvan > to write the patch, unless some ARM maintainer is willing to test it. That was implied from my side. It has been done recently on other various patches which require modifications in the ARM code. Regards,
On 09/06/2016 02:53 PM, Julien Grall wrote: > > > 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. I'm sure it's very easy to use for someone already familiar with the tools. I did manage to download the Foundation Model and run the test application that comes with it, but still need some time to figure out how to run anything more complicated. But before that, since Tamas has expressed interest in this feature and is already working with ARM projects, maybe he'd be interested in a follow-up patch implementing this for ARM? IMHO this would be the best case since he's got both the hardware and the ARM tools to do the actual testing of the patch. Tamas, what do you think? Thanks, Razvan
>>> On 06.09.16 at 12:00, <rcojocaru@bitdefender.com> wrote: > --- 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; > +} If this indeed fixes the build problem on ARM, then I'd like to have an explanation (by you or the ARM maintainers) where ENOTSUP gets defined. I can't find any single instance of this throughout the tree, and headers outside of the tree aren't supposed to get included in the hypervisor build. > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -28,6 +28,7 @@ > #include <xen/event.h> > #include <public/vm_event.h> > #include <asm/domain.h> > +#include <xen/guest_access.h> /* copy_from_guest() */ > #include <asm/page.h> I thought we've been through this before: Why does this xen/ include get added in the middle of asm/ ones? > +static int set_mem_access(struct domain *d, struct p2m_domain *p2m, > + struct p2m_domain *ap2m, p2m_access_t a, > + unsigned long gfn_l) > { > - struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL; > - p2m_access_t a, _a; > - p2m_type_t t; > - mfn_t mfn; > - unsigned long gfn_l; > - long rc = 0; > + int rc = 0; > > + if ( ap2m ) > + { > + rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, _gfn(gfn_l)); With this it becomes pretty clear that, following our general direction, set_mem_access()'s last parameter should itself be gfn_t. > +static bool_t p2m_xenmem_access_to_p2m_access(struct p2m_domain *p2m, The p2m_ prefix is kind of redundant here as long as the function is static. Also plain bool please in new code, and ... > @@ -1823,6 +1839,34 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, > #undef ACCESS > }; > > + switch ( xaccess ) > + { > + case 0 ... ARRAY_SIZE(memaccess) - 1: > + *paccess = memaccess[xaccess]; > + break; > + case XENMEM_access_default: > + *paccess = p2m->default_access; > + break; > + default: > + return 0; ... false and ... > + } > + > + return 1; ... true respectively then. > @@ -520,6 +534,9 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat) > rc = -EFAULT; > break; > > + case XENMEM_access_op: > + break; There's a group of several other XENMEM_* which also require no action (right above the XENMEM_get_vnumainfo handling). Please simply add this new case there. Jan
Razvan Cojocaru writes ("Re: [PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()"):
> But before that, since Tamas has expressed interest in this feature and
> is already working with ARM projects, maybe he'd be interested in a
> follow-up patch implementing this for ARM? IMHO this would be the best
> case since he's got both the hardware and the ARM tools to do the actual
> testing of the patch. Tamas, what do you think?
Julien has volunteered to test a patch you provide. How hard would it
be for you to do what seems like the obvious thing and hand it over to
Julien ?
Ian.
On 09/06/2016 05:07 PM, Jan Beulich wrote: >>>> On 06.09.16 at 12:00, <rcojocaru@bitdefender.com> wrote: >> --- 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; >> +} > > If this indeed fixes the build problem on ARM, then I'd like to have an > explanation (by you or the ARM maintainers) where ENOTSUP gets > defined. I can't find any single instance of this throughout the tree, > and headers outside of the tree aren't supposed to get included in > the hypervisor build. It's defined in errno.h, which some in-tree files do include, but as stated I don't, at the moment, have access to an ARM setup, so it is theoretically possible that the code still doesn't compile on ARM. I thought it would, since it's trivial. I won't send the next version until it becomes possible for me to at least compile it on ARM. >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -28,6 +28,7 @@ >> #include <xen/event.h> >> #include <public/vm_event.h> >> #include <asm/domain.h> >> +#include <xen/guest_access.h> /* copy_from_guest() */ >> #include <asm/page.h> > > I thought we've been through this before: Why does this xen/ > include get added in the middle of asm/ ones? I'll fix this. I initially #included <asm/guest_access.h>, then fixed it to be <xen/guest_access.h> but forgot to reorder the includes. >> +static int set_mem_access(struct domain *d, struct p2m_domain *p2m, >> + struct p2m_domain *ap2m, p2m_access_t a, >> + unsigned long gfn_l) >> { >> - struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL; >> - p2m_access_t a, _a; >> - p2m_type_t t; >> - mfn_t mfn; >> - unsigned long gfn_l; >> - long rc = 0; >> + int rc = 0; >> >> + if ( ap2m ) >> + { >> + rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, _gfn(gfn_l)); > > With this it becomes pretty clear that, following our general direction, > set_mem_access()'s last parameter should itself be gfn_t. I'll change it. >> +static bool_t p2m_xenmem_access_to_p2m_access(struct p2m_domain *p2m, > > The p2m_ prefix is kind of redundant here as long as the function is > static. > > Also plain bool please in new code, and ... > >> @@ -1823,6 +1839,34 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, >> #undef ACCESS >> }; >> >> + switch ( xaccess ) >> + { >> + case 0 ... ARRAY_SIZE(memaccess) - 1: >> + *paccess = memaccess[xaccess]; >> + break; >> + case XENMEM_access_default: >> + *paccess = p2m->default_access; >> + break; >> + default: >> + return 0; > > ... false and ... > >> + } >> + >> + return 1; > > ... true respectively then. I'll switch to bool. >> @@ -520,6 +534,9 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat) >> rc = -EFAULT; >> break; >> >> + case XENMEM_access_op: >> + break; > > There's a group of several other XENMEM_* which also require no > action (right above the XENMEM_get_vnumainfo handling). Please > simply add this new case there. I'll move it there. Thanks, Razvan
On 09/06/2016 05:08 PM, Ian Jackson wrote: > Razvan Cojocaru writes ("Re: [PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()"): >> But before that, since Tamas has expressed interest in this feature and >> is already working with ARM projects, maybe he'd be interested in a >> follow-up patch implementing this for ARM? IMHO this would be the best >> case since he's got both the hardware and the ARM tools to do the actual >> testing of the patch. Tamas, what do you think? > > Julien has volunteered to test a patch you provide. How hard would it > be for you to do what seems like the obvious thing and hand it over to > Julien ? I was under the impression that still enough voices called for this to be done at a later time / by interested parties working on ARM, both if which fit Tamas' profile. To answer your question, it wouldn't be very hard, so I guess I'm taking that route. Thanks, Razvan
>>> On 06.09.16 at 16:21, <rcojocaru@bitdefender.com> wrote: > On 09/06/2016 05:07 PM, Jan Beulich wrote: >>>>> On 06.09.16 at 12:00, <rcojocaru@bitdefender.com> wrote: >>> --- 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; >>> +} >> >> If this indeed fixes the build problem on ARM, then I'd like to have an >> explanation (by you or the ARM maintainers) where ENOTSUP gets >> defined. I can't find any single instance of this throughout the tree, >> and headers outside of the tree aren't supposed to get included in >> the hypervisor build. > > It's defined in errno.h, which some in-tree files do include, but as > stated I don't, at the moment, have access to an ARM setup, so it is > theoretically possible that the code still doesn't compile on ARM. I > thought it would, since it's trivial. I can't find any inclusion of plain errno.h (apart in tools built to aid the building process). There are xen/errno.h and, public/errno.h, but those - afaics - don't define ENOTSUP. > I won't send the next version until it becomes possible for me to at > least compile it on ARM. Indeed, compile testing is the minimal requirement. Jan
On 09/06/2016 05:28 PM, Jan Beulich wrote: >>>> On 06.09.16 at 16:21, <rcojocaru@bitdefender.com> wrote: >> On 09/06/2016 05:07 PM, Jan Beulich wrote: >>>>>> On 06.09.16 at 12:00, <rcojocaru@bitdefender.com> wrote: >>>> --- 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; >>>> +} >>> >>> If this indeed fixes the build problem on ARM, then I'd like to have an >>> explanation (by you or the ARM maintainers) where ENOTSUP gets >>> defined. I can't find any single instance of this throughout the tree, >>> and headers outside of the tree aren't supposed to get included in >>> the hypervisor build. >> >> It's defined in errno.h, which some in-tree files do include, but as >> stated I don't, at the moment, have access to an ARM setup, so it is >> theoretically possible that the code still doesn't compile on ARM. I >> thought it would, since it's trivial. > > I can't find any inclusion of plain errno.h (apart in tools built to aid > the building process). There are xen/errno.h and, public/errno.h, > but those - afaics - don't define ENOTSUP. You're right. And yet, cross-compiling Xen for ARM64 as recommended here: https://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions/CrossCompiling#64-bit_crossbuild works without a hitch. Maybe the some standard libc header we include brings errno.h in? I'm not sure what to do, should I switch to an error number from the in-tree errno.h (if so, which one would be the most appropriate)? Thanks, Razvan
On 09/07/2016 11:36 AM, Razvan Cojocaru wrote: > On 09/06/2016 05:28 PM, Jan Beulich wrote: >>>>> On 06.09.16 at 16:21, <rcojocaru@bitdefender.com> wrote: >>> On 09/06/2016 05:07 PM, Jan Beulich wrote: >>>>>>> On 06.09.16 at 12:00, <rcojocaru@bitdefender.com> wrote: >>>>> --- 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; >>>>> +} >>>> >>>> If this indeed fixes the build problem on ARM, then I'd like to have an >>>> explanation (by you or the ARM maintainers) where ENOTSUP gets >>>> defined. I can't find any single instance of this throughout the tree, >>>> and headers outside of the tree aren't supposed to get included in >>>> the hypervisor build. >>> >>> It's defined in errno.h, which some in-tree files do include, but as >>> stated I don't, at the moment, have access to an ARM setup, so it is >>> theoretically possible that the code still doesn't compile on ARM. I >>> thought it would, since it's trivial. >> >> I can't find any inclusion of plain errno.h (apart in tools built to aid >> the building process). There are xen/errno.h and, public/errno.h, >> but those - afaics - don't define ENOTSUP. > > You're right. And yet, cross-compiling Xen for ARM64 as recommended here: > > https://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions/CrossCompiling#64-bit_crossbuild > > works without a hitch. > > Maybe the some standard libc header we include brings errno.h in? I'm > not sure what to do, should I switch to an error number from the in-tree > errno.h (if so, which one would be the most appropriate)? Nevermind, make dist-tools does not build xen.gz. Sorry for the noise. Thanks, Razvan
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 560ce7b..5e685a6 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -2126,6 +2126,15 @@ int xc_set_mem_access(xc_interface *xch, domid_t domain_id, uint32_t nr); /* + * Set an array of pages to their respective access in the access array. + * The nr parameter specifies the size of the pages and access arrays. + * The same allowed access types as for xc_set_mem_access() apply. + */ +int xc_set_mem_access_multi(xc_interface *xch, domid_t domain_id, + uint8_t *access, uint64_t *pages, + uint32_t nr); + +/* * Gets the mem access for the given page (returned in access on success) */ int xc_get_mem_access(xc_interface *xch, domid_t domain_id, diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c index eee088c..9536635 100644 --- a/tools/libxc/xc_mem_access.c +++ b/tools/libxc/xc_mem_access.c @@ -41,6 +41,44 @@ int xc_set_mem_access(xc_interface *xch, return do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao)); } +int xc_set_mem_access_multi(xc_interface *xch, + domid_t domain_id, + uint8_t *access, + uint64_t *pages, + uint32_t nr) +{ + DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN); + DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t), + XC_HYPERCALL_BUFFER_BOUNCE_IN); + int rc; + + xen_mem_access_op_t mao = + { + .op = XENMEM_access_op_set_access_multi, + .domid = domain_id, + .access = XENMEM_access_default + 1, /* Invalid value */ + .pfn = ~0UL, /* Invalid GFN */ + .nr = nr, + }; + + if ( xc_hypercall_bounce_pre(xch, pages) || + xc_hypercall_bounce_pre(xch, access) ) + { + PERROR("Could not bounce memory for XENMEM_access_op_set_access_multi"); + return -1; + } + + set_xen_guest_handle(mao.pfn_list, pages); + set_xen_guest_handle(mao.access_list, access); + + rc = do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao)); + + xc_hypercall_bounce_post(xch, access); + xc_hypercall_bounce_post(xch, pages); + + return rc; +} + int xc_get_mem_access(xc_interface *xch, domid_t domain_id, uint64_t pfn, 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; +} + int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access) { diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 27f9d26..980661a 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -28,6 +28,7 @@ #include <xen/event.h> #include <public/vm_event.h> #include <asm/domain.h> +#include <xen/guest_access.h> /* copy_from_guest() */ #include <asm/page.h> #include <asm/paging.h> #include <asm/p2m.h> @@ -1793,21 +1794,36 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, (current->domain != d)); } -/* - * Set access type for a region of gfns. - * If gfn == INVALID_GFN, sets the default access type. - */ -long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, - uint32_t start, uint32_t mask, xenmem_access_t access, - unsigned int altp2m_idx) +static int set_mem_access(struct domain *d, struct p2m_domain *p2m, + struct p2m_domain *ap2m, p2m_access_t a, + unsigned long gfn_l) { - struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL; - p2m_access_t a, _a; - p2m_type_t t; - mfn_t mfn; - unsigned long gfn_l; - long rc = 0; + int rc = 0; + if ( ap2m ) + { + rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, _gfn(gfn_l)); + /* If the corresponding mfn is invalid we will want to just skip it */ + if ( rc == -ESRCH ) + rc = 0; + } + else + { + mfn_t mfn; + p2m_access_t _a; + p2m_type_t t; + + mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL); + rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1); + } + + return rc; +} + +static bool_t p2m_xenmem_access_to_p2m_access(struct p2m_domain *p2m, + xenmem_access_t xaccess, + p2m_access_t *paccess) +{ static const p2m_access_t memaccess[] = { #define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac ACCESS(n), @@ -1823,6 +1839,34 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, #undef ACCESS }; + switch ( xaccess ) + { + case 0 ... ARRAY_SIZE(memaccess) - 1: + *paccess = memaccess[xaccess]; + break; + case XENMEM_access_default: + *paccess = p2m->default_access; + break; + default: + return 0; + } + + return 1; +} + +/* + * Set access type for a region of gfns. + * If gfn == INVALID_GFN, sets the default access type. + */ +long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, + uint32_t start, uint32_t mask, xenmem_access_t access, + unsigned int altp2m_idx) +{ + struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL; + p2m_access_t a; + unsigned long gfn_l; + long rc = 0; + /* altp2m view 0 is treated as the hostp2m */ if ( altp2m_idx ) { @@ -1833,17 +1877,8 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, ap2m = d->arch.altp2m_p2m[altp2m_idx]; } - switch ( access ) - { - case 0 ... ARRAY_SIZE(memaccess) - 1: - a = memaccess[access]; - break; - case XENMEM_access_default: - a = p2m->default_access; - break; - default: + if ( !p2m_xenmem_access_to_p2m_access(p2m, access, &a) ) return -EINVAL; - } /* If request to set default access. */ if ( gfn_eq(gfn, INVALID_GFN) ) @@ -1858,21 +1893,69 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, for ( gfn_l = gfn_x(gfn) + start; nr > start; ++gfn_l ) { - if ( ap2m ) + rc = set_mem_access(d, p2m, ap2m, a, gfn_l); + + if ( rc ) + break; + + /* Check for continuation if it's not the last iteration. */ + if ( nr > ++start && !(start & mask) && hypercall_preempt_check() ) { - rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, _gfn(gfn_l)); - /* If the corresponding mfn is invalid we will just skip it */ - if ( rc && rc != -ESRCH ) - break; + rc = start; + break; } - else + } + + if ( ap2m ) + p2m_unlock(ap2m); + p2m_unlock(p2m); + + return rc; +} + +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) +{ + struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL; + long rc = 0; + + /* altp2m view 0 is treated as the hostp2m */ + if ( altp2m_idx ) + { + if ( altp2m_idx >= MAX_ALTP2M || + d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) + return -EINVAL; + + ap2m = d->arch.altp2m_p2m[altp2m_idx]; + } + + p2m_lock(p2m); + if ( ap2m ) + p2m_lock(ap2m); + + while ( start < nr ) + { + p2m_access_t a; + uint8_t access; + uint64_t gfn_l; + + copy_from_guest_offset(&gfn_l, pfn_list, start, 1); + copy_from_guest_offset(&access, access_list, start, 1); + + if ( !p2m_xenmem_access_to_p2m_access(p2m, access, &a) ) { - mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL); - rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1); - if ( rc ) - break; + rc = -EINVAL; + break; } + rc = set_mem_access(d, p2m, ap2m, a, gfn_l); + + if ( rc ) + break; + /* Check for continuation if it's not the last iteration. */ if ( nr > ++start && !(start & mask) && hypercall_preempt_check() ) { diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c index 579040e..1b45b31 100644 --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -15,7 +15,6 @@ CHECK_TYPE(domid); #undef compat_domid_t #undef xen_domid_t -CHECK_mem_access_op; CHECK_vmemrange; #ifdef CONFIG_HAS_PASSTHROUGH @@ -71,6 +70,7 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat) struct xen_add_to_physmap_batch *atpb; struct xen_remove_from_physmap *xrfp; struct xen_vnuma_topology_info *vnuma; + struct xen_mem_access_op *mao; } nat; union { struct compat_memory_reservation rsrv; @@ -78,6 +78,7 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat) struct compat_add_to_physmap atp; struct compat_add_to_physmap_batch atpb; struct compat_vnuma_topology_info vnuma; + struct compat_mem_access_op mao; } cmp; set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE); @@ -321,9 +322,22 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat) } case XENMEM_access_op: - return mem_access_memop(cmd, - guest_handle_cast(compat, - xen_mem_access_op_t)); + { + if ( copy_from_guest(&cmp.mao, compat, 1) ) + return -EFAULT; + +#define XLAT_mem_access_op_HNDL_pfn_list(_d_, _s_) \ + guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list) +#define XLAT_mem_access_op_HNDL_access_list(_d_, _s_) \ + guest_from_compat_handle((_d_)->access_list, (_s_)->access_list) + + XLAT_mem_access_op(nat.mao, &cmp.mao); + +#undef XLAT_mem_access_op_HNDL_pfn_list +#undef XLAT_mem_access_op_HNDL_access_list + + break; + } case XENMEM_get_vnumainfo: { @@ -520,6 +534,9 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat) rc = -EFAULT; break; + case XENMEM_access_op: + break; + default: domain_crash(current->domain); split = 0; diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c index 82f4bad..565a320 100644 --- a/xen/common/mem_access.c +++ b/xen/common/mem_access.c @@ -76,6 +76,17 @@ int mem_access_memop(unsigned long cmd, } break; + case XENMEM_access_op_set_access_multi: + rc = p2m_set_mem_access_multi(d, mao.pfn_list, mao.access_list, mao.nr, + start_iter, MEMOP_CMD_MASK, 0); + if ( rc > 0 ) + { + ASSERT(!(rc & MEMOP_CMD_MASK)); + rc = hypercall_create_continuation(__HYPERVISOR_memory_op, "lh", + XENMEM_access_op | rc, arg); + } + break; + case XENMEM_access_op_get_access: { xenmem_access_t access; diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h index 3badfb9..a5547a9 100644 --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -410,6 +410,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_paging_op_t); * #define XENMEM_access_op_enable_emulate 2 * #define XENMEM_access_op_disable_emulate 3 */ +#define XENMEM_access_op_set_access_multi 4 typedef enum { XENMEM_access_n, @@ -442,7 +443,8 @@ struct xen_mem_access_op { uint8_t access; domid_t domid; /* - * Number of pages for set op + * Number of pages for set op (or size of pfn_list for + * XENMEM_access_op_set_access_multi) * Ignored on setting default access and other ops */ uint32_t nr; @@ -452,6 +454,16 @@ struct xen_mem_access_op { * ~0ull is used to set and get the default access for pages */ uint64_aligned_t pfn; + /* + * List of pfns to set access for + * Used only with XENMEM_access_op_set_access_multi + */ + XEN_GUEST_HANDLE(const_uint64) pfn_list; + /* + * Corresponding list of access settings for pfn_list + * Used only with XENMEM_access_op_set_access_multi + */ + XEN_GUEST_HANDLE(const_uint8) access_list; }; typedef struct xen_mem_access_op xen_mem_access_op_t; DEFINE_XEN_GUEST_HANDLE(xen_mem_access_op_t); diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h index b4f9077..3be1e91 100644 --- a/xen/include/xen/p2m-common.h +++ b/xen/include/xen/p2m-common.h @@ -53,6 +53,12 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, uint32_t start, uint32_t mask, xenmem_access_t access, unsigned int altp2m_idx); +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); + /* * Get access type for a gfn. * If gfn == INVALID_GFN, gets the default access type. diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst index 801a1c1..bdf1d05 100644 --- a/xen/include/xlat.lst +++ b/xen/include/xlat.lst @@ -68,7 +68,7 @@ ! memory_exchange memory.h ! memory_map memory.h ! memory_reservation memory.h -? mem_access_op memory.h +! mem_access_op memory.h ! pod_target memory.h ! remove_from_physmap memory.h ! reserved_device_memory_map memory.h