| Message ID | 84794f97bc738add96a66790425a3aa5f5084a25.1717356829.git.w1benny@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Make MAX_ALTP2M configurable | expand |
Hi Petr, On 02/06/2024 21:04, Petr Beneš wrote: > From: Petr Beneš <w1benny@gmail.com> > > x86: Make the maximum number of altp2m views configurable > > This commit introduces the ability to configure the maximum number of altp2m > views for the domain during its 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 nr_altp2m on x86 is now set to > MAX_NR_ALTP2M (which currently holds the MAX_EPTP value - 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 nr_altp2m > in scenarios not utilizing VMFUNC, decoupling these components would necessitate > substantial code changes. > > xen_domctl_createdomain::altp2m is extended for a new field `nr`, that will > configure this limit for a domain. Additionally, previous altp2m.opts value > has been reduced from uint32_t to uint16_t so that both of these fields occupy > as little space as possible. > > altp2m_get_p2m() function is modified to respect the new nr_altp2m value. > Accessor functions that operate on EPT arrays are unmodified, since these > arrays always have fixed size of MAX_EPTP. > > A dummy hvm_altp2m_supported() function is introduced for non-HVM builds, so > that the compilation won't fail for them. > > Additional sanitization is introduced in the x86/arch_sanitise_domain_config > to fix the altp2m.nr value to 10 if it is previously set to 0. This behavior > is only temporary and immediately removed in the upcoming commit (which will > disallow creating a domain with enabled altp2m with zero nr_altp2m). > > The reason for this temporary workaround is to retain the legacy behavior > until the feature is fully activated in libxl. > > Also, arm/arch_sanitise_domain_config is extended to not allow requesting > non-zero altp2ms. > > Signed-off-by: Petr Beneš <w1benny@gmail.com> For the small change in Arm: Acked-by: Julien Grall <jgrall@amazon.com> # arm Cheers,
On 02.06.2024 22:04, Petr Beneš wrote: > @@ -5245,7 +5251,7 @@ void hvm_fast_singlestep(struct vcpu *v, uint16_t p2midx) > if ( !hvm_is_singlestep_supported() ) > return; > > - if ( p2midx >= MAX_ALTP2M ) > + if ( p2midx >= v->domain->nr_altp2m ) > return; As (iirc) indicated before, just like you don't add a d local variable here or ... > --- a/xen/arch/x86/include/asm/p2m.h > +++ b/xen/arch/x86/include/asm/p2m.h > @@ -887,7 +887,7 @@ static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v) > if ( index == INVALID_ALTP2M ) > return NULL; > > - BUG_ON(index >= MAX_ALTP2M); > + BUG_ON(index >= v->domain->nr_altp2m); > > return altp2m_get_p2m(v->domain, index); > } > @@ -898,7 +898,7 @@ static inline bool p2m_set_altp2m(struct vcpu *v, unsigned int idx) > struct p2m_domain *orig; > struct p2m_domain *ap2m; > > - BUG_ON(idx >= MAX_ALTP2M); > + BUG_ON(idx >= v->domain->nr_altp2m); > > if ( idx == vcpu_altp2m(v).p2midx ) > return false; ... in either of these, I see little reason to have such ... > --- a/xen/arch/x86/mm/altp2m.c > +++ b/xen/arch/x86/mm/altp2m.c > @@ -15,6 +15,11 @@ > void > altp2m_vcpu_initialise(struct vcpu *v) > { > + struct domain *d = v->domain; > + > + if ( d->nr_altp2m == 0 ) > + return; > + > if ( v != current ) > vcpu_pause(v); > > @@ -30,8 +35,12 @@ altp2m_vcpu_initialise(struct vcpu *v) > void > altp2m_vcpu_destroy(struct vcpu *v) > { > + struct domain *d = v->domain; > struct p2m_domain *p2m; > > + if ( d->nr_altp2m == 0 ) > + return; > + > if ( v != current ) > vcpu_pause(v); ... in both of these. > @@ -122,7 +131,12 @@ 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++ ) > + d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m); > + > + if ( !d->arch.altp2m_p2m ) > + return -ENOMEM; This isn't really needed, is it? Both ... > + for ( i = 0; i < d->nr_altp2m; i++ ) ... this and ... > { > d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d); > if ( p2m == NULL ) > @@ -143,7 +157,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->nr_altp2m; i++ ) > { > if ( !d->arch.altp2m_p2m[i] ) > continue; > @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d) > d->arch.altp2m_p2m[i] = NULL; > p2m_free_one(p2m); > } > + > + XFREE(d->arch.altp2m_p2m); > } ... this ought to be fine without? > @@ -538,8 +538,8 @@ void hap_final_teardown(struct domain *d) > unsigned int i; > > if ( hvm_altp2m_supported() ) > - for ( i = 0; i < MAX_ALTP2M; i++ ) > - p2m_teardown(d->arch.altp2m_p2m[i], true, NULL); > + for ( i = 0; i < d->nr_altp2m; i++ ) > + p2m_teardown(altp2m_get_p2m(d, i), true, NULL); Shouldn't the switch to altp2m_get_p2m() be part of the respective earlier patch? Jan
On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote: > > @@ -122,7 +131,12 @@ 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++ ) > > + d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m); > > + > > + if ( !d->arch.altp2m_p2m ) > > + return -ENOMEM; > > This isn't really needed, is it? Both ... > > > + for ( i = 0; i < d->nr_altp2m; i++ ) > > ... this and ... > > > { > > d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d); > > if ( p2m == NULL ) > > @@ -143,7 +157,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->nr_altp2m; i++ ) > > { > > if ( !d->arch.altp2m_p2m[i] ) > > continue; > > @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d) > > d->arch.altp2m_p2m[i] = NULL; > > p2m_free_one(p2m); > > } > > + > > + XFREE(d->arch.altp2m_p2m); > > } > > ... this ought to be fine without? Could you, please, elaborate? I honestly don't know what you mean here (by "this isn't needed"). P.
On 09.06.2024 01:06, Petr Beneš wrote: > On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote: >>> @@ -122,7 +131,12 @@ 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++ ) >>> + d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m); >>> + >>> + if ( !d->arch.altp2m_p2m ) >>> + return -ENOMEM; >> >> This isn't really needed, is it? Both ... >> >>> + for ( i = 0; i < d->nr_altp2m; i++ ) >> >> ... this and ... >> >>> { >>> d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d); >>> if ( p2m == NULL ) >>> @@ -143,7 +157,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; I'm sorry, the question was meant to be on this if() instead. >>> + for ( i = 0; i < d->nr_altp2m; i++ ) >>> { >>> if ( !d->arch.altp2m_p2m[i] ) >>> continue; >>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d) >>> d->arch.altp2m_p2m[i] = NULL; >>> p2m_free_one(p2m); >>> } >>> + >>> + XFREE(d->arch.altp2m_p2m); >>> } >> >> ... this ought to be fine without? > > Could you, please, elaborate? I honestly don't know what you mean here > (by "this isn't needed"). I hope the above correction is enough? Jan
On Mon, Jun 10, 2024 at 9:30 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 09.06.2024 01:06, Petr Beneš wrote: > > On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote: > >>> @@ -122,7 +131,12 @@ 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++ ) > >>> + d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m); > >>> + > >>> + if ( !d->arch.altp2m_p2m ) > >>> + return -ENOMEM; > >> > >> This isn't really needed, is it? Both ... > >> > >>> + for ( i = 0; i < d->nr_altp2m; i++ ) > >> > >> ... this and ... > >> > >>> { > >>> d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d); > >>> if ( p2m == NULL ) > >>> @@ -143,7 +157,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; > > I'm sorry, the question was meant to be on this if() instead. > > >>> + for ( i = 0; i < d->nr_altp2m; i++ ) > >>> { > >>> if ( !d->arch.altp2m_p2m[i] ) > >>> continue; > >>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d) > >>> d->arch.altp2m_p2m[i] = NULL; > >>> p2m_free_one(p2m); > >>> } > >>> + > >>> + XFREE(d->arch.altp2m_p2m); > >>> } > >> > >> ... this ought to be fine without? > > > > Could you, please, elaborate? I honestly don't know what you mean here > > (by "this isn't needed"). > > I hope the above correction is enough? I'm sorry, but not really? I feel like I'm blind but I can't see anything I could remove without causing (or risking) crash. P.
On 10.06.2024 11:10, Petr Beneš wrote: > On Mon, Jun 10, 2024 at 9:30 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 09.06.2024 01:06, Petr Beneš wrote: >>> On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote: >>>>> @@ -122,7 +131,12 @@ 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++ ) >>>>> + d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m); >>>>> + >>>>> + if ( !d->arch.altp2m_p2m ) >>>>> + return -ENOMEM; >>>> >>>> This isn't really needed, is it? Both ... >>>> >>>>> + for ( i = 0; i < d->nr_altp2m; i++ ) >>>> >>>> ... this and ... >>>> >>>>> { >>>>> d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d); >>>>> if ( p2m == NULL ) >>>>> @@ -143,7 +157,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; >> >> I'm sorry, the question was meant to be on this if() instead. >> >>>>> + for ( i = 0; i < d->nr_altp2m; i++ ) >>>>> { >>>>> if ( !d->arch.altp2m_p2m[i] ) >>>>> continue; >>>>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d) >>>>> d->arch.altp2m_p2m[i] = NULL; >>>>> p2m_free_one(p2m); >>>>> } >>>>> + >>>>> + XFREE(d->arch.altp2m_p2m); >>>>> } >>>> >>>> ... this ought to be fine without? >>> >>> Could you, please, elaborate? I honestly don't know what you mean here >>> (by "this isn't needed"). >> >> I hope the above correction is enough? > > I'm sorry, but not really? I feel like I'm blind but I can't see > anything I could remove without causing (or risking) crash. The loop is going to do nothing when d->nr_altp2m == 0, and the XFREE() is going to do nothing when d->arch.altp2m_p2m == NULL. Hence what does the if() guard against? IOW what possible crashes are you seeing that I don't see? Jan
On Mon, Jun 10, 2024 at 12:16 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 10.06.2024 11:10, Petr Beneš wrote: > > On Mon, Jun 10, 2024 at 9:30 AM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 09.06.2024 01:06, Petr Beneš wrote: > >>> On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote: > >>>>> @@ -122,7 +131,12 @@ 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++ ) > >>>>> + d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m); > >>>>> + > >>>>> + if ( !d->arch.altp2m_p2m ) > >>>>> + return -ENOMEM; > >>>> > >>>> This isn't really needed, is it? Both ... > >>>> > >>>>> + for ( i = 0; i < d->nr_altp2m; i++ ) > >>>> > >>>> ... this and ... > >>>> > >>>>> { > >>>>> d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d); > >>>>> if ( p2m == NULL ) > >>>>> @@ -143,7 +157,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; > >> > >> I'm sorry, the question was meant to be on this if() instead. > >> > >>>>> + for ( i = 0; i < d->nr_altp2m; i++ ) > >>>>> { > >>>>> if ( !d->arch.altp2m_p2m[i] ) > >>>>> continue; > >>>>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d) > >>>>> d->arch.altp2m_p2m[i] = NULL; > >>>>> p2m_free_one(p2m); > >>>>> } > >>>>> + > >>>>> + XFREE(d->arch.altp2m_p2m); > >>>>> } > >>>> > >>>> ... this ought to be fine without? > >>> > >>> Could you, please, elaborate? I honestly don't know what you mean here > >>> (by "this isn't needed"). > >> > >> I hope the above correction is enough? > > > > I'm sorry, but not really? I feel like I'm blind but I can't see > > anything I could remove without causing (or risking) crash. > > The loop is going to do nothing when d->nr_altp2m == 0, and the XFREE() is > going to do nothing when d->arch.altp2m_p2m == NULL. Hence what does the > if() guard against? IOW what possible crashes are you seeing that I don't > see? I see now. I was seeing ghosts - my thinking was that if p2m_init_altp2m fails to allocate altp2m_p2m, it will call p2m_teardown_altp2m - which, without the if(), would start to iterate through an array that is not allocated. It doesn't happen, it just returns -ENOMEM. So to reiterate: if ( !d->arch.altp2m_p2m ) return; ... are we talking that this condition inside p2m_teardown_altp2m isn't needed? Or is there anything else? P.
On 10.06.2024 12:34, Petr Beneš wrote: > On Mon, Jun 10, 2024 at 12:16 PM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 10.06.2024 11:10, Petr Beneš wrote: >>> On Mon, Jun 10, 2024 at 9:30 AM Jan Beulich <jbeulich@suse.com> wrote: >>>> >>>> On 09.06.2024 01:06, Petr Beneš wrote: >>>>> On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote: >>>>>>> @@ -122,7 +131,12 @@ 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++ ) >>>>>>> + d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m); >>>>>>> + >>>>>>> + if ( !d->arch.altp2m_p2m ) >>>>>>> + return -ENOMEM; >>>>>> >>>>>> This isn't really needed, is it? Both ... >>>>>> >>>>>>> + for ( i = 0; i < d->nr_altp2m; i++ ) >>>>>> >>>>>> ... this and ... >>>>>> >>>>>>> { >>>>>>> d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d); >>>>>>> if ( p2m == NULL ) >>>>>>> @@ -143,7 +157,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; >>>> >>>> I'm sorry, the question was meant to be on this if() instead. >>>> >>>>>>> + for ( i = 0; i < d->nr_altp2m; i++ ) >>>>>>> { >>>>>>> if ( !d->arch.altp2m_p2m[i] ) >>>>>>> continue; >>>>>>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d) >>>>>>> d->arch.altp2m_p2m[i] = NULL; >>>>>>> p2m_free_one(p2m); >>>>>>> } >>>>>>> + >>>>>>> + XFREE(d->arch.altp2m_p2m); >>>>>>> } >>>>>> >>>>>> ... this ought to be fine without? >>>>> >>>>> Could you, please, elaborate? I honestly don't know what you mean here >>>>> (by "this isn't needed"). >>>> >>>> I hope the above correction is enough? >>> >>> I'm sorry, but not really? I feel like I'm blind but I can't see >>> anything I could remove without causing (or risking) crash. >> >> The loop is going to do nothing when d->nr_altp2m == 0, and the XFREE() is >> going to do nothing when d->arch.altp2m_p2m == NULL. Hence what does the >> if() guard against? IOW what possible crashes are you seeing that I don't >> see? > > I see now. I was seeing ghosts - my thinking was that if > p2m_init_altp2m fails to allocate altp2m_p2m, it will call > p2m_teardown_altp2m - which, without the if(), would start to iterate > through an array that is not allocated. It doesn't happen, it just > returns -ENOMEM. > > So to reiterate: > > if ( !d->arch.altp2m_p2m ) > return; > > ... are we talking that this condition inside p2m_teardown_altp2m > isn't needed? I'm not sure about "isn't" vs "shouldn't". The call from p2m_final_teardown() also needs to remain safe to make. Which may require that upon allocation failure you zap d->nr_altp2m. Or which alternatively may mean that the if() actually needs to stay. > Or is there anything else? There was also the question of whether to guard the allocation, to avoid a de-generate xmalloc_array() of zero size. Yet in the interest of avoiding not strictly necessary conditionals, that may well want to remain as is. Jan
On Mon, Jun 10, 2024 at 1:21 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 10.06.2024 12:34, Petr Beneš wrote: > > On Mon, Jun 10, 2024 at 12:16 PM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 10.06.2024 11:10, Petr Beneš wrote: > >>> On Mon, Jun 10, 2024 at 9:30 AM Jan Beulich <jbeulich@suse.com> wrote: > >>>> > >>>> On 09.06.2024 01:06, Petr Beneš wrote: > >>>>> On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote: > >>>>>>> @@ -122,7 +131,12 @@ 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++ ) > >>>>>>> + d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m); > >>>>>>> + > >>>>>>> + if ( !d->arch.altp2m_p2m ) > >>>>>>> + return -ENOMEM; > >>>>>> > >>>>>> This isn't really needed, is it? Both ... > >>>>>> > >>>>>>> + for ( i = 0; i < d->nr_altp2m; i++ ) > >>>>>> > >>>>>> ... this and ... > >>>>>> > >>>>>>> { > >>>>>>> d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d); > >>>>>>> if ( p2m == NULL ) > >>>>>>> @@ -143,7 +157,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; > >>>> > >>>> I'm sorry, the question was meant to be on this if() instead. > >>>> > >>>>>>> + for ( i = 0; i < d->nr_altp2m; i++ ) > >>>>>>> { > >>>>>>> if ( !d->arch.altp2m_p2m[i] ) > >>>>>>> continue; > >>>>>>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d) > >>>>>>> d->arch.altp2m_p2m[i] = NULL; > >>>>>>> p2m_free_one(p2m); > >>>>>>> } > >>>>>>> + > >>>>>>> + XFREE(d->arch.altp2m_p2m); > >>>>>>> } > >>>>>> > >>>>>> ... this ought to be fine without? > >>>>> > >>>>> Could you, please, elaborate? I honestly don't know what you mean here > >>>>> (by "this isn't needed"). > >>>> > >>>> I hope the above correction is enough? > >>> > >>> I'm sorry, but not really? I feel like I'm blind but I can't see > >>> anything I could remove without causing (or risking) crash. > >> > >> The loop is going to do nothing when d->nr_altp2m == 0, and the XFREE() is > >> going to do nothing when d->arch.altp2m_p2m == NULL. Hence what does the > >> if() guard against? IOW what possible crashes are you seeing that I don't > >> see? > > > > I see now. I was seeing ghosts - my thinking was that if > > p2m_init_altp2m fails to allocate altp2m_p2m, it will call > > p2m_teardown_altp2m - which, without the if(), would start to iterate > > through an array that is not allocated. It doesn't happen, it just > > returns -ENOMEM. > > > > So to reiterate: > > > > if ( !d->arch.altp2m_p2m ) > > return; > > > > ... are we talking that this condition inside p2m_teardown_altp2m > > isn't needed? > > I'm not sure about "isn't" vs "shouldn't". The call from p2m_final_teardown() > also needs to remain safe to make. Which may require that upon allocation > failure you zap d->nr_altp2m. Or which alternatively may mean that the if() > actually needs to stay. True, p2m_final_teardown is called whenever p2m_init (and by extension p2m_init_altp2m) fails. Which means that condition must stay - or, as you suggested, reset nr_altp2m to 0. I would rather leave the code as is. Modifying nr_altp2m would (in my opinion) feel semantically incorrect, as that value should behave more or less as const, that is initialized once. > > Or is there anything else? > > There was also the question of whether to guard the allocation, to avoid a > de-generate xmalloc_array() of zero size. Yet in the interest of avoiding > not strictly necessary conditionals, that may well want to remain as is. True, nr_altp2m would mean zero-sized allocation, as p2m_init_altp2m is called unconditionally (when booted with altp2m=1). Is it a problem, though? P.
> (when booted with altp2m=1)
Sorry, forgot to remove this statement before sending the email, it's
not really true.
I wanted to add that as I wrote in a previous email exchange - altp2m
should be ideally initialized only when it's requested - as opposed to
the current situation, when it's initialized in the domain_create.
However, that is more suited for 4.20.
P.
On 10.06.2024 14:21, Petr Beneš wrote: > On Mon, Jun 10, 2024 at 1:21 PM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 10.06.2024 12:34, Petr Beneš wrote: >>> On Mon, Jun 10, 2024 at 12:16 PM Jan Beulich <jbeulich@suse.com> wrote: >>>> >>>> On 10.06.2024 11:10, Petr Beneš wrote: >>>>> On Mon, Jun 10, 2024 at 9:30 AM Jan Beulich <jbeulich@suse.com> wrote: >>>>>> >>>>>> On 09.06.2024 01:06, Petr Beneš wrote: >>>>>>> On Thu, Jun 6, 2024 at 9:24 AM Jan Beulich <jbeulich@suse.com> wrote: >>>>>>>>> @@ -122,7 +131,12 @@ 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++ ) >>>>>>>>> + d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m); >>>>>>>>> + >>>>>>>>> + if ( !d->arch.altp2m_p2m ) >>>>>>>>> + return -ENOMEM; >>>>>>>> >>>>>>>> This isn't really needed, is it? Both ... >>>>>>>> >>>>>>>>> + for ( i = 0; i < d->nr_altp2m; i++ ) >>>>>>>> >>>>>>>> ... this and ... >>>>>>>> >>>>>>>>> { >>>>>>>>> d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d); >>>>>>>>> if ( p2m == NULL ) >>>>>>>>> @@ -143,7 +157,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; >>>>>> >>>>>> I'm sorry, the question was meant to be on this if() instead. >>>>>> >>>>>>>>> + for ( i = 0; i < d->nr_altp2m; i++ ) >>>>>>>>> { >>>>>>>>> if ( !d->arch.altp2m_p2m[i] ) >>>>>>>>> continue; >>>>>>>>> @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d) >>>>>>>>> d->arch.altp2m_p2m[i] = NULL; >>>>>>>>> p2m_free_one(p2m); >>>>>>>>> } >>>>>>>>> + >>>>>>>>> + XFREE(d->arch.altp2m_p2m); >>>>>>>>> } >>>>>>>> >>>>>>>> ... this ought to be fine without? >>>>>>> >>>>>>> Could you, please, elaborate? I honestly don't know what you mean here >>>>>>> (by "this isn't needed"). >>>>>> >>>>>> I hope the above correction is enough? >>>>> >>>>> I'm sorry, but not really? I feel like I'm blind but I can't see >>>>> anything I could remove without causing (or risking) crash. >>>> >>>> The loop is going to do nothing when d->nr_altp2m == 0, and the XFREE() is >>>> going to do nothing when d->arch.altp2m_p2m == NULL. Hence what does the >>>> if() guard against? IOW what possible crashes are you seeing that I don't >>>> see? >>> >>> I see now. I was seeing ghosts - my thinking was that if >>> p2m_init_altp2m fails to allocate altp2m_p2m, it will call >>> p2m_teardown_altp2m - which, without the if(), would start to iterate >>> through an array that is not allocated. It doesn't happen, it just >>> returns -ENOMEM. >>> >>> So to reiterate: >>> >>> if ( !d->arch.altp2m_p2m ) >>> return; >>> >>> ... are we talking that this condition inside p2m_teardown_altp2m >>> isn't needed? >> >> I'm not sure about "isn't" vs "shouldn't". The call from p2m_final_teardown() >> also needs to remain safe to make. Which may require that upon allocation >> failure you zap d->nr_altp2m. Or which alternatively may mean that the if() >> actually needs to stay. > > True, p2m_final_teardown is called whenever p2m_init (and by extension > p2m_init_altp2m) fails. Which means that condition must stay - or, as > you suggested, reset nr_altp2m to 0. > > I would rather leave the code as is. Modifying nr_altp2m would (in my > opinion) feel semantically incorrect, as that value should behave more > or less as const, that is initialized once. > >>> Or is there anything else? >> >> There was also the question of whether to guard the allocation, to avoid a >> de-generate xmalloc_array() of zero size. Yet in the interest of avoiding >> not strictly necessary conditionals, that may well want to remain as is. > > True, nr_altp2m would mean zero-sized allocation, as p2m_init_altp2m > is called unconditionally (when booted with altp2m=1). Is it a > problem, though? Not a significant one. Initially I thought this would end up being a non- zero-size allocation, which we might like to avoid. But as it's a zero- size one, I think that's okay to leave as is. Jan
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 5234b627d0..e5785d2d96 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -688,7 +688,7 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) return -EINVAL; } - if ( config->altp2m.opts ) + if ( config->altp2m.opts || config->altp2m.nr ) { dprintk(XENLOG_INFO, "Altp2m not supported\n"); return -EINVAL; diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index bb5ba8fc1e..011cffb07e 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -724,16 +724,42 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) return -EINVAL; } - if ( altp2m_mode && nested_virt ) + if ( altp2m_mode ) { - dprintk(XENLOG_INFO, - "Nested virt and altp2m are not supported together\n"); - return -EINVAL; - } + if ( nested_virt ) + { + dprintk(XENLOG_INFO, + "Nested virt and altp2m are not supported together\n"); + return -EINVAL; + } + + if ( !hap ) + { + dprintk(XENLOG_INFO, "altp2m is only supported with HAP\n"); + return -EINVAL; + } + + if ( !hvm_altp2m_supported() ) + { + dprintk(XENLOG_INFO, "altp2m is not supported\n"); + return -EINVAL; + } + + if ( !config->altp2m.nr ) + { + /* Fix the value to the legacy default */ + config->altp2m.nr = 10; + } - if ( altp2m_mode && !hap ) + if ( config->altp2m.nr > MAX_NR_ALTP2M ) + { + dprintk(XENLOG_INFO, "altp2m.nr must be <= %lu\n", MAX_NR_ALTP2M); + return -EINVAL; + } + } + else if ( config->altp2m.nr ) { - dprintk(XENLOG_INFO, "altp2m is only supported with HAP\n"); + dprintk(XENLOG_INFO, "altp2m.nr must be zero when altp2m is off\n"); return -EINVAL; } diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index a66ebaaceb..3d0357a0f8 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4657,6 +4657,12 @@ static int do_altp2m_op( goto out; } + if ( d->nr_altp2m == 0 ) + { + rc = -EINVAL; + goto out; + } + if ( (rc = xsm_hvm_altp2mhvm_op(XSM_OTHER, d, mode, a.cmd)) ) goto out; @@ -5245,7 +5251,7 @@ void hvm_fast_singlestep(struct vcpu *v, uint16_t p2midx) if ( !hvm_is_singlestep_supported() ) return; - if ( p2midx >= MAX_ALTP2M ) + if ( p2midx >= v->domain->nr_altp2m ) return; v->arch.hvm.single_step = true; diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index a420d452b3..9292a2c8d8 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -4885,7 +4885,7 @@ bool asmlinkage vmx_vmenter_helper(const struct cpu_user_regs *regs) { unsigned int i; - for ( i = 0; i < MAX_ALTP2M; ++i ) + for ( i = 0; i < currd->nr_altp2m; ++i ) { if ( altp2m_get_eptp(currd, i) == mfn_x(INVALID_MFN) ) continue; diff --git a/xen/arch/x86/include/asm/altp2m.h b/xen/arch/x86/include/asm/altp2m.h index 2f064c61a2..a4cc3d3ffc 100644 --- a/xen/arch/x86/include/asm/altp2m.h +++ b/xen/arch/x86/include/asm/altp2m.h @@ -22,7 +22,7 @@ static inline bool altp2m_active(const struct domain *d) static inline struct p2m_domain *altp2m_get_p2m(const struct domain* d, unsigned int idx) { - return d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)]; + return d->arch.altp2m_p2m[array_index_nospec(idx, d->nr_altp2m)]; } static inline uint64_t altp2m_get_eptp(const struct domain* d, diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h index f5daeb182b..855e844bed 100644 --- a/xen/arch/x86/include/asm/domain.h +++ b/xen/arch/x86/include/asm/domain.h @@ -258,11 +258,12 @@ struct paging_vcpu { struct shadow_vcpu shadow; }; -#define MAX_NESTEDP2M 10 +#define MAX_EPTP (PAGE_SIZE / sizeof(uint64_t)) +#define MAX_NR_ALTP2M MAX_EPTP +#define MAX_NESTEDP2M 10 -#define MAX_ALTP2M 10 /* arbitrary */ #define INVALID_ALTP2M 0xffff -#define MAX_EPTP (PAGE_SIZE / sizeof(uint64_t)) + struct p2m_domain; struct time_scale { int shift; @@ -353,7 +354,7 @@ struct arch_domain /* altp2m: allow multiple copies of host p2m */ bool altp2m_active; - struct p2m_domain *altp2m_p2m[MAX_ALTP2M]; + struct p2m_domain **altp2m_p2m; mm_lock_t altp2m_list_lock; uint64_t *altp2m_eptp; uint64_t *altp2m_visible_eptp; diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h index 1c01e22c8e..277648dd18 100644 --- a/xen/arch/x86/include/asm/hvm/hvm.h +++ b/xen/arch/x86/include/asm/hvm/hvm.h @@ -828,6 +828,11 @@ static inline bool hvm_hap_supported(void) return false; } +static inline bool hvm_altp2m_supported(void) +{ + return false; +} + static inline bool hvm_nested_virt_supported(void) { return false; diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h index e6f7764f9f..a1094fc7b3 100644 --- a/xen/arch/x86/include/asm/p2m.h +++ b/xen/arch/x86/include/asm/p2m.h @@ -887,7 +887,7 @@ static inline struct p2m_domain *p2m_get_altp2m(struct vcpu *v) if ( index == INVALID_ALTP2M ) return NULL; - BUG_ON(index >= MAX_ALTP2M); + BUG_ON(index >= v->domain->nr_altp2m); return altp2m_get_p2m(v->domain, index); } @@ -898,7 +898,7 @@ static inline bool p2m_set_altp2m(struct vcpu *v, unsigned int idx) struct p2m_domain *orig; struct p2m_domain *ap2m; - BUG_ON(idx >= MAX_ALTP2M); + BUG_ON(idx >= v->domain->nr_altp2m); if ( idx == vcpu_altp2m(v).p2midx ) return false; diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c index 7fb1738376..d47277e5e5 100644 --- a/xen/arch/x86/mm/altp2m.c +++ b/xen/arch/x86/mm/altp2m.c @@ -15,6 +15,11 @@ void altp2m_vcpu_initialise(struct vcpu *v) { + struct domain *d = v->domain; + + if ( d->nr_altp2m == 0 ) + return; + if ( v != current ) vcpu_pause(v); @@ -30,8 +35,12 @@ altp2m_vcpu_initialise(struct vcpu *v) void altp2m_vcpu_destroy(struct vcpu *v) { + struct domain *d = v->domain; struct p2m_domain *p2m; + if ( d->nr_altp2m == 0 ) + return; + if ( v != current ) vcpu_pause(v); @@ -122,7 +131,12 @@ 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++ ) + d->arch.altp2m_p2m = xzalloc_array(struct p2m_domain *, d->nr_altp2m); + + if ( !d->arch.altp2m_p2m ) + return -ENOMEM; + + for ( i = 0; i < d->nr_altp2m; i++ ) { d->arch.altp2m_p2m[i] = p2m = p2m_init_one(d); if ( p2m == NULL ) @@ -143,7 +157,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->nr_altp2m; i++ ) { if ( !d->arch.altp2m_p2m[i] ) continue; @@ -151,6 +168,8 @@ void p2m_teardown_altp2m(struct domain *d) d->arch.altp2m_p2m[i] = NULL; p2m_free_one(p2m); } + + XFREE(d->arch.altp2m_p2m); } int altp2m_get_effective_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn, @@ -200,7 +219,7 @@ bool p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx) struct domain *d = v->domain; bool rc = false; - if ( idx >= MAX_ALTP2M ) + if ( idx >= d->nr_altp2m ) return rc; altp2m_list_lock(d); @@ -306,7 +325,7 @@ static void p2m_reset_altp2m(struct domain *d, unsigned int idx, { struct p2m_domain *p2m; - ASSERT(idx < MAX_ALTP2M); + ASSERT(idx < d->nr_altp2m); p2m = altp2m_get_p2m(d, idx); p2m_lock(p2m); @@ -332,7 +351,7 @@ void p2m_flush_altp2m(struct domain *d) altp2m_list_lock(d); - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { p2m_reset_altp2m(d, i, ALTP2M_DEACTIVATE); altp2m_set_eptp(d, i, mfn_x(INVALID_MFN)); @@ -348,7 +367,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx, struct p2m_domain *hostp2m, *p2m; int rc; - ASSERT(idx < MAX_ALTP2M); + ASSERT(idx < d->nr_altp2m); p2m = altp2m_get_p2m(d, idx); hostp2m = p2m_get_hostp2m(d); @@ -388,7 +407,7 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx) int rc = -EINVAL; struct p2m_domain *hostp2m = p2m_get_hostp2m(d); - if ( idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ) + if ( idx >= d->nr_altp2m ) return rc; altp2m_list_lock(d); @@ -414,7 +433,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx, altp2m_list_lock(d); - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) ) continue; @@ -436,7 +455,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx) struct p2m_domain *p2m; int rc = -EBUSY; - if ( !idx || idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ) + if ( !idx || idx >= d->nr_altp2m ) return rc; rc = domain_pause_except_self(d); @@ -471,7 +490,7 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx) struct vcpu *v; int rc = -EINVAL; - if ( idx >= MAX_ALTP2M ) + if ( idx >= d->nr_altp2m ) return rc; rc = domain_pause_except_self(d); @@ -506,8 +525,7 @@ 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) || - altp2m_get_eptp(d, idx) == mfn_x(INVALID_MFN) ) + if ( idx >= d->nr_altp2m || altp2m_get_eptp(d, idx) == mfn_x(INVALID_MFN) ) return rc; hp2m = p2m_get_hostp2m(d); @@ -567,7 +585,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn, altp2m_list_lock(d); - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { p2m_type_t t; p2m_access_t a; @@ -590,7 +608,7 @@ int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn, else { /* At least 2 altp2m's impacted, so reset everything */ - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { if ( i == last_reset_idx || altp2m_get_eptp(d, i) == mfn_x(INVALID_MFN) ) @@ -654,7 +672,7 @@ 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 >= d->nr_altp2m || altp2m_get_eptp(d, sve->view) == mfn_x(INVALID_MFN) ) return -EINVAL; @@ -721,7 +739,7 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve, if ( altp2m_idx > 0 ) { - if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) || + if ( altp2m_idx >= d->nr_altp2m || altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) ) return -EINVAL; @@ -756,9 +774,9 @@ int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int altp2m_idx, /* * Eptp index is correlated with altp2m index and should not exceed - * min(MAX_ALTP2M, MAX_EPTP). + * d->nr_altp2m. */ - if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) || + if ( altp2m_idx >= d->nr_altp2m || altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) ) rc = -EINVAL; else if ( visible ) diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c index 8fc8348152..8b23792a0d 100644 --- a/xen/arch/x86/mm/hap/hap.c +++ b/xen/arch/x86/mm/hap/hap.c @@ -515,7 +515,7 @@ int hap_enable(struct domain *d, u32 mode) altp2m_set_visible_eptp(d, i, mfn_x(INVALID_MFN)); } - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { rv = p2m_alloc_table(altp2m_get_p2m(d, i)); if ( rv != 0 ) @@ -538,8 +538,8 @@ void hap_final_teardown(struct domain *d) unsigned int i; if ( hvm_altp2m_supported() ) - for ( i = 0; i < MAX_ALTP2M; i++ ) - p2m_teardown(d->arch.altp2m_p2m[i], true, NULL); + for ( i = 0; i < d->nr_altp2m; i++ ) + p2m_teardown(altp2m_get_p2m(d, i), true, NULL); /* Destroy nestedp2m's first */ for (i = 0; i < MAX_NESTEDP2M; i++) { @@ -590,7 +590,7 @@ void hap_teardown(struct domain *d, bool *preempted) FREE_XENHEAP_PAGE(d->arch.altp2m_eptp); FREE_XENHEAP_PAGE(d->arch.altp2m_visible_eptp); - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { p2m_teardown(altp2m_get_p2m(d, i), false, preempted); if ( preempted && *preempted ) diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index 43f3a8c2aa..669a0d0a54 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -347,7 +347,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, /* altp2m view 0 is treated as the hostp2m */ if ( altp2m_idx ) { - if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) || + if ( altp2m_idx >= d->nr_altp2m || altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) ) return -EINVAL; @@ -402,7 +402,7 @@ long p2m_set_mem_access_multi(struct domain *d, /* altp2m view 0 is treated as the hostp2m */ if ( altp2m_idx ) { - if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) || + if ( altp2m_idx >= d->nr_altp2m || altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) ) return -EINVAL; @@ -464,7 +464,7 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access, } else if ( altp2m_idx ) /* altp2m view 0 is treated as the hostp2m */ { - if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) || + if ( altp2m_idx >= d->nr_altp2m || altp2m_get_eptp(d, altp2m_idx) == mfn_x(INVALID_MFN) ) return -EINVAL; @@ -483,7 +483,7 @@ void arch_p2m_set_access_required(struct domain *d, bool access_required) if ( altp2m_active(d) ) { unsigned int i; - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { struct p2m_domain *p2m = altp2m_get_p2m(d, i); diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index 21ac361111..2139d13009 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -912,7 +912,7 @@ static int nominate_page(struct domain *d, gfn_t gfn, altp2m_list_lock(d); - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { ap2m = altp2m_get_p2m(d, i); if ( !ap2m ) diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index ed4252822e..f90c82f89b 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -1293,7 +1293,7 @@ static void ept_set_ad_sync(struct domain *d, bool value) { unsigned int i; - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { struct p2m_domain *p2m; @@ -1519,7 +1519,7 @@ unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp) altp2m_list_lock(d); - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { if ( altp2m_get_eptp(d, i) == mfn_x(INVALID_MFN) ) continue; diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 30a44441ba..380b7ece9c 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -105,7 +105,7 @@ void p2m_change_entry_type_global(struct domain *d, { unsigned int i; - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) ) { @@ -140,7 +140,7 @@ void p2m_memory_type_changed(struct domain *d) { unsigned int i; - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) ) { @@ -913,7 +913,7 @@ void p2m_change_type_range(struct domain *d, { unsigned int i; - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) ) { @@ -986,7 +986,7 @@ int p2m_finish_type_change(struct domain *d, { unsigned int i; - for ( i = 0; i < MAX_ALTP2M; i++ ) + for ( i = 0; i < d->nr_altp2m; i++ ) { if ( altp2m_get_eptp(d, i) != mfn_x(INVALID_MFN) ) { diff --git a/xen/common/domain.c b/xen/common/domain.c index 67cadb7c3f..776442cec0 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -610,6 +610,7 @@ struct domain *domain_create(domid_t domid, if ( config ) { d->options = config->flags; + d->nr_altp2m = config->altp2m.nr; d->vmtrace_size = config->vmtrace_size; } diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index dea399aa8e..056bbc82a2 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -103,7 +103,10 @@ struct xen_domctl_createdomain { /* Altp2m mode signaling uses bits [0, 1]. */ #define XEN_DOMCTL_ALTP2M_mode_mask (0x3U) #define XEN_DOMCTL_ALTP2M_mode(m) ((m) & XEN_DOMCTL_ALTP2M_mode_mask) - uint32_t opts; + uint16_t opts; + + /* Number of altp2ms to allocate. */ + uint16_t nr; } altp2m; /* Per-vCPU buffer size in bytes. 0 to disable. */ diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 2dcd1d1a4f..7119f3c44f 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -610,6 +610,8 @@ struct domain unsigned int guest_request_sync : 1; } monitor; + unsigned int nr_altp2m; /* Number of altp2m tables */ + unsigned int vmtrace_size; /* Buffer size in bytes, or 0 to disable. */ #ifdef CONFIG_ARGO