All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Hongyan Xia <hx242@xen.org>
Cc: xen-devel@lists.xenproject.org,
	"Roger Pau Monné" <roger.pau@citrix.com>,
	julien@xen.org, "Wei Liu" <wl@xen.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v6 12/15] x86/smpboot: switch pl*e to use new APIs in clone_mapping
Date: Thu, 30 Apr 2020 17:15:38 +0200	[thread overview]
Message-ID: <88709097-661e-ce7b-1a46-1dcecf029428@suse.com> (raw)
In-Reply-To: <a1c29e58a5d40748413e8088ad88ba4319a328d4.1587735799.git.hongyxia@amazon.com>

On 24.04.2020 16:09, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@citrix.com>

Nit: Why the emphasis on pl*e in the title? Is there anything left
unconverted in the function? IOW how about "switch clone_mapping()
to new page table APIs"?

> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -672,9 +672,9 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
>  {
>      unsigned long linear = (unsigned long)ptr, pfn;
>      unsigned int flags;
> -    l3_pgentry_t *pl3e;
> -    l2_pgentry_t *pl2e;
> -    l1_pgentry_t *pl1e;
> +    l3_pgentry_t *pl3e = NULL;
> +    l2_pgentry_t *pl2e = NULL;
> +    l1_pgentry_t *pl1e = NULL;

The latter two need initializers, yes, but why pl3e?

> @@ -689,8 +689,8 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
>           (linear >= XEN_VIRT_END && linear < DIRECTMAP_VIRT_START) )
>          return -EINVAL;
>  
> -    pl3e = l4e_to_l3e(idle_pg_table[root_table_offset(linear)]) +
> -        l3_table_offset(linear);
> +    pl3e = map_l3t_from_l4e(idle_pg_table[root_table_offset(linear)]);
> +    pl3e += l3_table_offset(linear);

By keeping original style (a single assignment) you'd have slightly
less of a diff, and I think keeping original style where it's not
colliding with any of our rules is generally a good idea. Doing so
won't even make you hit the slightly flawed definition of
map_l3t_from_l4e() at al (missing outer parentheses). I notice you
do so ...

> @@ -702,7 +702,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
>      }
>      else
>      {
> -        pl2e = l3e_to_l2e(*pl3e) + l2_table_offset(linear);
> +        pl2e = map_l2t_from_l3e(*pl3e) + l2_table_offset(linear);
>          flags = l2e_get_flags(*pl2e);
>          ASSERT(flags & _PAGE_PRESENT);
>          if ( flags & _PAGE_PSE )
> @@ -713,7 +713,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
>          }
>          else
>          {
> -            pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(linear);
> +            pl1e = map_l1t_from_l2e(*pl2e) + l1_table_offset(linear);

... in both of these cases.

> @@ -724,48 +724,61 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
>          }
>      }
>  
> +    UNMAP_DOMAIN_PAGE(pl1e);
> +    UNMAP_DOMAIN_PAGE(pl2e);
> +    UNMAP_DOMAIN_PAGE(pl3e);
> +
>      if ( !(root_get_flags(rpt[root_table_offset(linear)]) & _PAGE_PRESENT) )
>      {
> -        pl3e = alloc_xen_pagetable();
> -        if ( !pl3e )
> +        mfn_t l3mfn = alloc_xen_pagetable_new();
> +
> +        if ( mfn_eq(l3mfn, INVALID_MFN) )
>              goto out;
> +
> +        pl3e = map_domain_page(l3mfn);

Seeing this recur (from other patches) I wonder whether we wouldn't
better make map_domain_page() accept INVALID_MFN and return NULL in
this case. In cases like the one here it would eliminate the need
for several local variables. Of course the downside of this is that
then we'll have to start checking map_domain_page()'s return value.
A middle ground could be to have

void *alloc_mapped_pagetable(mfn_t *mfn);

allowing to pass in NULL if the MFN is of no interest.

> @@ -781,6 +794,9 @@ static int clone_mapping(const void *ptr, root_pgentry_t *rpt)
>  
>      rc = 0;
>   out:
> +    UNMAP_DOMAIN_PAGE(pl1e);
> +    UNMAP_DOMAIN_PAGE(pl2e);
> +    UNMAP_DOMAIN_PAGE(pl3e);
>      return rc;
>  }

I don't think the writing of NULL into the variables is necessary
here. And if the needed if()-s are of concern, then perhaps we
should consider making unmap_domain_page() finally accept NULL as
input?

Jan


  reply	other threads:[~2020-04-30 15:16 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-24 14:08 [PATCH v6 00/15] switch to domheap for Xen page tables Hongyan Xia
2020-04-24 14:08 ` [PATCH v6 01/15] x86/mm: map_pages_to_xen would better have one exit path Hongyan Xia
2020-04-24 14:08 ` [PATCH v6 02/15] x86/mm: make sure there is one exit path for modify_xen_mappings Hongyan Xia
2020-04-24 14:08 ` [PATCH v6 03/15] x86/mm: rewrite virt_to_xen_l*e Hongyan Xia
2020-05-20  9:27   ` Jan Beulich
2020-04-24 14:08 ` [PATCH v6 04/15] x86/mm: switch to new APIs in map_pages_to_xen Hongyan Xia
2020-04-24 14:08 ` [PATCH v6 05/15] x86/mm: switch to new APIs in modify_xen_mappings Hongyan Xia
2020-04-24 14:08 ` [PATCH v6 06/15] x86_64/mm: introduce pl2e in paging_init Hongyan Xia
2020-05-20  9:35   ` Jan Beulich
2020-04-24 14:08 ` [PATCH v6 07/15] x86_64/mm: switch to new APIs " Hongyan Xia
2020-05-20  9:46   ` Jan Beulich
2020-05-27 15:01     ` Hongyan Xia
2020-04-24 14:08 ` [PATCH v6 08/15] x86_64/mm: switch to new APIs in setup_m2p_table Hongyan Xia
2020-05-20  9:54   ` Jan Beulich
2020-04-24 14:09 ` [PATCH v6 09/15] efi: use new page table APIs in copy_mapping Hongyan Xia
2020-04-30 12:42   ` Jan Beulich
2020-04-24 14:09 ` [PATCH v6 10/15] efi: switch to new APIs in EFI code Hongyan Xia
2020-04-30 12:55   ` Jan Beulich
2020-04-24 14:09 ` [PATCH v6 11/15] x86/smpboot: clone_mapping should have one exit path Hongyan Xia
2020-04-30 14:59   ` Jan Beulich
2020-04-24 14:09 ` [PATCH v6 12/15] x86/smpboot: switch pl*e to use new APIs in clone_mapping Hongyan Xia
2020-04-30 15:15   ` Jan Beulich [this message]
2020-05-11 10:55     ` Hongyan Xia
2020-04-24 14:09 ` [PATCH v6 13/15] x86/mm: drop old page table APIs Hongyan Xia
2020-05-20 10:09   ` Jan Beulich
2020-05-27 15:19     ` Hongyan Xia
2020-04-24 14:09 ` [PATCH v6 14/15] x86: switch to use domheap page for page tables Hongyan Xia
2020-04-24 14:09 ` [PATCH v6 15/15] x86/mm: drop _new suffix for page table APIs Hongyan Xia
2020-05-20  9:56   ` 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=88709097-661e-ce7b-1a46-1dcecf029428@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=hx242@xen.org \
    --cc=julien@xen.org \
    --cc=roger.pau@citrix.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /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.