All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
From: "Jan Beulich" <JBeulich@suse.com>
To: Malcolm Crossley <malcolm.crossley@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Yu C Zhang <yu.c.zhang@intel.com>,
	AndrewCooper <andrew.cooper3@citrix.com>,
	Paul Durrant <Paul.Durrant@citrix.com>,
	David Vrabel <david.vrabel@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Zhiyuan Lv <zhiyuan.lv@intel.com>
Subject: Re: [RFC] Xen PV IOMMU interface draft B
Date: Tue, 16 Jun 2015 16:56:30 +0100	[thread overview]
Message-ID: <5580634F0200007800085AAD@mail.emea.novell.com> (raw)
In-Reply-To: <558036E9.9000004@citrix.com>

>>> On 16.06.15 at 16:47, <malcolm.crossley@citrix.com> wrote:
> On 16/06/15 14:19, Jan Beulich wrote:
>>>>> On 12.06.15 at 18:43, <malcolm.crossley@citrix.com> wrote:
>>> IOMMU_QUERY_map_all_gfns     1        IOMMUOP_map_page subop can map any MFN
>>>                                       not used by Xen
>> 
>> "gfns" or "MFN"?
> 
> gfns . This is meant to apply to the hardware domain only, it's to allow the
> same access control as dom0-relaxed mode allows currently.

But why "gfns" in the name and "any MFN" in the description?

>>> IOMMUOP_unmap_page
>>> ------------------
>>> This subop uses `struct unmap_page` part of the `struct pv_iommu_op`.
>>>
>>> The subop usage of the "struct pv_iommu_op" and ``struct unmap_page` fields
>>> are detailed below:
>>>
>>> --------------------------------------------------------------------
>>> Field          Purpose
>>> -----          -----------------------------------------------------
>>> `bfn`          [in] Bus address frame number to be unmapped in DOMID_SELF
>> 
>> Has it been determined that unmapping based on GFN is never
>> going to be needed, and that unmapping by BFN is the more
>> practical solution? The map counterpart doesn't seem to exclude
>> establishing multiple mappings for the same BFN, and hence the
>> inverse here would become kind of fuzzy in that case.
> 
> There will be only one BFN to MFN mapping per domain, the map hypercall will
> fail any attempt to have map a BFN to more than one GFN. This is why the 
> unmap
> is based on the BFN. It is allowed to have multiple BFN mappings of the same
> GFN however.

Okay, I got confused again by the term BFN - I keep mixing up the
parts of the bus between device and IOMMU vs between IOMMU
and RAM. Alternatives I could think of (DFN for Device Frame Number)
wouldn't be any better, so I guess we need to live with the ambiguity.

>>> Each successful subop will add to the M2B if there was not an existing 
>>> identical
>>> M2B entry.
>>>
>>> Every new M2B entry will take a reference to the MFN backing the GFN.
>> 
>> This is a lookup - why would it add something somewhere? Perhaps
>> this is just a copy-and-paste mistake? Or is the use of "lookup" here
>> misleading (as the last section of the document seems to suggest)?
> 
> I can see how the term "lookup" is misleading. This subop really does a
> lookup and take reference. Can you suggest an alternative name?

get_foreign_page_map? In any event, in particular with possibly
ambiguous name the description should be particularly clear and
obvious to help the reader (which also applies to the code to be
written).

> I'm wary of allowing

???

>>> PV IOMMU interactions with self ballooning
>>> ==========================================
>>>
>>> The guest should clear any IOMMU mappings it has of it's own pages before
>>> releasing a page back to Xen. It will need to add IOMMU mappings after
>>> repopulating a page with the populate_physmap hypercall.
>>>
>>> This requires that IOMMU mappings get a writeable page type reference count 
>>> and
>>> that guests clear any IOMMU mappings before pinning page table pages.
>> 
>> I suppose this is only for aware PV guests. If so, perhaps this should
>> be made explicit.
> 
> This is only for PV guests. I will make the correction.

The emphasis was on "aware", not on "PV" (which is already stated).

>>> The grant map operation would then behave similarly to the IOMMUOP_map_page
>>> subop for the creation of the IOMMU mapping.
>>>
>>> The grant unmap operation would then behave similarly to the 
>>> IOMMUOP_unmap_page
>>> subop for the removal of the IOMMU mapping.
>> 
>> We're talking about mappings of foreign pages here - aren't these the
>> wrong IOMMUOPs then? And if so, where would the ioserver id come
>> from?
>> 
> 
> I don't expected grant mapped pages to be ballooned out or to be directly 
> used by
> ioservers so I believe the grant mapped pages match more closely to the 
> standard
> map_page than the foreign_map_page.

Right, that became clear with you saying that the ioserver id is
meant to be used for balloon out notifications only. But that
should be made explicit.

> Generally I think I need to rework the document to introduce the some 
> concepts
> before the actual interface itself.

Yes, that would be very helpful. The interface spec should probably
the (almost) last thing.

Jan

  reply	other threads:[~2015-06-16 15:56 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-12 16:43 [RFC] Xen PV IOMMU interface draft B Malcolm Crossley
2015-06-16 13:19 ` Jan Beulich
2015-06-16 14:47   ` Malcolm Crossley
2015-06-16 15:56     ` Jan Beulich [this message]
2015-06-17 12:48 ` Yu, Zhang
2015-06-17 13:34   ` Jan Beulich
2015-06-17 13:44   ` Malcolm Crossley
2015-06-26 10:23 ` Xen PV IOMMU interface draft C Malcolm Crossley
2015-06-26 11:03   ` Ian Campbell
2015-06-29 14:40     ` Konrad Rzeszutek Wilk
2015-06-29 14:52       ` Ian Campbell
2015-06-29 15:05         ` Malcolm Crossley
2015-06-29 15:24         ` David Vrabel
2015-06-29 15:36           ` Ian Campbell
2015-07-10 19:32   ` Konrad Rzeszutek Wilk
2016-02-10 10:09   ` Xen PV IOMMU interface draft D Malcolm Crossley
2016-02-18  8:21     ` Tian, Kevin
2016-02-23 16:17     ` Jan Beulich
2016-02-23 16:22       ` Malcolm Crossley
2016-03-02  6:54     ` Tian, Kevin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5580634F0200007800085AAD@mail.emea.novell.com \
    --to=jbeulich@suse.com \
    --cc=Paul.Durrant@citrix.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=david.vrabel@citrix.com \
    --cc=kevin.tian@intel.com \
    --cc=malcolm.crossley@citrix.com \
    --cc=xen-devel@lists.xenproject.org \
    --cc=yu.c.zhang@intel.com \
    --cc=zhiyuan.lv@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.