WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH V2 4/4] amd iommu: interfaces for p2m sharing

To: Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH V2 4/4] amd iommu: interfaces for p2m sharing
From: Wei Wang2 <wei.wang2@xxxxxxx>
Date: Mon, 18 Apr 2011 15:48:55 +0200
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Keir Fraser <keir@xxxxxxx>
Delivery-date: Mon, 18 Apr 2011 06:52:42 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20110418102636.GB16867@xxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <201104151414.04671.wei.wang2@xxxxxxx> <20110418102636.GB16867@xxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: KMail/1.9.6 (enterprise 20070904.708012)
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ünchen Registergericht München,
> > HRB Nr. 43632
> > WEEE-Reg-Nr: DE 12919551
> > Geschäftsführer:
> > Alberto Bozzo, Andrew Bowd
> >
> > # HG changeset patch
> > # User Wei Wang <wei.wang2@xxxxxxx>
> > # Node ID b5c197b1d2ac80881d0683cdadf12ac33b762614
> > # Parent  1d9b0e45566ec3cd0e293212d9a454920116b2b1
> > Add a generic interface for both vtd and amd iommu p2m sharing. Also
> > introduce a new parameter (iommu=sharept) to enable this feature.
>
> Why does this introduce a separate variable?  Surely "iommu=sharept"
> should just set iommu_hap_pt_share directly.  Also, it looks like
> "iommu=sharept" 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 compatible 
issue of re-using p2m table for iommu, so vtd code only turns 
iommu_hap_pt_share on after some compatible checks (in vtd_ept_share). To 
avoid breaking vtd, I intended to keep this in the new code. But your 
suggestion also looks good to me. We could set iommu_hap_pt_share directly 
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 sharing 
will not been enabled.
Thanks,
Wei


> Cheers,
>
> Tim.
>
> > Signed-off-by: Wei Wang <wei.wang2@xxxxxxx>
> >
> > 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 = 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 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 = 0;
> > +        return;
> > +    }
> > +
> >      pgd_mfn = pagetable_get_mfn(p2m_get_pagetable(p2m_get_hostp2m(d)));
> >      p2m_table = 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 = {
> >      .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 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 = 1;
> >          else if ( !strcmp(s, "dom0-strict") )
> >              iommu_dom0_strict = 1;
> > +        else if ( !strcmp(s, "sharept") )
> > +            iommu_sharept = 1;
> >
> >          s = 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 = 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 = TRUE;
> >
> >      /* sharept defaults to 0 for now, default to 1 when feature matures
> > */ -    if ( !sharept )
> > +    if ( !iommu_sharept )
> >          share = FALSE;
> >
> >      /*
> > @@ -2299,6 +2299,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 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_ */


Attachment: amd_iommu_p2m_4_new.patch
Description: amd_iommu_p2m_4_new.patch

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel