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 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 (6):
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: New XENMEM, XENMEM_add_to_physmap_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 | 203 +++++++++++++++++++++--------------
xen/drivers/passthrough/iommu.c | 25 +++++
xen/drivers/passthrough/vtd/iommu.c | 100 ++++++++++--------
xen/include/public/memory.h | 5 +-
xen/include/xen/iommu.h | 7 ++
6 files changed, 230 insertions(+), 130 deletions(-)
Jean
_______________________________________________
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> --- 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-07 15:16 UTC
[Xen-devel] [PATCH 2/6] iommu: Introduce iommu_flush and iommu_flush_all.
Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.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-07 15:16 UTC
[Xen-devel] [PATCH 3/6] 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 | 188 ++++++++++++++++++++++++++++------------------------- 1 files changed, 99 insertions(+), 89 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-07 15:16 UTC
[Xen-devel] [PATCH 4/6] mm: New XENMEM, XENMEM_add_to_physmap_gmfn_range
XENMEM_add_to_physmap_gmfn_range is like XENMEM_add_to_physmap on the gmfn space but the number of pages on which xen will iterate. Use the 16 bits padding between .domid and .space in struct xen_add_to_physmap to keep compatibility with older versions. Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com> --- xen/arch/x86/mm.c | 22 +++++++++++++++++++++- xen/include/public/memory.h | 5 ++++- 2 files changed, 25 insertions(+), 2 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-07 15:16 UTC
[Xen-devel] [PATCH 5/6] 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-07 15:16 UTC
[Xen-devel] [PATCH 6/6] 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 | 15 ++++++++++++++- xen/drivers/passthrough/iommu.c | 5 +++++ xen/drivers/passthrough/vtd/iommu.c | 6 ++++-- xen/include/xen/iommu.h | 2 ++ 4 files changed, 25 insertions(+), 3 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-07 16:42 UTC
Re: [Xen-devel] [PATCH 2/6] iommu: Introduce iommu_flush and iommu_flush_all.
>>> On 07.11.11 at 16:16, Jean Guyader <jean.guyader@eu.citrix.com> wrote:> Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.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(-)In iommu_iotlb_flush() you check whether the to-be-called function pointer is NULL, whereas in iommu_iotlb_flush_all() you don''t. I actually think the second behavior is the correct one, but that implies that you need to also implement respective AMD IOMMU functions. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-07 16:45 UTC
Re: [Xen-devel] [PATCH 4/6] mm: New XENMEM, XENMEM_add_to_physmap_gmfn_range
>>> On 07.11.11 at 16:16, Jean Guyader <jean.guyader@eu.citrix.com> wrote:> XENMEM_add_to_physmap_gmfn_range is like XENMEM_add_to_physmap on > the gmfn space but the number of pages on which xen will iterate. > > Use the 16 bits padding between .domid and .space in struct > xen_add_to_physmap > to keep compatibility with older versions. > > Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com> > --- > xen/arch/x86/mm.c | 22 +++++++++++++++++++++- > xen/include/public/memory.h | 5 ++++- > 2 files changed, 25 insertions(+), 2 deletions(-)Please remove the stray last hunk of the patch (the newline there should stay). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-07 16:58 UTC
Re: [Xen-devel] [PATCH 4/6] mm: New XENMEM, XENMEM_add_to_physmap_gmfn_range
On 07/11 04:45, Jan Beulich wrote:> >>> On 07.11.11 at 16:16, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > > > XENMEM_add_to_physmap_gmfn_range is like XENMEM_add_to_physmap on > > the gmfn space but the number of pages on which xen will iterate. > > > > Use the 16 bits padding between .domid and .space in struct > > xen_add_to_physmap > > to keep compatibility with older versions. > > > > Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com> > > --- > > xen/arch/x86/mm.c | 22 +++++++++++++++++++++- > > xen/include/public/memory.h | 5 ++++- > > 2 files changed, 25 insertions(+), 2 deletions(-) > > Please remove the stray last hunk of the patch (the newline there > should stay). >Yes, it shouldn''t be there. I''ve updated the patch. Thanks, Jean _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-07 17:06 UTC
Re: [Xen-devel] [PATCH 2/6] iommu: Introduce iommu_flush and iommu_flush_all.
On 07/11 04:42, Jan Beulich wrote:> >>> On 07.11.11 at 16:16, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > > > Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.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(-) > > In iommu_iotlb_flush() you check whether the to-be-called function > pointer is NULL, whereas in iommu_iotlb_flush_all() you don''t. I > actually think the second behavior is the correct one, but that > implies that you need to also implement respective AMD IOMMU > functions. >Yes, It''s an error on my part. I''ve updated the patch to check for the present of iotlb_flush_all before I call it now. Thanks, Jean _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-08 08:24 UTC
Re: [Xen-devel] [PATCH 2/6] iommu: Introduce iommu_flush and iommu_flush_all.
>>> On 07.11.11 at 18:06, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > On 07/11 04:42, Jan Beulich wrote: >> >>> On 07.11.11 at 16:16, Jean Guyader <jean.guyader@eu.citrix.com> wrote: >> >> > Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.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(-) >> >> In iommu_iotlb_flush() you check whether the to-be-called function >> pointer is NULL, whereas in iommu_iotlb_flush_all() you don''t. I >> actually think the second behavior is the correct one, but that >> implies that you need to also implement respective AMD IOMMU >> functions. >> > > Yes, It''s an error on my part. I''ve updated the patch to check > for the present of iotlb_flush_all before I call it now.But as said, I don''t think this is the right solution: How can it be correct on non-Intel hardware to have these functions simply do nothing? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-08 10:25 UTC
Re: [Xen-devel] [PATCH 2/6] iommu: Introduce iommu_flush and iommu_flush_all.
On 08/11 08:24, Jan Beulich wrote:> >>> On 07.11.11 at 18:06, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > > On 07/11 04:42, Jan Beulich wrote: > >> >>> On 07.11.11 at 16:16, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > >> > >> > Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.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(-) > >> > >> In iommu_iotlb_flush() you check whether the to-be-called function > >> pointer is NULL, whereas in iommu_iotlb_flush_all() you don''t. I > >> actually think the second behavior is the correct one, but that > >> implies that you need to also implement respective AMD IOMMU > >> functions. > >> > > > > Yes, It''s an error on my part. I''ve updated the patch to check > > for the present of iotlb_flush_all before I call it now. > > But as said, I don''t think this is the right solution: How can it be > correct on non-Intel hardware to have these functions simply do > nothing? >These functions are only here to counter balance the dont_flush_iotlb_flag. If they don''t acknowlage this flag doing nothing is the right thing to do. That said I can probably implement a naive version for the AMD iommu that will use amd_iommu_flush_pages once or in a loop. Obviously I will remove the check for the non existing callback if I do that as it become mandatory. Jean _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jean Guyader
2011-Nov-08 10:38 UTC
Re: [Xen-devel] [PATCH 2/6] iommu: Introduce iommu_flush and iommu_flush_all.
On 08/11 10:39, Wei Wang2 wrote:> On Tuesday 08 November 2011 11:25:15 Jean Guyader wrote: > > On 08/11 08:24, Jan Beulich wrote: > > > >>> On 07.11.11 at 18:06, Jean Guyader <jean.guyader@eu.citrix.com> > > > >>> wrote: > > > > > > > > On 07/11 04:42, Jan Beulich wrote: > > > >> >>> On 07.11.11 at 16:16, Jean Guyader <jean.guyader@eu.citrix.com> > > > >> >>> wrote: > > > >> > > > > >> > Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.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(-) > > > >> > > > >> In iommu_iotlb_flush() you check whether the to-be-called function > > > >> pointer is NULL, whereas in iommu_iotlb_flush_all() you don''t. I > > > >> actually think the second behavior is the correct one, but that > > > >> implies that you need to also implement respective AMD IOMMU > > > >> functions. > > > > > > > > Yes, It''s an error on my part. I''ve updated the patch to check > > > > for the present of iotlb_flush_all before I call it now. > > > > > > But as said, I don''t think this is the right solution: How can it be > > > correct on non-Intel hardware to have these functions simply do > > > nothing? > > > > These functions are only here to counter balance the dont_flush_iotlb_flag. > > If they don''t acknowlage this flag doing nothing is the right thing to do. > > > > That said I can probably implement a naive version for the AMD iommu > > that will use amd_iommu_flush_pages once or in a loop. > > There are some pending patches that enable ats / iotlb flush for amd iommu. > Then, you might want to use functions: amd_iommu_flush_iotlb and > amd_iommu_flush_all_iotlbs for this. > Thanks, > Wei >Ok, I''ll do that in the next version. Thanks, Jean _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Nov-08 10:39 UTC
Re: [Xen-devel] [PATCH 2/6] iommu: Introduce iommu_flush and iommu_flush_all.
>>> On 08.11.11 at 11:25, Jean Guyader <jean.guyader@eu.citrix.com> wrote: > On 08/11 08:24, Jan Beulich wrote: >> >>> On 07.11.11 at 18:06, Jean Guyader <jean.guyader@eu.citrix.com> wrote: >> > On 07/11 04:42, Jan Beulich wrote: >> >> >>> On 07.11.11 at 16:16, Jean Guyader <jean.guyader@eu.citrix.com> wrote: >> >> >> >> > Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.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(-) >> >> >> >> In iommu_iotlb_flush() you check whether the to-be-called function >> >> pointer is NULL, whereas in iommu_iotlb_flush_all() you don''t. I >> >> actually think the second behavior is the correct one, but that >> >> implies that you need to also implement respective AMD IOMMU >> >> functions. >> >> >> > >> > Yes, It''s an error on my part. I''ve updated the patch to check >> > for the present of iotlb_flush_all before I call it now. >> >> But as said, I don''t think this is the right solution: How can it be >> correct on non-Intel hardware to have these functions simply do >> nothing? >> > > These functions are only here to counter balance the dont_flush_iotlb_flag. > If they don''t acknowlage this flag doing nothing is the right thing to do.Ah, okay, I overlooked this aspect. Might be worth a comment in the header file. Jan> That said I can probably implement a naive version for the AMD iommu > that will use amd_iommu_flush_pages once or in a loop. > > Obviously I will remove the check for the non existing callback if I do > that as it become mandatory. > > Jean_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Wang2
2011-Nov-08 10:39 UTC
Re: [Xen-devel] [PATCH 2/6] iommu: Introduce iommu_flush and iommu_flush_all.
On Tuesday 08 November 2011 11:25:15 Jean Guyader wrote:> On 08/11 08:24, Jan Beulich wrote: > > >>> On 07.11.11 at 18:06, Jean Guyader <jean.guyader@eu.citrix.com> > > >>> wrote: > > > > > > On 07/11 04:42, Jan Beulich wrote: > > >> >>> On 07.11.11 at 16:16, Jean Guyader <jean.guyader@eu.citrix.com> > > >> >>> wrote: > > >> > > > >> > Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.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(-) > > >> > > >> In iommu_iotlb_flush() you check whether the to-be-called function > > >> pointer is NULL, whereas in iommu_iotlb_flush_all() you don''t. I > > >> actually think the second behavior is the correct one, but that > > >> implies that you need to also implement respective AMD IOMMU > > >> functions. > > > > > > Yes, It''s an error on my part. I''ve updated the patch to check > > > for the present of iotlb_flush_all before I call it now. > > > > But as said, I don''t think this is the right solution: How can it be > > correct on non-Intel hardware to have these functions simply do > > nothing? > > These functions are only here to counter balance the dont_flush_iotlb_flag. > If they don''t acknowlage this flag doing nothing is the right thing to do. > > That said I can probably implement a naive version for the AMD iommu > that will use amd_iommu_flush_pages once or in a loop.There are some pending patches that enable ats / iotlb flush for amd iommu. Then, you might want to use functions: amd_iommu_flush_iotlb and amd_iommu_flush_all_iotlbs for this. Thanks, Wei> Obviously I will remove the check for the non existing callback if I do > that as it become mandatory. > > Jean > > _______________________________________________ > 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