|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 3/4] amd-iommu: disable iommu_hap_pt_share with AMD IOMMUs
On 08/08/14 17:36, Suravee Suthikulanit wrote:
> On 7/22/2014 12:39 PM, Roger Pau Monnà wrote:
>> On 22/07/14 19:30, Suravee Suthikulpanit wrote:
>>> Roger,
>>>
>>> I am not quite sure why you would disable "iommu_hap_pt_share" for AMD
>>> IOMMU. The current implementation assumes that the p2m can be shared.
>>>
>>> Also, I feel that simply just set iommu_hap_pt_share = 0 (while still
>>> having several places in the AMD iommu drivers and p2m-pt.c assuming
>>> that it can be shared) seems a bit messy.
>>
>> According to the comment in p2m.h, AMD IOMMU only supports bit 52 to bit
>> 58 in the pte to be 0, otherwise the hw generates page faults.
>>
>> If we want to support doing IO to devices behind an IOMMU from page
>> types different than p2m_ram_rw the p2m tables cannot be shared, because
>> the bits from 52 to 58 will indeed be different than 0, and will
>> generate page faults.
>>
>> Roger.
>>
>
> As you have mentioned, they cannot be shared due to the 52 and 58 bits.
> However, what I was trying to say is that, besides just simply set the
> flag to 0, we probably should remove existing logic in various places
> that assumes that AMD IOMMU can have share_p2m_table=1.
>
> If you are agree, the attachment is the patch that should do that.
>
> I have tested device-passthrough w/ the amd-iommu: disable
> iommu_hap_pt_share with AMD IOMMUs, and my patch and it is working.
>
> Acked-by Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
>
> Thanks,
>
> Suravee
>
> iommu-removal-of-share_p2m_table-from-AMD-IOMMU.patch
>
>
> From f28838679867fbbc3be6286556eed7f908eea559 Mon Sep 17 00:00:00 2001
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
> Date: Fri, 8 Aug 2014 07:26:15 -0500
> Subject: [PATCH] iommu: Removal of share_p2m_table from AMD IOMMU
>
> This patch removes existing logics which assumes iommu_hap_pt_share
> is enabled for AMD IOMMU.
>
> Signed-off-by Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> Cd: Roger Pau MonnÃÂ <roger.pau@xxxxxxxxxx>
> Cc: Xiantao Zhang <xiantao.zhang@xxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> NOTES: This patch depends on the "amd-iommu: disable iommu_hap_pt_share with
> AMD IOMMUs"
> patch from Roger Pau Monne <roger.pau@xxxxxxxxxx>.
>
> xen/arch/x86/mm/p2m-pt.c | 23 ++++++--------------
> xen/drivers/passthrough/amd/iommu_map.c | 33
> -----------------------------
> xen/drivers/passthrough/amd/pci_amd_iommu.c | 4 ----
> xen/drivers/passthrough/iommu.c | 2 +-
> 4 files changed, 8 insertions(+), 54 deletions(-)
>
> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
> index 085ab6f..6bec0e9 100644
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -36,7 +36,6 @@
> #include <xen/event.h>
> #include <xen/trace.h>
> #include <asm/hvm/nestedhvm.h>
> -#include <asm/hvm/svm/amd-iommu-proto.h>
>
> #include "mm-locks.h"
>
> @@ -653,22 +652,14 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long
> gfn, mfn_t mfn,
>
> if ( iommu_enabled && need_iommu(p2m->domain) )
> {
> - if ( iommu_hap_pt_share )
> - {
> - if ( old_mfn && (old_mfn != mfn_x(mfn)) )
> - amd_iommu_flush_pages(p2m->domain, gfn, page_order);
> - }
> + unsigned int flags = p2m_get_iommu_flags(p2mt);
> +
> + if ( flags != 0 )
> + for ( i = 0; i < (1UL << page_order); i++ )
> + iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, flags);
> else
> - {
> - unsigned int flags = p2m_get_iommu_flags(p2mt);
> -
> - if ( flags != 0 )
> - for ( i = 0; i < (1UL << page_order); i++ )
> - iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, flags);
> - else
> - for ( int i = 0; i < (1UL << page_order); i++ )
> - iommu_unmap_page(p2m->domain, gfn+i);
> - }
> + for ( int i = 0; i < (1UL << page_order); i++ )
> + iommu_unmap_page(p2m->domain, gfn+i);
> }
>
> out:
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c
> b/xen/drivers/passthrough/amd/iommu_map.c
> index a8c60ec..2808c31 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -640,9 +640,6 @@ int amd_iommu_map_page(struct domain *d, unsigned long
> gfn, unsigned long mfn,
>
> BUG_ON( !hd->arch.root_table );
>
> - if ( iommu_use_hap_pt(d) )
> - return 0;
> -
> memset(pt_mfn, 0, sizeof(pt_mfn));
>
> spin_lock(&hd->arch.mapping_lock);
> @@ -718,9 +715,6 @@ int amd_iommu_unmap_page(struct domain *d, unsigned long
> gfn)
>
> BUG_ON( !hd->arch.root_table );
>
> - if ( iommu_use_hap_pt(d) )
> - return 0;
> -
> memset(pt_mfn, 0, sizeof(pt_mfn));
>
> spin_lock(&hd->arch.mapping_lock);
> @@ -777,30 +771,3 @@ int amd_iommu_reserve_domain_unity_map(struct domain
> *domain,
> }
> return 0;
> }
> -
> -/* Share p2m table with iommu. */
> -void amd_iommu_share_p2m(struct domain *d)
> -{
> - struct hvm_iommu *hd = domain_hvm_iommu(d);
> - struct page_info *p2m_table;
> - mfn_t pgd_mfn;
> -
> - ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled );
> -
> - if ( !iommu_use_hap_pt(d) )
> - return;
> -
> - pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
> - p2m_table = mfn_to_page(mfn_x(pgd_mfn));
> -
> - if ( hd->arch.root_table != p2m_table )
> - {
> - free_amd_iommu_pgtable(hd->arch.root_table);
> - hd->arch.root_table = p2m_table;
> -
> - /* When sharing p2m with iommu, paging mode = 4 */
> - hd->arch.paging_mode = IOMMU_PAGING_MODE_LEVEL_4;
> - AMD_IOMMU_DEBUG("Share p2m table with iommu: p2m table = %#lx\n",
> - mfn_x(pgd_mfn));
> - }
> -}
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 0b301b3..c893dea 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -453,9 +453,6 @@ static void deallocate_iommu_page_tables(struct domain *d)
> {
> struct hvm_iommu *hd = domain_hvm_iommu(d);
>
> - if ( iommu_use_hap_pt(d) )
> - return;
> -
> spin_lock(&hd->arch.mapping_lock);
> if ( hd->arch.root_table )
> {
> @@ -619,7 +616,6 @@ const struct iommu_ops amd_iommu_ops = {
> .setup_hpet_msi = amd_setup_hpet_msi,
> .suspend = amd_iommu_suspend,
> .resume = amd_iommu_resume,
> - .share_p2m = amd_iommu_share_p2m,
> .crash_shutdown = amd_iommu_suspend,
> .dump_p2m_table = amd_dump_p2m_table,
> };
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index cc12735..5d3299a 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -332,7 +332,7 @@ void iommu_share_p2m_table(struct domain* d)
> {
> const struct iommu_ops *ops = iommu_get_ops();
>
> - if ( iommu_enabled && is_hvm_domain(d) )
> + if ( iommu_enabled && is_hvm_domain(d) && ops->share_p2m)
> ops->share_p2m(d);
Looks fine to me, however I'm not sure if it would be clearer to use
iommu_use_hap_pt(d) instead of checking ops->share_p2m directly.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |