Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Jan Beulich]

>>> On 20.03.18 at 10:32,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 20 March 2018 08:12
>> 
>> >>> On 19.03.18 at 17:57,  wrote:
>> > What I meant was that safety (against underflow) is predicated on
>> > iommu_lookup_page() failing if the mapping was not established through
>> an
>> > iommu op hypercall. So, the only things that should be valid in the iommu
>> > (and hence that iommu_lookup_page() would succeed for) at the point
>> where the
>> > guest starts to boot must all fall within reserved regions, so thay they 
>> > are
>> > ruled out by the earlier check.
>> 
>> Ah, I see. What I don't see is how you want to arrange for that.
>> The tool stack wouldn't know ahead of time whether the guest
>> wants to use the PV IOMMU interfaces, would it? IOW rather than
>> guaranteeing said state at start of guest, shouldn't you blow away
>> all non-special mappings the first time a PV IOMMU request is made?
>> 
> 
> I suspect we want both. Kevin suggested a 'big switch' when the domain 
> boots, in which I could blow away all non-reserved mappings. But, for 
> performance sake, I think it would also be worth a Xen command line option to 
> avoid populating the IOMMU mappings for dom0 in the first place (so when it 
> pulls the 'big switch' it's a no-op). Non-aware dom0s will, of course, 
> probably 
> fail to boot but whoever is setting the command line for Xen should know what 
> their dom0 is capable of. As for other domains, it may be worth adding a 
> similar domain create option to the toolstack but that could be done at a 
> later date.

Oh, yes, options to avoid the entire teardown are certainly going
to be a good thing to have.

Jan


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

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 20 March 2018 08:12
> To: Paul Durrant 
> Cc: Andrew Cooper ; George Dunlap
> ; Ian Jackson ; Wei Liu
> ; Stefano Stabellini ; xen-
> de...@lists.xenproject.org; Tim (Xen.org) 
> Subject: RE: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and
> unmap pages, and also to flush the IOTLB
> 
> >>> On 19.03.18 at 17:57,  wrote:
> >>  -Original Message-
> > [snip]
> >> >> How are you making sure this is a mapping that was established via
> >> >> the map op? Without that this can be (ab)used to ...
> >> >>
> >> >> > +put_page(page);
> >> >>
> >> >> ... underflow the refcount of a page.
> >> >>
> >> >
> >> > Yes, I guess I need to ensure that only non-RAM (i.e. RMRR and E820
> >> reserved
> >> > areas) are mapped through the IOMMU or this could indeed be abused.
> >>
> >> Now I'm confused - then you don't need to deal with struct page_info
> >> and page references at all. Nor would you need to call
> >> get_page_from_gfn() and check p2m_is_any_ram(). Also - what use
> >> would the interface be if you couldn't map any RAM?
> >>
> >
> > Sorry to confuse...
> >
> > What I meant was that safety (against underflow) is predicated on
> > iommu_lookup_page() failing if the mapping was not established through
> an
> > iommu op hypercall. So, the only things that should be valid in the iommu
> > (and hence that iommu_lookup_page() would succeed for) at the point
> where the
> > guest starts to boot must all fall within reserved regions, so thay they are
> > ruled out by the earlier check.
> 
> Ah, I see. What I don't see is how you want to arrange for that.
> The tool stack wouldn't know ahead of time whether the guest
> wants to use the PV IOMMU interfaces, would it? IOW rather than
> guaranteeing said state at start of guest, shouldn't you blow away
> all non-special mappings the first time a PV IOMMU request is made?
> 

I suspect we want both. Kevin suggested a 'big switch' when the domain boots, 
in which I could blow away all non-reserved mappings. But, for performance 
sake, I think it would also be worth a Xen command line option to avoid 
populating the IOMMU mappings for dom0 in the first place (so when it pulls the 
'big switch' it's a no-op). Non-aware dom0s will, of course, probably fail to 
boot but whoever is setting the command line for Xen should know what their 
dom0 is capable of. As for other domains, it may be worth adding a similar 
domain create option to the toolstack but that could be done at a later date.

  Paul

> Jan


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

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Jan Beulich]

>>> On 19.03.18 at 17:57,  wrote:
>>  -Original Message-
> [snip]
>> >> How are you making sure this is a mapping that was established via
>> >> the map op? Without that this can be (ab)used to ...
>> >>
>> >> > +put_page(page);
>> >>
>> >> ... underflow the refcount of a page.
>> >>
>> >
>> > Yes, I guess I need to ensure that only non-RAM (i.e. RMRR and E820
>> reserved
>> > areas) are mapped through the IOMMU or this could indeed be abused.
>> 
>> Now I'm confused - then you don't need to deal with struct page_info
>> and page references at all. Nor would you need to call
>> get_page_from_gfn() and check p2m_is_any_ram(). Also - what use
>> would the interface be if you couldn't map any RAM?
>> 
> 
> Sorry to confuse...
> 
> What I meant was that safety (against underflow) is predicated on 
> iommu_lookup_page() failing if the mapping was not established through an 
> iommu op hypercall. So, the only things that should be valid in the iommu 
> (and hence that iommu_lookup_page() would succeed for) at the point where the 
> guest starts to boot must all fall within reserved regions, so thay they are 
> ruled out by the earlier check.

Ah, I see. What I don't see is how you want to arrange for that.
The tool stack wouldn't know ahead of time whether the guest
wants to use the PV IOMMU interfaces, would it? IOW rather than
guaranteeing said state at start of guest, shouldn't you blow away
all non-special mappings the first time a PV IOMMU request is made?

Jan


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

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Paul Durrant]

> -Original Message-
[snip]
> >> How are you making sure this is a mapping that was established via
> >> the map op? Without that this can be (ab)used to ...
> >>
> >> > +put_page(page);
> >>
> >> ... underflow the refcount of a page.
> >>
> >
> > Yes, I guess I need to ensure that only non-RAM (i.e. RMRR and E820
> reserved
> > areas) are mapped through the IOMMU or this could indeed be abused.
> 
> Now I'm confused - then you don't need to deal with struct page_info
> and page references at all. Nor would you need to call
> get_page_from_gfn() and check p2m_is_any_ram(). Also - what use
> would the interface be if you couldn't map any RAM?
> 

Sorry to confuse...

What I meant was that safety (against underflow) is predicated on 
iommu_lookup_page() failing if the mapping was not established through an iommu 
op hypercall. So, the only things that should be valid in the iommu (and hence 
that iommu_lookup_page() would succeed for) at the point where the guest starts 
to boot must all fall within reserved regions, so thay they are ruled out by 
the earlier check.

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

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 19 March 2018 15:12
> To: Paul Durrant 
> Cc: Andrew Cooper ; Wei Liu
> ; George Dunlap ; Ian
> Jackson ; Stefano Stabellini
> ; xen-devel@lists.xenproject.org; Konrad Rzeszutek
> Wilk ; Tim (Xen.org) 
> Subject: Re: [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and
> also to flush the IOTLB
> 
> >>> On 12.02.18 at 11:47,  wrote:
> > This patch adds iommu_ops to allow a domain with control_iommu
> privilege
> > to map and unmap pages from any guest over which it has mapping
> privilege
> > in the IOMMU.
> > These operations implicitly disable IOTLB flushing so that the caller can
> > batch operations and then explicitly flush the IOTLB using the iommu_op
> > also added by this patch.
> 
> Can't this be abused for unmaps?

Hmm. I think we're ok. The calls just play with the CPU local flush disable 
flag so they should only disable anything resulting from the current hypercall. 
Manipulation of other IOMMU page tables (on behalf of other domains) should not 
be affected AFAICT. I'll double check though.

> 
> > --- a/xen/arch/x86/iommu_op.c
> > +++ b/xen/arch/x86/iommu_op.c
> > @@ -24,6 +24,174 @@
> >  #include 
> >  #include 
> >
> > +/* Override macros from asm/page.h to make them work with mfn_t */
> > +#undef mfn_to_page
> > +#define mfn_to_page(mfn) __mfn_to_page(mfn_x(mfn))
> > +#undef page_to_mfn
> > +#define page_to_mfn(page) _mfn(__page_to_mfn(page))
> 
> I guess with Julien's this needs to go away, but it looks like his
> series hasn't landed yet.
> 

Yes, I'll remove this once that happens.

> > +struct check_rdm_ctxt {
> > +bfn_t bfn;
> > +};
> > +
> > +static int check_rdm(xen_pfn_t start, xen_ulong_t nr, u32 id, void *arg)
> 
> uint32_t
> 

Yep.

> > +{
> > +struct check_rdm_ctxt *ctxt = arg;
> > +
> > +if ( bfn_x(ctxt->bfn) >= start &&
> > + bfn_x(ctxt->bfn) < start + nr )
> > +return -EINVAL;
> 
> Something more distinguishable than EINVAL would certainly be
> nice here. Also how come this check does not depend on the
> domain? Only RMRRs of devices owned by a domain are relevant
> in the BFN range (unless I still didn't fully understand how BFN is
> meant to be different from GFN and MFN).
> 

I thought that the reserved range check was only for the current domain's 
mappings (optionally limited to a single initiator), but I could be wrong. I'll 
check.

> > +static int iommuop_map(struct xen_iommu_op_map *op, unsigned int
> flags)
> > +{
> > +struct domain *d, *od, *currd = current->domain;
> > +struct domain_iommu *iommu = dom_iommu(currd);
> > +const struct iommu_ops *ops = iommu->platform_ops;
> > +domid_t domid = op->domid;
> > +gfn_t gfn = _gfn(op->gfn);
> > +bfn_t bfn = _bfn(op->bfn);
> > +mfn_t mfn;
> > +struct check_rdm_ctxt ctxt = {
> > +.bfn = bfn,
> > +};
> > +p2m_type_t p2mt;
> > +p2m_query_t p2mq;
> > +struct page_info *page;
> > +unsigned int prot;
> > +int rc;
> > +
> > +if (op->pad0 != 0 || op->pad1 != 0)
> 
> Missing blanks again (and please again consider dropping the " != 0").
> 
> > +return -EINVAL;
> > +
> > +/*
> > + * Both map_page and lookup_page operations must be implemented.
> > + * The lookup_page method is not used here but is relied upon by
> > + * iommuop_unmap() to drop the page reference taken here.
> > + */
> > +if ( !ops->map_page || !ops->lookup_page )
> > +return -ENOSYS;
> 
> EOPNOTSUPP (also further down)
> 

I wanted the 'not implemented' case to be distinct from the 'not supported 
because of some configuration detail' case, which is why I chose ENOSYS. I'll 
change it if you don't think that matters though.

> Also how about the unmap hook? If that's not implemented, how
> would the page ref obtained below ever be dropped again? Or
> you may need to re-order the unmap side code.

Ok. I'll just check for all map, unmap and lookup in both cases.

> 
> > +/* Check whether the specified BFN falls in a reserved region */
> > +rc = iommu_get_reserved_device_memory(check_rdm, );
> > +if ( rc )
> > +return rc;
> > +
> > +d = rcu_lock_domain_by_any_id(domid);
> > +if ( !d )
> > +return -ESRCH;
> > +
> > +p2mq = (flags & XEN_IOMMUOP_map_readonly) ?
> > +P2M_UNSHARE : P2M_ALLOC;
> 
> Isn't this the wrong way round?
> 

I don't think so. If we're doing a readonly mapping then the page should not be 
forcibly populated, right?

> > +page = get_page_from_gfn(d, gfn_x(gfn), , p2mq);
> > +
> > +rc = -ENOENT;
> > +if ( !page )
> > +goto unlock;
> > +
> > +if ( p2m_is_paged(p2mt) )
> > +{
> > +p2m_mem_paging_populate(d, gfn_x(gfn));
> > +goto 

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Jan Beulich]

>>> On 12.02.18 at 11:47,  wrote:
> This patch adds iommu_ops to allow a domain with control_iommu privilege
> to map and unmap pages from any guest over which it has mapping privilege
> in the IOMMU.
> These operations implicitly disable IOTLB flushing so that the caller can
> batch operations and then explicitly flush the IOTLB using the iommu_op
> also added by this patch.

Can't this be abused for unmaps?

> --- a/xen/arch/x86/iommu_op.c
> +++ b/xen/arch/x86/iommu_op.c
> @@ -24,6 +24,174 @@
>  #include 
>  #include 
>  
> +/* Override macros from asm/page.h to make them work with mfn_t */
> +#undef mfn_to_page
> +#define mfn_to_page(mfn) __mfn_to_page(mfn_x(mfn))
> +#undef page_to_mfn
> +#define page_to_mfn(page) _mfn(__page_to_mfn(page))

I guess with Julien's this needs to go away, but it looks like his
series hasn't landed yet.

> +struct check_rdm_ctxt {
> +bfn_t bfn;
> +};
> +
> +static int check_rdm(xen_pfn_t start, xen_ulong_t nr, u32 id, void *arg)

uint32_t

> +{
> +struct check_rdm_ctxt *ctxt = arg;
> +
> +if ( bfn_x(ctxt->bfn) >= start &&
> + bfn_x(ctxt->bfn) < start + nr )
> +return -EINVAL;

Something more distinguishable than EINVAL would certainly be
nice here. Also how come this check does not depend on the
domain? Only RMRRs of devices owned by a domain are relevant
in the BFN range (unless I still didn't fully understand how BFN is
meant to be different from GFN and MFN).

> +static int iommuop_map(struct xen_iommu_op_map *op, unsigned int flags)
> +{
> +struct domain *d, *od, *currd = current->domain;
> +struct domain_iommu *iommu = dom_iommu(currd);
> +const struct iommu_ops *ops = iommu->platform_ops;
> +domid_t domid = op->domid;
> +gfn_t gfn = _gfn(op->gfn);
> +bfn_t bfn = _bfn(op->bfn);
> +mfn_t mfn;
> +struct check_rdm_ctxt ctxt = {
> +.bfn = bfn,
> +};
> +p2m_type_t p2mt;
> +p2m_query_t p2mq;
> +struct page_info *page;
> +unsigned int prot;
> +int rc;
> +
> +if (op->pad0 != 0 || op->pad1 != 0)

Missing blanks again (and please again consider dropping the " != 0").

> +return -EINVAL;
> +
> +/*
> + * Both map_page and lookup_page operations must be implemented.
> + * The lookup_page method is not used here but is relied upon by
> + * iommuop_unmap() to drop the page reference taken here.
> + */
> +if ( !ops->map_page || !ops->lookup_page )
> +return -ENOSYS;

EOPNOTSUPP (also further down)

Also how about the unmap hook? If that's not implemented, how
would the page ref obtained below ever be dropped again? Or
you may need to re-order the unmap side code.

> +/* Check whether the specified BFN falls in a reserved region */
> +rc = iommu_get_reserved_device_memory(check_rdm, );
> +if ( rc )
> +return rc;
> +
> +d = rcu_lock_domain_by_any_id(domid);
> +if ( !d )
> +return -ESRCH;
> +
> +p2mq = (flags & XEN_IOMMUOP_map_readonly) ?
> +P2M_UNSHARE : P2M_ALLOC;

Isn't this the wrong way round?

> +page = get_page_from_gfn(d, gfn_x(gfn), , p2mq);
> +
> +rc = -ENOENT;
> +if ( !page )
> +goto unlock;
> +
> +if ( p2m_is_paged(p2mt) )
> +{
> +p2m_mem_paging_populate(d, gfn_x(gfn));
> +goto release;
> +}
> +
> +if ( (p2mq & P2M_UNSHARE) && p2m_is_shared(p2mt) )
> +goto release;

Same for this check then?

> +/*
> + * Make sure the page is RAM and, if it is read-only, that the
> + * read-only flag is present.
> + */
> +rc = -EPERM;
> +if ( !p2m_is_any_ram(p2mt) ||
> + (p2m_is_readonly(p2mt) && !(flags & XEN_IOMMUOP_map_readonly)) )
> +goto release;

Don't you also need to obtain a PGT_writable reference in the
"not r/o" case?

> +/*
> + * If the calling domain does not own the page then make sure it
> + * has mapping privilege over the page owner.
> + */
> +od = page_get_owner(page);
> +if ( od != currd )
> +{
> +rc = xsm_domain_memory_map(XSM_TARGET, od);
> +if ( rc )
> +goto release;
> +}

With XSM_TARGET I don't see the point of the if() around here.
Perhaps simply

rc = xsm_domain_memory_map(XSM_TARGET, page_get_owner(page));

?

> +static int iommuop_unmap(struct xen_iommu_op_unmap *op)
> +{
> +struct domain *currd = current->domain;
> +struct domain_iommu *iommu = dom_iommu(currd);
> +const struct iommu_ops *ops = iommu->platform_ops;
> +bfn_t bfn = _bfn(op->bfn);
> +mfn_t mfn;
> +struct check_rdm_ctxt ctxt = {
> +.bfn = bfn,
> +};
> +unsigned int flags;
> +struct page_info *page;
> +int rc;
> +
> +/*
> + * Both unmap_page and lookup_page operations must be implemented.
> + */

Single line comment (there are more below).

> +if ( !ops->unmap_page || !ops->lookup_page )
> +return -ENOSYS;
> +
> +/* Check whether the specified BFN falls in a 

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Paul Durrant]

> -Original Message-
> From: Tian, Kevin [mailto:kevin.t...@intel.com]
> Sent: 24 February 2018 03:02
> To: Paul Durrant ; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini ; Wei Liu
> ; George Dunlap ;
> Andrew Cooper ; Ian Jackson
> ; Tim (Xen.org) ; Jan Beulich
> 
> Subject: RE: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and
> unmap pages, and also to flush the IOTLB
[snip]
> 
> >
> > ...so my plan is that the dom0 API picks a bfn, does the mapping and then
> > passes the bfn back to the caller.
> >
> 
> A curious question. How to pass the domid from XenGT to pvIOMMU
> driver so the latter knows whether it's a local or remote mapping?
> Ideally pvIOMMU driver is registered to Linux IOMMU core layer,
> of which all existing API wrappers are only for local mapping today...
> 

It may well be that, for remote mapping, that layer is just not appropriate. 
I'll look into that when I get to that stage.

Cheers,

Paul

> Thanks
> Kevin
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Tian, Kevin]

> From: Paul Durrant [mailto:paul.durr...@citrix.com]
> Sent: Friday, February 23, 2018 5:35 PM
> 
> > -Original Message-
> > From: Tian, Kevin [mailto:kevin.t...@intel.com]
> > Sent: 23 February 2018 05:36
> > To: Paul Durrant ; xen-
> de...@lists.xenproject.org
> > Cc: Stefano Stabellini ; Wei Liu
> > ; George Dunlap ;
> > Andrew Cooper ; Ian Jackson
> > ; Tim (Xen.org) ; Jan Beulich
> > 
> > Subject: RE: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and
> > unmap pages, and also to flush the IOTLB
> >
> > > From: Paul Durrant [mailto:paul.durr...@citrix.com]
> > > Sent: Tuesday, February 13, 2018 5:56 PM
> > >
> > > > -Original Message-
> > > > From: Tian, Kevin [mailto:kevin.t...@intel.com]
> > > > Sent: 13 February 2018 06:56
> > > > To: Paul Durrant ; xen-
> > > de...@lists.xenproject.org
> > > > Cc: Stefano Stabellini ; Wei Liu
> > > > ; George Dunlap ;
> > > > Andrew Cooper ; Ian Jackson
> > > > ; Tim (Xen.org) ; Jan Beulich
> > > > 
> > > > Subject: RE: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and
> > > > unmap pages, and also to flush the IOTLB
> > > >
> > > > > From: Paul Durrant
> > > > > Sent: Monday, February 12, 2018 6:47 PM
> > > > >
> > > > > This patch adds iommu_ops to allow a domain with control_iommu
> > > > > privilege
> > > > > to map and unmap pages from any guest over which it has mapping
> > > > > privilege
> > > > > in the IOMMU.
> > > > > These operations implicitly disable IOTLB flushing so that the caller
> can
> > > > > batch operations and then explicitly flush the IOTLB using the
> > > iommu_op
> > > > > also added by this patch.
> > > >
> > > > given that last discussion is 2yrs ago and you said actual
> implementation
> > > > already biased from original spec, it'd be difficult to judge whether
> > > current
> > > > change is sufficient or just 1st step. Could you summarize what have
> > > > been changed from last spec, and also any further tasks in your TODO
> > list?
> > >
> > > Kevin,
> > >
> > > The main changes are:
> > >
> > > - there is no op to query mapping capability... instead the hypercall will
> fail
> > > with -EACCES
> > > - there is no longer an option to avoid reference counting map and
> unmap
> > > operations
> > > - there are no longer separate ops for mapping local and remote pages
> > > (DOMID_SELF should be passed to the map op for local pages), and ops
> > > always deal with GFNs not MFNs
> > >   - also I have dropped the idea of a global m2b map, so...
> > >   - it is now going to be the responsibility of the code running in the
> > > mapping domain to track what it has mapped [1]
> > > - there is no illusion that pages other 4k are supported at the moment
> > > - the flush operation is now explicit
> > >
> > > [1] this would be an issue if the interface becomes usable for anything
> > > other than dom0 as we'd also need something in Xen to release the
> page
> > > refs if the domain was forcibly destroyed, but I think the m2b was the
> > > wrong solution since it necessitates a full scan of *host* RAM on any
> > > domain destruction
> > >
> > > The main item on my TODO list is to implement a new IOREQ to allow
> > > invalidation of specific guest pages. Think of the current 'invalidate map
> > > cache' as a global flush... I need a specific flush so that a
> > > decrease_reservation hypercall issued by a guest can instead tell
> emulators
> > > exactly which pages are being removed from guest. It is then the
> > emulators'
> > > responsibilities to unmap those pages if they had them mapped (either
> > > through MMU or IOMMU) which then drop page refs and actually allow
> the
> > > pages to be recycled.
> > >
> > > I will, of course, need to come up with more Linux code to test all this,
> > > which will eventually lead to kernel and user APIs to allow emulators
> > > running in dom0 to IOMMU map guest pages.
> >
> > Thanks for elaboration. I didn't find original proposal. Can you
> > attach or point me to a link?
> >
> 
> FWIW, I've attached Malcolm's original for reference.

Thanks. I'll have a read.

> 
> > >
> > > >
> > > > at least just map/unmap operations definitely not meet XenGT
> > > > requirement...
> > > >
> > >
> > > What aspect of the hypercall interface does not meet XenGT's
> > > requirements? It would be good to know now then I can make any
> > > necessary adjustments in v2.
> > >
> >
> > XenGT needs to replace GFN with BFN into shadow GPU page table
> > for a given domain.
> 
> I assume xengt would be dynamically mapping the gfn at this point...
> 
> > Previously iirc there is a query interface for such
> > purpose, since the mapping is managed by 

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Paul Durrant]

> -Original Message-
> From: Tian, Kevin [mailto:kevin.t...@intel.com]
> Sent: 23 February 2018 05:36
> To: Paul Durrant ; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini ; Wei Liu
> ; George Dunlap ;
> Andrew Cooper ; Ian Jackson
> ; Tim (Xen.org) ; Jan Beulich
> 
> Subject: RE: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and
> unmap pages, and also to flush the IOTLB
> 
> > From: Paul Durrant [mailto:paul.durr...@citrix.com]
> > Sent: Tuesday, February 13, 2018 5:56 PM
> >
> > > -Original Message-
> > > From: Tian, Kevin [mailto:kevin.t...@intel.com]
> > > Sent: 13 February 2018 06:56
> > > To: Paul Durrant ; xen-
> > de...@lists.xenproject.org
> > > Cc: Stefano Stabellini ; Wei Liu
> > > ; George Dunlap ;
> > > Andrew Cooper ; Ian Jackson
> > > ; Tim (Xen.org) ; Jan Beulich
> > > 
> > > Subject: RE: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and
> > > unmap pages, and also to flush the IOTLB
> > >
> > > > From: Paul Durrant
> > > > Sent: Monday, February 12, 2018 6:47 PM
> > > >
> > > > This patch adds iommu_ops to allow a domain with control_iommu
> > > > privilege
> > > > to map and unmap pages from any guest over which it has mapping
> > > > privilege
> > > > in the IOMMU.
> > > > These operations implicitly disable IOTLB flushing so that the caller 
> > > > can
> > > > batch operations and then explicitly flush the IOTLB using the
> > iommu_op
> > > > also added by this patch.
> > >
> > > given that last discussion is 2yrs ago and you said actual implementation
> > > already biased from original spec, it'd be difficult to judge whether
> > current
> > > change is sufficient or just 1st step. Could you summarize what have
> > > been changed from last spec, and also any further tasks in your TODO
> list?
> >
> > Kevin,
> >
> > The main changes are:
> >
> > - there is no op to query mapping capability... instead the hypercall will 
> > fail
> > with -EACCES
> > - there is no longer an option to avoid reference counting map and unmap
> > operations
> > - there are no longer separate ops for mapping local and remote pages
> > (DOMID_SELF should be passed to the map op for local pages), and ops
> > always deal with GFNs not MFNs
> >   - also I have dropped the idea of a global m2b map, so...
> >   - it is now going to be the responsibility of the code running in the
> > mapping domain to track what it has mapped [1]
> > - there is no illusion that pages other 4k are supported at the moment
> > - the flush operation is now explicit
> >
> > [1] this would be an issue if the interface becomes usable for anything
> > other than dom0 as we'd also need something in Xen to release the page
> > refs if the domain was forcibly destroyed, but I think the m2b was the
> > wrong solution since it necessitates a full scan of *host* RAM on any
> > domain destruction
> >
> > The main item on my TODO list is to implement a new IOREQ to allow
> > invalidation of specific guest pages. Think of the current 'invalidate map
> > cache' as a global flush... I need a specific flush so that a
> > decrease_reservation hypercall issued by a guest can instead tell emulators
> > exactly which pages are being removed from guest. It is then the
> emulators'
> > responsibilities to unmap those pages if they had them mapped (either
> > through MMU or IOMMU) which then drop page refs and actually allow the
> > pages to be recycled.
> >
> > I will, of course, need to come up with more Linux code to test all this,
> > which will eventually lead to kernel and user APIs to allow emulators
> > running in dom0 to IOMMU map guest pages.
> 
> Thanks for elaboration. I didn't find original proposal. Can you
> attach or point me to a link?
> 

FWIW, I've attached Malcolm's original for reference.

> >
> > >
> > > at least just map/unmap operations definitely not meet XenGT
> > > requirement...
> > >
> >
> > What aspect of the hypercall interface does not meet XenGT's
> > requirements? It would be good to know now then I can make any
> > necessary adjustments in v2.
> >
> 
> XenGT needs to replace GFN with BFN into shadow GPU page table
> for a given domain.

I assume xengt would be dynamically mapping the gfn at this point...

> Previously iirc there is a query interface for such
> purpose, since the mapping is managed by hypervisor. Based on above
> description (e.g. m2b), did you intend to let Dom0 pvIOMMU driver
> manage all related mapping information thus GVT-g just consults
> pvIOMMU driver for such purpose?
> 

...so my plan is that the dom0 API picks a bfn, does the mapping and then 
passes the bfn back to the caller.

> Thanks
> Kevin
% Xen PV IOMMU 

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Tian, Kevin]

> From: Paul Durrant [mailto:paul.durr...@citrix.com]
> Sent: Tuesday, February 13, 2018 5:56 PM
> 
> > -Original Message-
> > From: Tian, Kevin [mailto:kevin.t...@intel.com]
> > Sent: 13 February 2018 06:56
> > To: Paul Durrant ; xen-
> de...@lists.xenproject.org
> > Cc: Stefano Stabellini ; Wei Liu
> > ; George Dunlap ;
> > Andrew Cooper ; Ian Jackson
> > ; Tim (Xen.org) ; Jan Beulich
> > 
> > Subject: RE: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and
> > unmap pages, and also to flush the IOTLB
> >
> > > From: Paul Durrant
> > > Sent: Monday, February 12, 2018 6:47 PM
> > >
> > > This patch adds iommu_ops to allow a domain with control_iommu
> > > privilege
> > > to map and unmap pages from any guest over which it has mapping
> > > privilege
> > > in the IOMMU.
> > > These operations implicitly disable IOTLB flushing so that the caller can
> > > batch operations and then explicitly flush the IOTLB using the
> iommu_op
> > > also added by this patch.
> >
> > given that last discussion is 2yrs ago and you said actual implementation
> > already biased from original spec, it'd be difficult to judge whether
> current
> > change is sufficient or just 1st step. Could you summarize what have
> > been changed from last spec, and also any further tasks in your TODO list?
> 
> Kevin,
> 
> The main changes are:
> 
> - there is no op to query mapping capability... instead the hypercall will 
> fail
> with -EACCES
> - there is no longer an option to avoid reference counting map and unmap
> operations
> - there are no longer separate ops for mapping local and remote pages
> (DOMID_SELF should be passed to the map op for local pages), and ops
> always deal with GFNs not MFNs
>   - also I have dropped the idea of a global m2b map, so...
>   - it is now going to be the responsibility of the code running in the
> mapping domain to track what it has mapped [1]
> - there is no illusion that pages other 4k are supported at the moment
> - the flush operation is now explicit
> 
> [1] this would be an issue if the interface becomes usable for anything
> other than dom0 as we'd also need something in Xen to release the page
> refs if the domain was forcibly destroyed, but I think the m2b was the
> wrong solution since it necessitates a full scan of *host* RAM on any
> domain destruction
> 
> The main item on my TODO list is to implement a new IOREQ to allow
> invalidation of specific guest pages. Think of the current 'invalidate map
> cache' as a global flush... I need a specific flush so that a
> decrease_reservation hypercall issued by a guest can instead tell emulators
> exactly which pages are being removed from guest. It is then the emulators'
> responsibilities to unmap those pages if they had them mapped (either
> through MMU or IOMMU) which then drop page refs and actually allow the
> pages to be recycled.
> 
> I will, of course, need to come up with more Linux code to test all this,
> which will eventually lead to kernel and user APIs to allow emulators
> running in dom0 to IOMMU map guest pages.

Thanks for elaboration. I didn't find original proposal. Can you 
attach or point me to a link?

> 
> >
> > at least just map/unmap operations definitely not meet XenGT
> > requirement...
> >
> 
> What aspect of the hypercall interface does not meet XenGT's
> requirements? It would be good to know now then I can make any
> necessary adjustments in v2.
> 

XenGT needs to replace GFN with BFN into shadow GPU page table
for a given domain. Previously iirc there is a query interface for such 
purpose, since the mapping is managed by hypervisor. Based on above 
description (e.g. m2b), did you intend to let Dom0 pvIOMMU driver 
manage all related mapping information thus GVT-g just consults 
pvIOMMU driver for such purpose?

Thanks
Kevin
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Paul Durrant]

> -Original Message-
> From: Tian, Kevin [mailto:kevin.t...@intel.com]
> Sent: 13 February 2018 06:56
> To: Paul Durrant ; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini ; Wei Liu
> ; George Dunlap ;
> Andrew Cooper ; Ian Jackson
> ; Tim (Xen.org) ; Jan Beulich
> 
> Subject: RE: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and
> unmap pages, and also to flush the IOTLB
> 
> > From: Paul Durrant
> > Sent: Monday, February 12, 2018 6:47 PM
> >
> > This patch adds iommu_ops to allow a domain with control_iommu
> > privilege
> > to map and unmap pages from any guest over which it has mapping
> > privilege
> > in the IOMMU.
> > These operations implicitly disable IOTLB flushing so that the caller can
> > batch operations and then explicitly flush the IOTLB using the iommu_op
> > also added by this patch.
> 
> given that last discussion is 2yrs ago and you said actual implementation
> already biased from original spec, it'd be difficult to judge whether current
> change is sufficient or just 1st step. Could you summarize what have
> been changed from last spec, and also any further tasks in your TODO list?

Kevin,

The main changes are:

- there is no op to query mapping capability... instead the hypercall will fail 
with -EACCES
- there is no longer an option to avoid reference counting map and unmap 
operations
- there are no longer separate ops for mapping local and remote pages 
(DOMID_SELF should be passed to the map op for local pages), and ops always 
deal with GFNs not MFNs
  - also I have dropped the idea of a global m2b map, so...
  - it is now going to be the responsibility of the code running in the mapping 
domain to track what it has mapped [1]
- there is no illusion that pages other 4k are supported at the moment
- the flush operation is now explicit

[1] this would be an issue if the interface becomes usable for anything other 
than dom0 as we'd also need something in Xen to release the page refs if the 
domain was forcibly destroyed, but I think the m2b was the wrong solution since 
it necessitates a full scan of *host* RAM on any domain destruction

The main item on my TODO list is to implement a new IOREQ to allow invalidation 
of specific guest pages. Think of the current 'invalidate map cache' as a 
global flush... I need a specific flush so that a decrease_reservation 
hypercall issued by a guest can instead tell emulators exactly which pages are 
being removed from guest. It is then the emulators' responsibilities to unmap 
those pages if they had them mapped (either through MMU or IOMMU) which then 
drop page refs and actually allow the pages to be recycled.

I will, of course, need to come up with more Linux code to test all this, which 
will eventually lead to kernel and user APIs to allow emulators running in dom0 
to IOMMU map guest pages.

> 
> at least just map/unmap operations definitely not meet XenGT
> requirement...
> 

What aspect of the hypercall interface does not meet XenGT's requirements? It 
would be good to know now then I can make any necessary adjustments in v2.

Cheers,

  Paul

> Thanks
> kevin
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Tian, Kevin]

> From: Paul Durrant
> Sent: Monday, February 12, 2018 6:47 PM
> 
> This patch adds iommu_ops to allow a domain with control_iommu
> privilege
> to map and unmap pages from any guest over which it has mapping
> privilege
> in the IOMMU.
> These operations implicitly disable IOTLB flushing so that the caller can
> batch operations and then explicitly flush the IOTLB using the iommu_op
> also added by this patch.

given that last discussion is 2yrs ago and you said actual implementation
already biased from original spec, it'd be difficult to judge whether current
change is sufficient or just 1st step. Could you summarize what have
been changed from last spec, and also any further tasks in your TODO list?

at least just map/unmap operations definitely not meet XenGT requirement...

Thanks
kevin
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel