From: Jan Beulich <jbeulich@suse.com>
To: "Petr Beneš" <w1benny@gmail.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
"Roger Pau Monné" <roger.pau@citrix.com>,
"George Dunlap" <george.dunlap@citrix.com>,
"Julien Grall" <julien@xen.org>,
"Stefano Stabellini" <sstabellini@kernel.org>,
"Tamas K Lengyel" <tamas@tklengyel.com>,
"Alexandru Isaila" <aisaila@bitdefender.com>,
"Petre Pircalabu" <ppircalabu@bitdefender.com>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 4/7] x86: Make the maximum number of altp2m views configurable
Date: Tue, 30 Apr 2024 16:27:44 +0200 [thread overview]
Message-ID: <5600e97e-5d44-4901-a867-b48192e9525f@suse.com> (raw)
In-Reply-To: <0041438ff7a5d5b2fad59b676f4ece80470bf3b3.1714322424.git.w1benny@gmail.com>
On 28.04.2024 18:52, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
>
> This commit introduces the ability to configure the maximum number of altp2m
> tables during domain creation. Previously, the limits were hardcoded to a
> maximum of 10. This change allows for greater flexibility in environments that
> require more or fewer altp2m views.
>
> The maximum configurable limit for max_altp2m on x86 is now set to MAX_EPTP
> (512). This cap is linked to the architectural limit of the EPTP-switching
> VMFUNC, which supports up to 512 entries. Despite there being no inherent need
> for limiting max_altp2m in scenarios not utilizing VMFUNC, decoupling these
> components would necessitate substantial code changes.
While I don't mind this connection, ...
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -685,6 +685,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> return -EINVAL;
> }
>
> + if ( config->max_altp2m > MAX_EPTP )
> + {
> + dprintk(XENLOG_INFO, "max_altp2m must be <= %u\n", MAX_EPTP);
> + return -EINVAL;
> + }
... using MAX_EPTP here feels like a layering violation to me. Imo there want
to be separate constants, tied together with a suitably placed BUILD_BUG_ON().
Furthermore comparisons like this (there are further ones elsewhere) suggest
there is a (continued) naming issue: A max_ or MAX_ prefix ought to name a
"maximum valid value", not "number of permitted values". This is not a
request to alter MAX_EPTP, but one to make sure the new struct fields really
have matching names and purposes.
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -258,11 +258,10 @@ struct paging_vcpu {
> struct shadow_vcpu shadow;
> };
>
> +#define INVALID_ALTP2M 0xffff
> +#define MAX_EPTP ((unsigned int)(PAGE_SIZE / sizeof(uint64_t)))
Aiui you add this cast because of the various min() uses. However, besides
wanting to avoid such casts (or in fact any, whenever possible), I don't
see why you need to retain all those min(): You bound d->max_altp2m against
MAX_EPTP during domain creation anyway.
> --- a/xen/arch/x86/mm/altp2m.c
> +++ b/xen/arch/x86/mm/altp2m.c
> @@ -13,6 +13,12 @@
> void
> altp2m_vcpu_initialise(struct vcpu *v)
> {
> + struct domain *d = v->domain;
> +
> + /* Skip initialisation if no altp2m will be used. */
> + if ( d->max_altp2m == 0 )
> + return;
While I'm fine with this comment, ...
> @@ -28,8 +34,13 @@ altp2m_vcpu_initialise(struct vcpu *v)
> void
> altp2m_vcpu_destroy(struct vcpu *v)
> {
> + struct domain *d = v->domain;
> struct p2m_domain *p2m;
>
> + /* Skip destruction if no altp2m was used. */
> + if ( d->max_altp2m == 0 )
> + return;
... this one doesn't look quite right: Even if altp2m-s were initialized,
none may have been used (yet).
> @@ -120,7 +131,13 @@ int p2m_init_altp2m(struct domain *d)
> struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>
> mm_lock_init(&d->arch.altp2m_list_lock);
> - for ( i = 0; i < MAX_ALTP2M; i++ )
> +
> + if ( (d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->max_altp2m)) == NULL )
> + {
> + return -ENOMEM;
> + }
Nit: Overlong line and no need for the braces.
> + for ( i = 0; i < d->max_altp2m; i++ )
> {
> d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d);
This loop, btw, also demonstrates that "maximum" isn't true here. The
domain gets all of them allocated in one go.
> @@ -141,7 +158,10 @@ void p2m_teardown_altp2m(struct domain *d)
> unsigned int i;
> struct p2m_domain *p2m;
>
> - for ( i = 0; i < MAX_ALTP2M; i++ )
> + if ( !d->arch.altp2m_p2m )
> + return;
> +
> + for ( i = 0; i < d->max_altp2m; i++ )
> {
> if ( !d->arch.altp2m_p2m[i] )
> continue;
> @@ -149,6 +169,9 @@ void p2m_teardown_altp2m(struct domain *d)
> d->arch.altp2m_p2m[i] = NULL;
> p2m_free_one(p2m);
> }
> +
> + xfree(d->arch.altp2m_p2m);
> + d->arch.altp2m_p2m = NULL;
> }
Please don't (wrongly) open-code XFREE().
> @@ -2090,13 +2090,13 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
> mfn_t mfn;
> int rc = -EINVAL;
>
> - if ( idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
> + if ( idx >= min(d->max_altp2m, MAX_EPTP) ||
Nit: Please take the opportunity and remove the excess blank.
> d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] ==
This use of MAX_EPTP also needs replacing, to avoid speculatively overrunning
the allocated array. At least one more instance elsewhere.
> @@ -2575,12 +2575,12 @@ int p2m_set_suppress_ve_multi(struct domain *d,
>
> if ( sve->view > 0 )
> {
> - if ( sve->view >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
> + if ( sve->view >= min(d->max_altp2m, MAX_EPTP) ||
> d->arch.altp2m_eptp[array_index_nospec(sve->view, MAX_EPTP)] ==
> mfn_x(INVALID_MFN) )
> return -EINVAL;
>
> - p2m = ap2m = array_access_nospec(d->arch.altp2m_p2m, sve->view);
> + p2m = ap2m = d->arch.altp2m_p2m[array_index_nospec(sve->view, d->max_altp2m)];
Nit: Overlong line (more elsewhere).
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -568,6 +568,12 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> }
> }
>
> + if ( config->max_altp2m && !hvm_altp2m_supported() )
This looks like it'll break the build on non-x86.
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -21,7 +21,7 @@
> #include "hvm/save.h"
> #include "memory.h"
>
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000016
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017
>
> /*
> * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> @@ -77,6 +77,7 @@ struct xen_domctl_createdomain {
> */
> uint32_t max_vcpus;
> uint32_t max_evtchn_port;
> + uint32_t max_altp2m;
> int32_t max_grant_frames;
> int32_t max_maptrack_frames;
Both this and ...
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -602,6 +602,8 @@ struct domain
> unsigned int guest_request_sync : 1;
> } monitor;
>
> + unsigned int max_altp2m; /* Maximum number of altp2m tables */
> +
> unsigned int vmtrace_size; /* Buffer size in bytes, or 0 to disable. */
... this suggest you're confident other architectures will also want
to support altp2m. Yet nothing like that is said in the description.
In the absence of that common code should not require touching (much).
Jan
next prev parent reply other threads:[~2024-04-30 14:28 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-28 16:52 [PATCH v2 0/7] x86: Make MAX_ALTP2M configurable Petr Beneš
2024-04-28 16:52 ` [PATCH v2 1/7] x86/p2m: Add braces for better code clarity Petr Beneš
2024-04-29 7:07 ` Jan Beulich
2024-04-29 10:26 ` Petr Beneš
2024-04-29 10:27 ` Jan Beulich
2024-04-28 16:52 ` [PATCH v2 2/7] tools/xl: Add max_altp2m parameter Petr Beneš
2024-04-28 16:52 ` [PATCH v2 3/7] docs/man: Add max_altp2m parameter to the xl.cfg manual Petr Beneš
2024-04-28 16:52 ` [PATCH v2 4/7] x86: Make the maximum number of altp2m views configurable Petr Beneš
2024-04-30 14:27 ` Jan Beulich [this message]
2024-04-30 16:00 ` Petr Beneš
2024-05-02 6:19 ` Jan Beulich
2024-04-28 16:52 ` [PATCH v2 5/7] tools/libxl: Activate the max_altp2m feature Petr Beneš
2024-04-28 16:52 ` [PATCH v2 6/7] tools/ocaml: Add max_altp2m parameter Petr Beneš
2024-04-28 16:52 ` [PATCH v2 7/7] x86/hap: Increase the number of initial mempool_size to 1024 pages Petr Beneš
2024-04-30 14:47 ` Jan Beulich
2024-04-30 15:40 ` Petr Beneš
2024-05-02 6:36 ` Jan Beulich
2024-05-02 11:59 ` Petr Beneš
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=5600e97e-5d44-4901-a867-b48192e9525f@suse.com \
--to=jbeulich@suse.com \
--cc=aisaila@bitdefender.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=julien@xen.org \
--cc=ppircalabu@bitdefender.com \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=tamas@tklengyel.com \
--cc=w1benny@gmail.com \
--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 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 read-only IMAP folder(s) and NNTP newsgroup(s).