Jean Guyader
2011-Nov-10 11:35 UTC
[Xen-devel] [PATCH 0/7] IOMMU, vtd and iotlb flush rework (v6)
In one of my previous email I detailed a bug I was seeing when passing
through a Intel GPU on a guest that has more that 4G or RAM.
Allen suggested that I go for the Plan B but after a discussion with Tim
we agreed that Plan B was way to disruptive in term of code change.
This patch series implements Plan A.
http://xen.1045712.n5.nabble.com/VTD-Intel-iommu-IOTLB-flush-really-slow-td4952866.html
Changes between v5 and v6:
- Rework the patch queue to make it more readable.
- Modify xatp in place in xenmem_add_to_physmap
- Only check for preemption if we are not at the last iteration
- Copy xatp guest handler back to the guest only in case of continuation
- Add continuation only when dealing with the new xenmem space
(XENMAPSPACE_gmfn_range).
Changes between v4 and v5:
- Fix hypercall continuation for add_to_physmap in compat mode.
Changes between v3 and v4:
- Move the loop for gmfn_range from arch_memory_op to
xenmem_add_to_physmap.
- Add a comment to comment to explain the purpose of
iommu_dont_flush_iotlb.
Changes between v2 and v3:
- Check for the presence iotlb_flush_all callback before calling it.
Changes between v1 and v2:
- Move size in struct xen_add_to_physmap in padding between .domid and
.space.
- Store iommu_dont_flush per cpu
- Change the code in hvmloader to relocate by batch of 64K, .size is now
16 bits.
Jean Guyader (7):
vtd: Refactor iotlb flush code
iommu: Introduce iommu_flush and iommu_flush_all.
add_to_physmap: Move the code for XENMEM_add_to_physmap
mm: xenmem_add_to_physmap now takes a pointer on xatp
mm: New XENMEM space, XENMAPSPACE_gmfn_range
hvmloader: Change memory relocation loop when overlap with PCI hole
Introduce per cpu flag (iommu_dont_flush_iotlb) to avoid unnecessary
iotlb flush
tools/firmware/hvmloader/pci.c | 20 ++-
xen/arch/x86/mm.c | 232 ++++++++++++++++++++++-------------
xen/arch/x86/x86_64/compat/mm.c | 12 ++
xen/drivers/passthrough/iommu.c | 25 ++++
xen/drivers/passthrough/vtd/iommu.c | 100 +++++++++-------
xen/include/public/memory.h | 4 +
xen/include/xen/iommu.h | 17 +++
7 files changed, 278 insertions(+), 132 deletions(-)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Factorize the iotlb flush code from map_page and unmap_page into it''s own function. Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com> Acked-by: Allen M Kay <allen.m.kay@intel.com> --- xen/drivers/passthrough/vtd/iommu.c | 86 +++++++++++++++++----------------- 1 files changed, 43 insertions(+), 43 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-10 11:35 UTC
[Xen-devel] [PATCH 2/7] iommu: Introduce iommu_flush and iommu_flush_all.
Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com> Acked-by: Allen M Kay <allen.m.kay@intel.com> --- xen/drivers/passthrough/iommu.c | 20 ++++++++++++++++++++ xen/drivers/passthrough/vtd/iommu.c | 12 ++++++++++++ xen/include/xen/iommu.h | 5 +++++ 3 files changed, 37 insertions(+), 0 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-10 11:35 UTC
[Xen-devel] [PATCH 3/7] add_to_physmap: Move the code for XENMEM_add_to_physmap
Move the code for the XENMEM_add_to_physmap case into it''s own function (xenmem_add_to_physmap). Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com> --- xen/arch/x86/mm.c | 189 ++++++++++++++++++++++++++++------------------------- 1 files changed, 100 insertions(+), 89 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-10 11:35 UTC
[Xen-devel] [PATCH 4/7] mm: xenmem_add_to_physmap now takes a pointer on xatp
Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com> --- xen/arch/x86/mm.c | 40 ++++++++++++++++++++-------------------- 1 files changed, 20 insertions(+), 20 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-10 11:35 UTC
[Xen-devel] [PATCH 5/7] mm: New XENMEM space, XENMAPSPACE_gmfn_range
XENMAPSPACE_gmfn_range is like XENMAPSPACE_gmfn but it runs on a range of pages. The size of the range is defined in a new field. This new field .size is located in the 16 bits padding between .domid and .space in struct xen_add_to_physmap to stay compatible with older versions. Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com> --- xen/arch/x86/mm.c | 47 +++++++++++++++++++++++++++++++++++++- xen/arch/x86/x86_64/compat/mm.c | 12 ++++++++++ xen/include/public/memory.h | 4 +++ 3 files changed, 61 insertions(+), 2 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-10 11:35 UTC
[Xen-devel] [PATCH 6/7] hvmloader: Change memory relocation loop when overlap with PCI hole
Change the way we relocate the memory page if they overlap with pci hole. Use new map space (XENMAPSPACE_gmfn_range) to move the loop into xen. This code usually get triggered when a device is pass through to a guest and the PCI hole has to be extended to have enough room to map the device BARs. The PCI hole will starts lower and it might overlap with some RAM that has been alocated for the guest. That usually happen if the guest has more than 4G of RAM. We have to relocate those pages in high mem otherwise they won''t be accessible. Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com> --- tools/firmware/hvmloader/pci.c | 20 ++++++++++++++------ 1 files changed, 14 insertions(+), 6 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-10 11:35 UTC
[Xen-devel] [PATCH 7/7] Introduce per cpu flag (iommu_dont_flush_iotlb) to avoid unnecessary iotlb flush
Add cpu flag that will be checked by the iommu low level code to skip iotlb flushes. iommu_iotlb_flush shall be called explicitly. Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com> --- xen/arch/x86/mm.c | 12 ++++++++++++ xen/drivers/passthrough/iommu.c | 5 +++++ xen/drivers/passthrough/vtd/iommu.c | 6 ++++-- xen/include/xen/iommu.h | 12 ++++++++++++ 4 files changed, 33 insertions(+), 2 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-10 12:18 UTC
[Xen-devel] Re: [PATCH 4/7] mm: xenmem_add_to_physmap now takes a pointer on xatp
>>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@eu.citrix.com> wrote:Any reason why this can''t be done right away in patch 3?> Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com> > --- > xen/arch/x86/mm.c | 40 ++++++++++++++++++++-------------------- > 1 files changed, 20 insertions(+), 20 deletions(-)_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-10 12:47 UTC
[Xen-devel] Re: [PATCH 5/7] mm: New XENMEM space, XENMAPSPACE_gmfn_range
>>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@eu.citrix.com> wrote: >@@ -4716,6 +4748,17 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg) > } > > rc = xenmem_add_to_physmap(d, &xatp); >+ if ( rc == -EAGAIN )if ( rc )>+ { >+ if ( copy_to_guest(arg, &xatp, 1) ) >+ { >+ rcu_unlock_domain(d); >+ return -EFAULT; >+ }} if ( rc == -EAGAIN ) (with some room for further simplification). Without that (or the minimal alternative of copying back just .size or yet some other mechanism), as pointed out before, the caller won''t have a way to know how far into the batch things succeeded.>+ >+ rc = hypercall_create_continuation( >+ __HYPERVISOR_memory_op, "ih", op, arg); >+ } > > rcu_unlock_domain(d);Also, the whole block above can be move past this rcu_unlock_domain(), eliminating the need to do it separately in the above error path(s).> >--- a/xen/arch/x86/x86_64/compat/mm.c >+++ b/xen/arch/x86/x86_64/compat/mm.c >@@ -63,6 +63,18 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg) > > XLAT_add_to_physmap(nat, &cmp); > rc = arch_memory_op(op, guest_handle_from_ptr(nat, void)); >+ if ( rc < 0 ) >+ break; >+With the way the code below is currently this is superfluous. But just as above you will need to provide some indication to the caller *where* the failure occurred.>+ if ( rc == __HYPERVISOR_memory_op ) >+ { >+ hypercall_xlat_continuation(NULL, 0x2, nat, arg); >+ >+ XLAT_add_to_physmap(&cmp, nat); >+ >+ if ( copy_to_guest(arg, &cmp, 1) ) >+ return -EFAULT;I realize that this is the same way in the code handling XENMEM_[gs]et_pod_target, but unfortunately that''s wrong (that''s why I''m copying you, George): Once a continuation was set up, you mustn''t change the return value anymore, since the continuation was established by adjusting the guest''s rIP. As for other memory ops, the continuation can be encoded in "op" (see xen/common/memory.c and xen/common/compat/memory.c). However, while suitable here I don''t think that''s usable for the PoD variant. The alternative is to cancel the continuation (would require quite a bit of new code I think) or to adjust the low level hypercall handler code (at least the compat mode one) to special case rAX values in the negative errno range, leaving rAX unchanged instead of returning -ENOSYS. Keir? Jan>+ } > > break; > }_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-10 13:46 UTC
[Xen-devel] Re: [PATCH 4/7] mm: xenmem_add_to_physmap now takes a pointer on xatp
On 10/11 12:18, Jan Beulich wrote:> >>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > > Any reason why this can''t be done right away in patch 3? >I think that moving and changing code at the same time make it headers to read. The preview patch didn''t change anything any code at all, just moved some code around. Jean _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-10 13:49 UTC
[Xen-devel] Re: [PATCH 4/7] mm: xenmem_add_to_physmap now takes a pointer on xatp
On 10/11 01:46, Jean Guyader wrote:> > On 10/11 12:18, Jan Beulich wrote: > > >>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > > > > Any reason why this can''t be done right away in patch 3? > > > > I think that moving and changing code at the same time make > it headers to read.harder...> > The preview patch didn''t change anything any code at all, > just moved some code around. >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-10 15:44 UTC
[Xen-devel] Re: [PATCH 5/7] mm: New XENMEM space, XENMAPSPACE_gmfn_range
On 10/11 12:47, Jan Beulich wrote:> >>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > >@@ -4716,6 +4748,17 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg) > > } > > > > rc = xenmem_add_to_physmap(d, &xatp); > >+ if ( rc == -EAGAIN ) > > if ( rc ) > > >+ { > >+ if ( copy_to_guest(arg, &xatp, 1) ) > >+ { > >+ rcu_unlock_domain(d); > >+ return -EFAULT; > >+ } > > } > if ( rc == -EAGAIN ) > > (with some room for further simplification). Without that (or the minimal > alternative of copying back just .size or yet some other mechanism), as > pointed out before, the caller won''t have a way to know how far into > the batch things succeeded. > > >+ > >+ rc = hypercall_create_continuation( > >+ __HYPERVISOR_memory_op, "ih", op, arg); > >+ } > > > > rcu_unlock_domain(d); > > Also, the whole block above can be move past this rcu_unlock_domain(), > eliminating the need to do it separately in the above error path(s). > > > > >--- a/xen/arch/x86/x86_64/compat/mm.c > >+++ b/xen/arch/x86/x86_64/compat/mm.c > >@@ -63,6 +63,18 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg) > > > > XLAT_add_to_physmap(nat, &cmp); > > rc = arch_memory_op(op, guest_handle_from_ptr(nat, void)); > >+ if ( rc < 0 ) > >+ break; > >+ > > With the way the code below is currently this is superfluous. But just > as above you will need to provide some indication to the caller > *where* the failure occurred. > > >+ if ( rc == __HYPERVISOR_memory_op ) > >+ { > >+ hypercall_xlat_continuation(NULL, 0x2, nat, arg); > >+ > >+ XLAT_add_to_physmap(&cmp, nat); > >+ > >+ if ( copy_to_guest(arg, &cmp, 1) ) > >+ return -EFAULT; > > I realize that this is the same way in the code handling > XENMEM_[gs]et_pod_target, but unfortunately that''s wrong (that''s > why I''m copying you, George): Once a continuation was set up, you > mustn''t change the return value anymore, since the continuation was > established by adjusting the guest''s rIP. > > As for other memory ops, the continuation can be encoded in "op" (see > xen/common/memory.c and xen/common/compat/memory.c). However, > while suitable here I don''t think that''s usable for the PoD variant. The > alternative is to cancel the continuation (would require quite a bit of > new code I think) or to adjust the low level hypercall handler code (at > least the compat mode one) to special case rAX values in the negative > errno range, leaving rAX unchanged instead of returning -ENOSYS. > Keir? >Jan, did you have something like the attached patch in mind? We will return EFAULT to the hypercall without touching the guest memory because the copy_to_guest failed. For the example I ignored the multicalls. Jean _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-10 16:26 UTC
[Xen-devel] Re: [PATCH 5/7] mm: New XENMEM space, XENMAPSPACE_gmfn_range
>>> On 10.11.11 at 16:44, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > On 10/11 12:47, Jan Beulich wrote: >> >>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@eu.citrix.com> wrote: >> >@@ -4716,6 +4748,17 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) > arg) >> > } >> > >> > rc = xenmem_add_to_physmap(d, &xatp); >> >+ if ( rc == -EAGAIN ) >> >> if ( rc ) >> >> >+ { >> >+ if ( copy_to_guest(arg, &xatp, 1) ) >> >+ { >> >+ rcu_unlock_domain(d); >> >+ return -EFAULT; >> >+ } >> >> } >> if ( rc == -EAGAIN ) >> >> (with some room for further simplification). Without that (or the minimal >> alternative of copying back just .size or yet some other mechanism), as >> pointed out before, the caller won''t have a way to know how far into >> the batch things succeeded. >> >> >+ >> >+ rc = hypercall_create_continuation( >> >+ __HYPERVISOR_memory_op, "ih", op, arg); >> >+ } >> > >> > rcu_unlock_domain(d); >> >> Also, the whole block above can be move past this rcu_unlock_domain(), >> eliminating the need to do it separately in the above error path(s). >> >> > >> >--- a/xen/arch/x86/x86_64/compat/mm.c >> >+++ b/xen/arch/x86/x86_64/compat/mm.c >> >@@ -63,6 +63,18 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE(void) > arg) >> > >> > XLAT_add_to_physmap(nat, &cmp); >> > rc = arch_memory_op(op, guest_handle_from_ptr(nat, void)); >> >+ if ( rc < 0 ) >> >+ break; >> >+ >> >> With the way the code below is currently this is superfluous. But just >> as above you will need to provide some indication to the caller >> *where* the failure occurred. >> >> >+ if ( rc == __HYPERVISOR_memory_op ) >> >+ { >> >+ hypercall_xlat_continuation(NULL, 0x2, nat, arg); >> >+ >> >+ XLAT_add_to_physmap(&cmp, nat); >> >+ >> >+ if ( copy_to_guest(arg, &cmp, 1) ) >> >+ return -EFAULT; >> >> I realize that this is the same way in the code handling >> XENMEM_[gs]et_pod_target, but unfortunately that''s wrong (that''s >> why I''m copying you, George): Once a continuation was set up, you >> mustn''t change the return value anymore, since the continuation was >> established by adjusting the guest''s rIP. >> >> As for other memory ops, the continuation can be encoded in "op" (see >> xen/common/memory.c and xen/common/compat/memory.c). However, >> while suitable here I don''t think that''s usable for the PoD variant. The >> alternative is to cancel the continuation (would require quite a bit of >> new code I think) or to adjust the low level hypercall handler code (at >> least the compat mode one) to special case rAX values in the negative >> errno range, leaving rAX unchanged instead of returning -ENOSYS. >> Keir? >> > > Jan, did you have something like the attached patch in mind?Indeed, and it''s fortunately much simpler than I would have thought.> We will return EFAULT to the hypercall without touching the guest memory > because the copy_to_guest failed. > > For the example I ignored the multicalls.Adding the support for that would seem to be strait forward too. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-10 17:37 UTC
[Xen-devel] Re: [PATCH 5/7] mm: New XENMEM space, XENMAPSPACE_gmfn_range
On 10/11 12:47, Jan Beulich wrote:> >>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > >@@ -4716,6 +4748,17 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg) > > } > > > > rc = xenmem_add_to_physmap(d, &xatp); > >+ if ( rc == -EAGAIN ) > > if ( rc ) > > >+ { > >+ if ( copy_to_guest(arg, &xatp, 1) ) > >+ { > >+ rcu_unlock_domain(d); > >+ return -EFAULT; > >+ } > > } > if ( rc == -EAGAIN ) > > (with some room for further simplification). Without that (or the minimal > alternative of copying back just .size or yet some other mechanism), as > pointed out before, the caller won''t have a way to know how far into > the batch things succeeded. >In xenmem_add_to_physmap I modify xatp in place so when I exit this function xatp will contain the updated value (new start value in .gpfn and .idx, how far do I need to go in .size). The idea here was to call the copy_to_guest only when we got preempted. If I copy xatp back to the guest I should get the updated value in xatp from the copy_from_guest when I''ll be called by the continuation.> >+ > >+ rc = hypercall_create_continuation( > >+ __HYPERVISOR_memory_op, "ih", op, arg); > >+ } > > > > rcu_unlock_domain(d); > > Also, the whole block above can be move past this rcu_unlock_domain(), > eliminating the need to do it separately in the above error path(s). >Ok. Jean> > > >--- a/xen/arch/x86/x86_64/compat/mm.c > >+++ b/xen/arch/x86/x86_64/compat/mm.c > >@@ -63,6 +63,18 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg) > > > > XLAT_add_to_physmap(nat, &cmp); > > rc = arch_memory_op(op, guest_handle_from_ptr(nat, void)); > >+ if ( rc < 0 ) > >+ break; > >+ > > With the way the code below is currently this is superfluous. But just > as above you will need to provide some indication to the caller > *where* the failure occurred. > > >+ if ( rc == __HYPERVISOR_memory_op ) > >+ { > >+ hypercall_xlat_continuation(NULL, 0x2, nat, arg); > >+ > >+ XLAT_add_to_physmap(&cmp, nat); > >+ > >+ if ( copy_to_guest(arg, &cmp, 1) ) > >+ return -EFAULT; > > I realize that this is the same way in the code handling > XENMEM_[gs]et_pod_target, but unfortunately that''s wrong (that''s > why I''m copying you, George): Once a continuation was set up, you > mustn''t change the return value anymore, since the continuation was > established by adjusting the guest''s rIP. > > As for other memory ops, the continuation can be encoded in "op" (see > xen/common/memory.c and xen/common/compat/memory.c). However, > while suitable here I don''t think that''s usable for the PoD variant. The > alternative is to cancel the continuation (would require quite a bit of > new code I think) or to adjust the low level hypercall handler code (at > least the compat mode one) to special case rAX values in the negative > errno range, leaving rAX unchanged instead of returning -ENOSYS. > Keir? > > Jan > > >+ } > > > > break; > > } >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-11 08:09 UTC
[Xen-devel] Re: [PATCH 5/7] mm: New XENMEM space, XENMAPSPACE_gmfn_range
>>> On 10.11.11 at 18:37, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > On 10/11 12:47, Jan Beulich wrote: >> >>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@eu.citrix.com> wrote: >> >@@ -4716,6 +4748,17 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) > arg) >> > } >> > >> > rc = xenmem_add_to_physmap(d, &xatp); >> >+ if ( rc == -EAGAIN ) >> >> if ( rc ) >> >> >+ { >> >+ if ( copy_to_guest(arg, &xatp, 1) ) >> >+ { >> >+ rcu_unlock_domain(d); >> >+ return -EFAULT; >> >+ } >> >> } >> if ( rc == -EAGAIN ) >> >> (with some room for further simplification). Without that (or the minimal >> alternative of copying back just .size or yet some other mechanism), as >> pointed out before, the caller won''t have a way to know how far into >> the batch things succeeded. >> > > In xenmem_add_to_physmap I modify xatp in place so when I exit this > function xatp will contain the updated value (new start value in > .gpfn and .idx, how far do I need to go in .size). > > The idea here was to call the copy_to_guest only when we got preempted. > If I copy xatp back to the guest I should get the updated value > in xatp from the copy_from_guest when I''ll be called by the continuation.I understand the continuation aspect. But you appear to have not read my comments completely: I''m asking how your caller, in the event of failure, would know how much of the batch was processed successfully. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Nov-11 09:13 UTC
Re: [Xen-devel] Re: [PATCH 5/7] mm: New XENMEM space, XENMAPSPACE_gmfn_range
On Fri, 2011-11-11 at 08:09 +0000, Jan Beulich wrote:> >>> On 10.11.11 at 18:37, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > > On 10/11 12:47, Jan Beulich wrote: > >> >>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > >> >@@ -4716,6 +4748,17 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) > > arg) > >> > } > >> > > >> > rc = xenmem_add_to_physmap(d, &xatp); > >> >+ if ( rc == -EAGAIN ) > >> > >> if ( rc ) > >> > >> >+ { > >> >+ if ( copy_to_guest(arg, &xatp, 1) ) > >> >+ { > >> >+ rcu_unlock_domain(d); > >> >+ return -EFAULT; > >> >+ } > >> > >> } > >> if ( rc == -EAGAIN ) > >> > >> (with some room for further simplification). Without that (or the minimal > >> alternative of copying back just .size or yet some other mechanism), as > >> pointed out before, the caller won''t have a way to know how far into > >> the batch things succeeded. > >> > > > > In xenmem_add_to_physmap I modify xatp in place so when I exit this > > function xatp will contain the updated value (new start value in > > .gpfn and .idx, how far do I need to go in .size). > > > > The idea here was to call the copy_to_guest only when we got preempted. > > If I copy xatp back to the guest I should get the updated value > > in xatp from the copy_from_guest when I''ll be called by the continuation. > > I understand the continuation aspect. But you appear to have not read > my comments completely: I''m asking how your caller, in the event of > failure, would know how much of the batch was processed successfully.For this sort of flush operation can the caller assume that failure means nothing was flushed, since a flush can always be repeated? Ian.> > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-11 09:21 UTC
Re: [Xen-devel] Re: [PATCH 5/7] mm: New XENMEM space, XENMAPSPACE_gmfn_range
>>> On 11.11.11 at 10:13, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Fri, 2011-11-11 at 08:09 +0000, Jan Beulich wrote: >> >>> On 10.11.11 at 18:37, Jean Guyader <jean.guyader@eu.citrix.com> wrote: >> > On 10/11 12:47, Jan Beulich wrote: >> >> >>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@eu.citrix.com> wrote: >> >> >@@ -4716,6 +4748,17 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) >> > arg) >> >> > } >> >> > >> >> > rc = xenmem_add_to_physmap(d, &xatp); >> >> >+ if ( rc == -EAGAIN ) >> >> >> >> if ( rc ) >> >> >> >> >+ { >> >> >+ if ( copy_to_guest(arg, &xatp, 1) ) >> >> >+ { >> >> >+ rcu_unlock_domain(d); >> >> >+ return -EFAULT; >> >> >+ } >> >> >> >> } >> >> if ( rc == -EAGAIN ) >> >> >> >> (with some room for further simplification). Without that (or the minimal >> >> alternative of copying back just .size or yet some other mechanism), as >> >> pointed out before, the caller won''t have a way to know how far into >> >> the batch things succeeded. >> >> >> > >> > In xenmem_add_to_physmap I modify xatp in place so when I exit this >> > function xatp will contain the updated value (new start value in >> > .gpfn and .idx, how far do I need to go in .size). >> > >> > The idea here was to call the copy_to_guest only when we got preempted. >> > If I copy xatp back to the guest I should get the updated value >> > in xatp from the copy_from_guest when I''ll be called by the continuation. >> >> I understand the continuation aspect. But you appear to have not read >> my comments completely: I''m asking how your caller, in the event of >> failure, would know how much of the batch was processed successfully. > > For this sort of flush operation can the caller assume that failure > means nothing was flushed, since a flush can always be repeated?This is not just a flush - instead, the flush is just a necessary sub- operation of what is being done here. I don''t think the actual add- to-physmap should be repeated. And even if it can, at least for diagnostic/debugging purposes knowing where things failed is rather useful. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-11 09:30 UTC
Re: [Xen-devel] Re: [PATCH 5/7] mm: New XENMEM space, XENMAPSPACE_gmfn_range
On 11/11 09:21, Jan Beulich wrote:> >>> On 11.11.11 at 10:13, Ian Campbell <Ian.Campbell@citrix.com> wrote: > > On Fri, 2011-11-11 at 08:09 +0000, Jan Beulich wrote: > >> >>> On 10.11.11 at 18:37, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > >> > On 10/11 12:47, Jan Beulich wrote: > >> >> >>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > >> >> >@@ -4716,6 +4748,17 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) > >> > arg) > >> >> > } > >> >> > > >> >> > rc = xenmem_add_to_physmap(d, &xatp); > >> >> >+ if ( rc == -EAGAIN ) > >> >> > >> >> if ( rc ) > >> >> > >> >> >+ { > >> >> >+ if ( copy_to_guest(arg, &xatp, 1) ) > >> >> >+ { > >> >> >+ rcu_unlock_domain(d); > >> >> >+ return -EFAULT; > >> >> >+ } > >> >> > >> >> } > >> >> if ( rc == -EAGAIN ) > >> >> > >> >> (with some room for further simplification). Without that (or the minimal > >> >> alternative of copying back just .size or yet some other mechanism), as > >> >> pointed out before, the caller won''t have a way to know how far into > >> >> the batch things succeeded. > >> >> > >> > > >> > In xenmem_add_to_physmap I modify xatp in place so when I exit this > >> > function xatp will contain the updated value (new start value in > >> > .gpfn and .idx, how far do I need to go in .size). > >> > > >> > The idea here was to call the copy_to_guest only when we got preempted. > >> > If I copy xatp back to the guest I should get the updated value > >> > in xatp from the copy_from_guest when I''ll be called by the continuation. > >> > >> I understand the continuation aspect. But you appear to have not read > >> my comments completely: I''m asking how your caller, in the event of > >> failure, would know how much of the batch was processed successfully. > > > > For this sort of flush operation can the caller assume that failure > > means nothing was flushed, since a flush can always be repeated? > > This is not just a flush - instead, the flush is just a necessary sub- > operation of what is being done here. I don''t think the actual add- > to-physmap should be repeated. And even if it can, at least for > diagnostic/debugging purposes knowing where things failed is rather > useful. >Ok, I''ll do the copy_to_guest if rc isn''t 0. Do you think I should do it for all the spaces or only for gmfn_range? Jean _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-11 10:57 UTC
Re: [Xen-devel] Re: [PATCH 5/7] mm: New XENMEM space, XENMAPSPACE_gmfn_range
>>> On 11.11.11 at 10:30, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > On 11/11 09:21, Jan Beulich wrote: >> >>> On 11.11.11 at 10:13, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> > On Fri, 2011-11-11 at 08:09 +0000, Jan Beulich wrote: >> >> >>> On 10.11.11 at 18:37, Jean Guyader <jean.guyader@eu.citrix.com> wrote: >> >> > On 10/11 12:47, Jan Beulich wrote: >> >> >> >>> On 10.11.11 at 12:35, Jean Guyader <jean.guyader@eu.citrix.com> wrote: >> >> >> >@@ -4716,6 +4748,17 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) >> >> > arg) >> >> >> > } >> >> >> > >> >> >> > rc = xenmem_add_to_physmap(d, &xatp); >> >> >> >+ if ( rc == -EAGAIN ) >> >> >> >> >> >> if ( rc ) >> >> >> >> >> >> >+ { >> >> >> >+ if ( copy_to_guest(arg, &xatp, 1) ) >> >> >> >+ { >> >> >> >+ rcu_unlock_domain(d); >> >> >> >+ return -EFAULT; >> >> >> >+ } >> >> >> >> >> >> } >> >> >> if ( rc == -EAGAIN ) >> >> >> >> >> >> (with some room for further simplification). Without that (or the minimal >> >> >> alternative of copying back just .size or yet some other mechanism), as >> >> >> pointed out before, the caller won''t have a way to know how far into >> >> >> the batch things succeeded. >> >> >> >> >> > >> >> > In xenmem_add_to_physmap I modify xatp in place so when I exit this >> >> > function xatp will contain the updated value (new start value in >> >> > .gpfn and .idx, how far do I need to go in .size). >> >> > >> >> > The idea here was to call the copy_to_guest only when we got preempted. >> >> > If I copy xatp back to the guest I should get the updated value >> >> > in xatp from the copy_from_guest when I''ll be called by the continuation. >> >> >> >> I understand the continuation aspect. But you appear to have not read >> >> my comments completely: I''m asking how your caller, in the event of >> >> failure, would know how much of the batch was processed successfully. >> > >> > For this sort of flush operation can the caller assume that failure >> > means nothing was flushed, since a flush can always be repeated? >> >> This is not just a flush - instead, the flush is just a necessary sub- >> operation of what is being done here. I don''t think the actual add- >> to-physmap should be repeated. And even if it can, at least for >> diagnostic/debugging purposes knowing where things failed is rather >> useful. >> > > Ok, I''ll do the copy_to_guest if rc isn''t 0. Do you think I should do it > for all the spaces or only for gmfn_range?I''d prefer doing it just for the latter - the others have no outputs, and their input could legitimately (albeit unlikely) live in read-only memory. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel