xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Tim Deegan <tim@xen.org>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Kevin Tian" <kevin.tian@intel.com>,
	"Jun Nakajima" <jun.nakajima@intel.com>
Subject: Re: [PATCH v4] VMX: use a single, global APIC access page
Date: Thu, 22 Apr 2021 11:38:42 +0200	[thread overview]
Message-ID: <9a00493c-319f-58c6-853c-382dd7a95561@suse.com> (raw)
In-Reply-To: <YIEo3DQojgc0zlzw@deinos.phlegethon.org>

On 22.04.2021 09:42, Tim Deegan wrote:
> At 13:25 +0200 on 19 Apr (1618838726), Jan Beulich wrote:
>> On 17.04.2021 21:24, Tim Deegan wrote:
>>> At 12:40 +0200 on 12 Apr (1618231248), Jan Beulich wrote:
>>>> --- a/xen/arch/x86/mm/shadow/set.c
>>>> +++ b/xen/arch/x86/mm/shadow/set.c
>>>> @@ -94,6 +94,22 @@ shadow_get_page_from_l1e(shadow_l1e_t sl
>>>>      ASSERT(!sh_l1e_is_magic(sl1e));
>>>>      ASSERT(shadow_mode_refcounts(d));
>>>>  
>>>> +    /*
>>>> +     * VMX'es APIC access MFN is just a surrogate page.  It doesn't actually
>>>> +     * get accessed, and hence there's no need to refcount it (and refcounting
>>>> +     * would fail, due to the page having no owner).
>>>> +     */
>>>> +    if ( mfn_valid(mfn = shadow_l1e_get_mfn(sl1e)) )
>>>
>>> Would it be better to check specifically for mfn == apic_access_mfn
>>> (and apic_access_mfn != 0, I guess)?
>>
>> Roger did ask about the same - I neither want to expose apic_access_mfn
>> outside its CU, nor do I want to introduce an accessor function. Both
>> feel like layering violations to me.
> 
> I think that this is even more of a layering violation: what we
> actually want is to allow un-refcounted mappings of the
> apic_access_mfn, but to do it we're relying on an internal
> implementation detail (that it happens to be un-owned and PGC_extra)
> rather than giving ourselves an API.
> 
> And so we're tangled up talking about how to write comments to warn
> our future selves about the possible side-effects.
> 
>>>  If we want this behaviour for
>>> for all un-owned PGC_extra MFNs it would be good to explain that in the
>>> comments.
>>
>> This is hard to tell without knowing which (or even if) further such
>> PGC_extra pages will appear. Hence any comment to that effect would be
>> guesswork at best. Of course I can add e.g. "Other pages with the same
>> properties would be treated the same", if that's what you're after?
> 
> If you want to go this way there should be a comment here saying that
> we're allowing this for all PGC_extra pages because we need it for
> apic_access_mfn, and a comment at PGC_extra saying that it has this
> effect.

So (along with a comment to this effect) how about I make
page_suppress_refcounting() and page_refcounting_suppressed() helpers?
The former would set PGC_extra on the page and assert the page has no
owner, while the latter would subsume the checks done here. The only
question then is what to do with the ASSERT(type == p2m_mmio_direct):
That's still a property of the APIC access MFN which may or may not
hold for future such pages. (It can't be part of the new helper anyway
as "put" doesn't have the type available.)

Jan


  reply	other threads:[~2021-04-22  9:38 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-22 10:55 [PATCH v3 0/2] VMX: apic access page handling adjustments Jan Beulich
2021-02-22 10:56 ` [PATCH v3 1/2][4.15] VMX: delay p2m insertion of APIC access page Jan Beulich
2021-02-22 11:25   ` Ian Jackson
2021-02-22 14:05     ` Jan Beulich
2021-02-22 17:17       ` Ian Jackson
2021-02-22 12:15   ` Roger Pau Monné
2021-02-25  8:44   ` Jan Beulich
2021-02-26  7:06     ` Tian, Kevin
2021-02-22 10:57 ` [PATCH v3 2/2] VMX: use a single, global " Jan Beulich
2021-03-01  2:34   ` Tian, Kevin
2021-03-01  8:18     ` Jan Beulich
2021-04-12 10:40 ` [PATCH v4] " Jan Beulich
2021-04-12 15:31   ` Roger Pau Monné
2021-04-13  9:24     ` Jan Beulich
2021-04-13 10:18       ` Roger Pau Monné
2021-04-13 12:03         ` Jan Beulich
2021-04-13 13:03           ` Roger Pau Monné
2021-04-17 19:24   ` Tim Deegan
2021-04-19 11:25     ` Jan Beulich
2021-04-22  7:42       ` Tim Deegan
2021-04-22  9:38         ` Jan Beulich [this message]
2021-04-22 15:05           ` Tim Deegan
2021-04-23 10:51 ` [PATCH v4 0/3] VMX APIC access page and shadow adjustments Jan Beulich
2021-04-23 10:52   ` [PATCH v4 1/3] VMX: use a single, global APIC access page Jan Beulich
2021-04-23 14:17     ` Roger Pau Monné
2021-04-23 14:42       ` Jan Beulich
2021-04-26 17:55         ` Tim Deegan
2021-04-25  1:27     ` Tian, Kevin
2021-04-26 17:53     ` Tim Deegan
2021-04-23 10:53   ` [PATCH v4 2/3] x86/shadow: re-use variables in shadow_get_page_from_l1e() Jan Beulich
2021-04-23 10:54   ` [PATCH v4 3/3] x86/shadow: streamline shadow_get_page_from_l1e() Jan Beulich
2021-04-23 11:00   ` Really v5 (was: [PATCH v4 0/3] VMX APIC access page and shadow adjustments) Jan Beulich

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=9a00493c-319f-58c6-853c-382dd7a95561@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=george.dunlap@citrix.com \
    --cc=jun.nakajima@intel.com \
    --cc=kevin.tian@intel.com \
    --cc=roger.pau@citrix.com \
    --cc=tim@xen.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH v4] VMX: use a single, global APIC access page' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).