RE: [Xen-devel] [VTD][patch 1/5] HVM device assignment using vt-d

["Guy Zana" <guy@xxxxxxxxxxxx>]

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