Re: [Xen-devel] [PATCH v6 08/14] vtd: add lookup_page method to iommu_ops

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf
> Of Paul Durrant
> Sent: 12 September 2018 09:45
> To: 'Jan Beulich' <JBeulich@xxxxxxxx>
> Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Kevin Tian
> <kevin.tian@xxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v6 08/14] vtd: add lookup_page method to
> iommu_ops
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> > Sent: 12 September 2018 09:44
> > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxx>; Kevin Tian
> > <kevin.tian@xxxxxxxxx>; xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> > Subject: RE: [PATCH v6 08/14] vtd: add lookup_page method to
> iommu_ops
> >
> > >>> On 12.09.18 at 10:31, <Paul.Durrant@xxxxxxxxxx> wrote:
> > >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> > >> Sent: 07 September 2018 12:11
> > >>
> > >> >>> On 23.08.18 at 11:47, <paul.durrant@xxxxxxxxxx> wrote:
> > >> > This patch adds a new method to the VT-d IOMMU implementation to
> > find
> > >> the
> > >> > MFN currently mapped by the specified BFN along with a wrapper
> > function
> > >> in
> > >> > generic IOMMU code to call the implementation if it exists.
> > >>
> > >> For this to go in, I think the AMD side of it wants to also be
> implemented.
> > >>
> > >> > --- a/xen/drivers/passthrough/iommu.c
> > >> > +++ b/xen/drivers/passthrough/iommu.c
> > >> > @@ -305,6 +305,17 @@ int iommu_unmap_page(struct domain *d,
> > bfn_t
> > >> bfn)
> > >> >      return rc;
> > >> >  }
> > >> >
> > >> > +int iommu_lookup_page(struct domain *d, bfn_t bfn, mfn_t *mfn,
> > >> > +                      unsigned int *flags)
> > >> > +{
> > >> > +    const struct domain_iommu *hd = dom_iommu(d);
> > >> > +
> > >> > +    if ( !iommu_enabled || !hd->platform_ops )
> > >> > +        return -EOPNOTSUPP;
> > >> > +
> > >> > +    return hd->platform_ops->lookup_page(d, bfn, mfn, flags);
> > >> > +}
> > >>
> > >> Shouldn't this be restricted to PV guests? HVM ones aren't supposed
> > >> to know about MFNs.
> > >
> > > Agreed, but I think this is the wrong level to be applying such a check:
> > > iommu_map_page() is supplied an MFN regardless of whether the
> domain
> > is PV or
> > > HVM, so I think it is reasonable for a lookup function to work in terms of
> > > MFNs.
> >
> > I don't mind much where the check sits, but ASSERT(!is_hvm_domain()),
> > if placed here, should not trigger.
> >
> 
> It will though. I'm going to need to use this function for HVM guests after
> having done a GFN lookup.

Sorry... I'm getting confused myself now. It won't fire because in my case the 
domain here will always by PV (because it is the not the domain owning the 
GFN). I still think this is the wrong level for such a check though, but I'll 
put in the ASSERT.

  Paul

> 
>   Paul
> 
> > Jan
> >
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel