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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.18 at 10:53, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Paul Durrant
>> Sent: 12 September 2018 09:52
>> 
>> > From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On
>> Behalf
>> > Of Paul Durrant
>> > Sent: 12 September 2018 09:45
>> >
>> > > From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> > > Sent: 12 September 2018 09:44
>> > >
>> > > >>> 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.

And what would guarantee the ASSERT() to not trigger? As said, I'm
fine with this being enforced at a different level, but it needs to be
enforced somewhere.

> Actually, no I still don't think the ASSERT is correct. Why should we rule 
> out HVM guests being able to use PV-IOMMU?

A HVM guest using the PV IOMMU is quite fine, but it shouldn't talk to
it in terms of MFNs.

Jan



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