We would like to have a no-iommu interface as well, and support pass-through
for machines without an iommu with the Neocleus' 1:1 mapping.
Some of my comments below...
> -----Original Message-----
> From: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
> [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of
> Muli Ben-Yehuda
> Sent: Monday, June 04, 2007 5:18 PM
> To: Kay, Allen M
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [VTD][patch 1/5] HVM device
> assignment using vt-d
>
> Hi there,
>
> Some comments and questions inline. In general I like it and
> hope we can converge to a common IOMMU support layer (for
> Intel, AMD and IBM
> Calgary/CalIOC2) soon.
>
> On Wed, May 30, 2007 at 12:07:15PM -0700, Kay, Allen M wrote:
> > vtd1.patch:
> > - vt-d specific code
> > - low risk changes in common code
> >
> > Signed-off-by: Allen Kay <allen.m.kay@xxxxxxxxx>
> > Signed-off-by: Xiaohui Xin <xiaohui.xin@xxxxxxxxx>
> >
> > diff -r 089696e0c603 buildconfigs/linux-defconfig_xen_x86_64
> > --- a/buildconfigs/linux-defconfig_xen_x86_64 Thu May
> 17 11:42:46 2007 +0100
> > +++ b/buildconfigs/linux-defconfig_xen_x86_64 Wed May
> 30 11:24:05 2007 -0400
> > +
> > + if (iommu_found())
> > + iommu_domain_init(d);
>
> This is where the fun starts :-)
>
> Is it Xen's or dom0's job to find the IOMMU? on one hand,
> dom0 will have all of that code through the Linux VT-d
> support patch, on the other hand, splitting the detection and
> initialization of the platform hardware between Xen and dom0
> leads to artifical interfaces. I guess the question is
> whether it's acceptable to add a whole lot of infrastructure
> to Xen that duplicates stuff that Linux has or will have soon
> for detecting the presence of the IOMMU?
>
iommu_domain_init contains code that is useful for pass-through without an
iommu as well.
Really, iommu != pass-through :-)
It should be really called passthrough_init, and have specific iommu handling
code inside that function.
For instance, I would like to use the g2m_ports struct and the related DOMCTLs
with passthrough without an iommu.
> > +
> > + case XEN_DOMCTL_memory_mapping:
> > + {
> > + struct domain *d;
> > + unsigned long gfn = domctl->u.memory_mapping.first_gfn;
> > + unsigned long mfn = domctl->u.memory_mapping.first_mfn;
> > + unsigned long nr_mfns = domctl->u.memory_mapping.nr_mfns;
> > + int i;
> > +
> > + ret = -EINVAL;
> > + if ( (mfn + nr_mfns - 1) < mfn ) /* wrap? */
> > + break;
> > + ret = -ESRCH;
> > + d = get_domain_by_id(domctl->domain);
> > + if ( d == NULL )
> > + break;
> > + if ( iomem_access_permitted(d, mfn, mfn) ) {
>
> Shouldn't the second mfn be 'mfn + nr_mfns - 1'?
It should be "if ( !iomem_access_permitted(d, mfn, mfn) )" (pay attention to
the NOT sign), and later on, allow access to that region.
Or am I wrong? I didn't notice any other *iomem_permit_access* to those _mfns_
in your patches...
In any case, this function needs to be stupid and stateless... (by just setting
the entries in the P2M table and using the rangeset interface)
> > + ret = iomem_permit_access(d, gfn, gfn + nr_mfns - 1);
> > + ret = 0;
>
> Err... surely one of the above two lines is wrong :-)
ret = 0 couldn't be wrong ;-)
>
> Why not make this a seperate struct, specific to vt-d, with a
> void* iommu pointer in struct hvm_domain (or better yet,
> struct domain)?
> That way we do not bloat struct hvm_domain even when no IOMMU
> is present, as well as make it possible to support different
> IOMMUs, and IOMMUs where the IOMMU data is shared between
> multiple domains.
>
That will be a good idea!
Thanks,
Guy.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home