From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Wang2 Subject: Re: [PATCH V2 4/4] amd iommu: interfaces for p2m sharing Date: Mon, 18 Apr 2011 15:48:55 +0200 Message-ID: <201104181548.55760.wei.wang2@amd.com> References: <201104151414.04671.wei.wang2@amd.com> <20110418102636.GB16867@whitby.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="Boundary-00=_HFErN6KOTjHyBOx" Return-path: In-Reply-To: <20110418102636.GB16867@whitby.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Tim Deegan Cc: "xen-devel@lists.xensource.com" , Keir Fraser List-Id: xen-devel@lists.xenproject.org --Boundary-00=_HFErN6KOTjHyBOx Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline On Monday 18 April 2011 12:26:36 Tim Deegan wrote: > At 13:14 +0100 on 15 Apr (1302873244), Wei Wang2 wrote: > > -- > > Advanced Micro Devices GmbH > > Sitz: Dornach, Gemeinde Aschheim, > > Landkreis M=FCnchen Registergericht M=FCnchen, > > HRB Nr. 43632 > > WEEE-Reg-Nr: DE 12919551 > > Gesch=E4ftsf=FChrer: > > Alberto Bozzo, Andrew Bowd > > > > # HG changeset patch > > # User Wei Wang > > # Node ID b5c197b1d2ac80881d0683cdadf12ac33b762614 > > # Parent 1d9b0e45566ec3cd0e293212d9a454920116b2b1 > > Add a generic interface for both vtd and amd iommu p2m sharing. Also > > introduce a new parameter (iommu=3Dsharept) to enable this feature. > > Why does this introduce a separate variable? Surely "iommu=3Dsharept" > should just set iommu_hap_pt_share directly. Also, it looks like > "iommu=3Dsharept" has different semantics on AMD and Intel machines > (i.e., it does nothing on Intel). Was that the intention? Hi Tim True, that is a little weird.. It looks like some vtd systems have compatib= le=20 issue of re-using p2m table for iommu, so vtd code only turns=20 iommu_hap_pt_share on after some compatible checks (in vtd_ept_share). To=20 avoid breaking vtd, I intended to keep this in the new code. But your=20 suggestion also looks good to me. We could set iommu_hap_pt_share directly= =20 and turn it off if vtd_ept_share() fails. New patch has been attached. > The rest of this series looks OK to me. Is it safe to apply the rest > without this patch or should I wait for the next version and apply them > all together? Yes, it should be OK to just apply the rest. Without the last one, p2m shar= ing=20 will not been enabled. Thanks, Wei > Cheers, > > Tim. > > > Signed-off-by: Wei Wang > > > > diff -r 1d9b0e45566e -r b5c197b1d2ac xen/arch/x86/mm/p2m.c > > --- a/xen/arch/x86/mm/p2m.c Fri Apr 15 12:10:39 2011 +0200 > > +++ b/xen/arch/x86/mm/p2m.c Fri Apr 15 12:13:24 2011 +0200 > > @@ -2061,8 +2061,8 @@ int p2m_alloc_table(struct p2m_domain *p > > > > p2m->phys_table =3D pagetable_from_mfn(page_to_mfn(p2m_top)); > > > > - if ( hap_enabled(d) && (boot_cpu_data.x86_vendor =3D=3D > > X86_VENDOR_INTEL) ) - iommu_set_pgd(d); > > + if ( hap_enabled(d) ) > > + iommu_share_p2m_table(d); > > > > P2M_PRINTK("populating p2m table\n"); > > > > diff -r 1d9b0e45566e -r b5c197b1d2ac > > xen/drivers/passthrough/amd/iommu_map.c --- > > a/xen/drivers/passthrough/amd/iommu_map.c Fri Apr 15 12:10:39 2011 +0200 > > +++ b/xen/drivers/passthrough/amd/iommu_map.c Fri Apr 15 12:13:24 2011 > > +0200 @@ -716,6 +716,12 @@ void amd_iommu_share_p2m(struct domain * > > > > ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled ); > > > > + if ( !iommu_sharept ) > > + { > > + iommu_hap_pt_share =3D 0; > > + return; > > + } > > + > > pgd_mfn =3D pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)= )); > > p2m_table =3D mfn_to_page(mfn_x(pgd_mfn)); > > > > diff -r 1d9b0e45566e -r b5c197b1d2ac > > xen/drivers/passthrough/amd/pci_amd_iommu.c --- > > a/xen/drivers/passthrough/amd/pci_amd_iommu.c Fri Apr 15 12:10:39 2011 > > +0200 +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c Fri Apr 15 > > 12:13:24 2011 +0200 @@ -458,4 +458,5 @@ const struct iommu_ops > > amd_iommu_ops =3D { > > .read_msi_from_ire =3D amd_iommu_read_msi_from_ire, > > .suspend =3D amd_iommu_suspend, > > .resume =3D amd_iommu_resume, > > + .share_p2m =3D amd_iommu_share_p2m, > > }; > > diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/iommu.c > > --- a/xen/drivers/passthrough/iommu.c Fri Apr 15 12:10:39 2011 +0200 > > +++ b/xen/drivers/passthrough/iommu.c Fri Apr 15 12:13:24 2011 +0200 > > @@ -50,6 +50,7 @@ bool_t __read_mostly iommu_hap_pt_share; > > bool_t __read_mostly iommu_hap_pt_share; > > bool_t __read_mostly amd_iommu_debug; > > bool_t __read_mostly amd_iommu_perdev_intremap; > > +bool_t __read_mostly iommu_sharept; > > > > static void __init parse_iommu_param(char *s) > > { > > @@ -82,6 +83,8 @@ static void __init parse_iommu_param(cha > > iommu_passthrough =3D 1; > > else if ( !strcmp(s, "dom0-strict") ) > > iommu_dom0_strict =3D 1; > > + else if ( !strcmp(s, "sharept") ) > > + iommu_sharept =3D 1; > > > > s =3D ss + 1; > > } while ( ss ); > > @@ -419,6 +422,14 @@ void iommu_suspend() > > ops->suspend(); > > } > > > > +void iommu_share_p2m_table(struct domain* d) > > +{ > > + const struct iommu_ops *ops =3D iommu_get_ops(); > > + > > + if ( iommu_enabled && is_hvm_domain(d) ) > > + ops->share_p2m(d); > > +} > > + > > /* > > * Local variables: > > * mode: C > > diff -r 1d9b0e45566e -r b5c197b1d2ac xen/drivers/passthrough/vtd/iommu.c > > --- a/xen/drivers/passthrough/vtd/iommu.c Fri Apr 15 12:10:39 2011 +0200 > > +++ b/xen/drivers/passthrough/vtd/iommu.c Fri Apr 15 12:13:24 2011 +0200 > > @@ -1747,7 +1747,7 @@ static bool_t vtd_ept_share(void) > > bool_t share =3D TRUE; > > > > /* sharept defaults to 0 for now, default to 1 when feature matures > > */ - if ( !sharept ) > > + if ( !iommu_sharept ) > > share =3D FALSE; > > > > /* > > @@ -2299,6 +2299,7 @@ const struct iommu_ops intel_iommu_ops =3D > > .read_msi_from_ire =3D msi_msg_read_remap_rte, > > .suspend =3D vtd_suspend, > > .resume =3D vtd_resume, > > + .share_p2m =3D iommu_set_pgd, > > }; > > > > /* > > diff -r 1d9b0e45566e -r b5c197b1d2ac xen/include/xen/iommu.h > > --- a/xen/include/xen/iommu.h Fri Apr 15 12:10:39 2011 +0200 > > +++ b/xen/include/xen/iommu.h Fri Apr 15 12:13:24 2011 +0200 > > @@ -33,6 +33,7 @@ extern bool_t iommu_hap_pt_share; > > extern bool_t iommu_hap_pt_share; > > extern bool_t amd_iommu_debug; > > extern bool_t amd_iommu_perdev_intremap; > > +extern bool_t iommu_sharept; > > > > extern struct rangeset *mmio_ro_ranges; > > > > @@ -132,6 +133,7 @@ struct iommu_ops { > > unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int > > reg); void (*suspend)(void); > > void (*resume)(void); > > + void (*share_p2m)(struct domain *d); > > }; > > > > void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, > > unsigned int value); @@ -143,5 +145,6 @@ void iommu_resume(void); > > void iommu_resume(void); > > > > void iommu_set_dom0_mapping(struct domain *d); > > +void iommu_share_p2m_table(struct domain *d); > > > > #endif /* _IOMMU_H_ */ --Boundary-00=_HFErN6KOTjHyBOx Content-Type: text/x-diff; charset="iso-8859-1"; name="amd_iommu_p2m_4_new.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="amd_iommu_p2m_4_new.patch" Content-Description: amd_iommu_p2m_4_new.patch # HG changeset patch # User Wei Wang # Node ID 15a9daf8a8aedf21e941a9ed3cb250536ac6972f # Parent 5846cc03cf0c1a4b02eba76a7b4aa4884de5e957 Add a generic interface for both vtd and amd iommu p2m sharing. Also introduce a new parameter (iommu=sharept) to enable this feature. Signed-off-by: Wei Wang diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/arch/x86/mm/p2m.c --- a/xen/arch/x86/mm/p2m.c Mon Apr 18 15:01:52 2011 +0200 +++ b/xen/arch/x86/mm/p2m.c Mon Apr 18 15:37:28 2011 +0200 @@ -2061,8 +2061,8 @@ int p2m_alloc_table(struct p2m_domain *p p2m->phys_table = pagetable_from_mfn(page_to_mfn(p2m_top)); - if ( hap_enabled(d) && (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) ) - iommu_set_pgd(d); + if ( hap_enabled(d) ) + iommu_share_p2m_table(d); P2M_PRINTK("populating p2m table\n"); diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/drivers/passthrough/amd/iommu_map.c --- a/xen/drivers/passthrough/amd/iommu_map.c Mon Apr 18 15:01:52 2011 +0200 +++ b/xen/drivers/passthrough/amd/iommu_map.c Mon Apr 18 15:37:28 2011 +0200 @@ -716,6 +716,9 @@ void amd_iommu_share_p2m(struct domain * ASSERT( is_hvm_domain(d) && d->arch.hvm_domain.hap_enabled ); + if ( !iommu_hap_pt_share ) + return; + pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d))); p2m_table = mfn_to_page(mfn_x(pgd_mfn)); @@ -726,8 +729,6 @@ void amd_iommu_share_p2m(struct domain * /* When sharing p2m with iommu, paging mode = 4 */ hd->paging_mode = IOMMU_PAGING_MODE_LEVEL_4; - iommu_hap_pt_share = 1; - AMD_IOMMU_DEBUG("Share p2m table with iommu: p2m table = 0x%lx\n", mfn_x(pgd_mfn)); } diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/drivers/passthrough/amd/pci_amd_iommu.c --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c Mon Apr 18 15:01:52 2011 +0200 +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c Mon Apr 18 15:37:28 2011 +0200 @@ -458,4 +458,5 @@ const struct iommu_ops amd_iommu_ops = { .read_msi_from_ire = amd_iommu_read_msi_from_ire, .suspend = amd_iommu_suspend, .resume = amd_iommu_resume, + .share_p2m = amd_iommu_share_p2m, }; diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/drivers/passthrough/iommu.c --- a/xen/drivers/passthrough/iommu.c Mon Apr 18 15:01:52 2011 +0200 +++ b/xen/drivers/passthrough/iommu.c Mon Apr 18 15:37:28 2011 +0200 @@ -82,6 +82,8 @@ static void __init parse_iommu_param(cha iommu_passthrough = 1; else if ( !strcmp(s, "dom0-strict") ) iommu_dom0_strict = 1; + else if ( !strcmp(s, "sharept") ) + iommu_hap_pt_share = 1; s = ss + 1; } while ( ss ); @@ -419,6 +421,14 @@ void iommu_suspend() ops->suspend(); } +void iommu_share_p2m_table(struct domain* d) +{ + const struct iommu_ops *ops = iommu_get_ops(); + + if ( iommu_enabled && is_hvm_domain(d) ) + ops->share_p2m(d); +} + /* * Local variables: * mode: C diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/drivers/passthrough/vtd/iommu.c --- a/xen/drivers/passthrough/vtd/iommu.c Mon Apr 18 15:01:52 2011 +0200 +++ b/xen/drivers/passthrough/vtd/iommu.c Mon Apr 18 15:37:28 2011 +0200 @@ -45,9 +45,6 @@ #define nr_ioapics iosapic_get_nr_iosapics() #endif -static int sharept = 0; -boolean_param("sharept", sharept); - int nr_iommus; static void setup_dom0_devices(struct domain *d); @@ -1747,7 +1744,7 @@ static bool_t vtd_ept_share(void) bool_t share = TRUE; /* sharept defaults to 0 for now, default to 1 when feature matures */ - if ( !sharept ) + if ( !iommu_hap_pt_share ) share = FALSE; /* @@ -2299,6 +2296,7 @@ const struct iommu_ops intel_iommu_ops = .read_msi_from_ire = msi_msg_read_remap_rte, .suspend = vtd_suspend, .resume = vtd_resume, + .share_p2m = iommu_set_pgd, }; /* diff -r 5846cc03cf0c -r 15a9daf8a8ae xen/include/xen/iommu.h --- a/xen/include/xen/iommu.h Mon Apr 18 15:01:52 2011 +0200 +++ b/xen/include/xen/iommu.h Mon Apr 18 15:37:28 2011 +0200 @@ -132,6 +132,7 @@ struct iommu_ops { unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg); void (*suspend)(void); void (*resume)(void); + void (*share_p2m)(struct domain *d); }; void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value); @@ -143,5 +144,6 @@ void iommu_resume(void); void iommu_resume(void); void iommu_set_dom0_mapping(struct domain *d); +void iommu_share_p2m_table(struct domain *d); #endif /* _IOMMU_H_ */ --Boundary-00=_HFErN6KOTjHyBOx Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --Boundary-00=_HFErN6KOTjHyBOx--