| Message ID | 20221013083818.36209-1-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create() | expand |
Hi Henry, On 13/10/2022 09:38, Henry Wang wrote: > Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area > when the domain is created. Considering the worst case of page tables Can you describe in the commit message what is the worst case scenario? > and keep a buffer, populate 16 pages as the default value to the P2M > pages pool in arch_domain_create() at the domain creation stage to > satisfy the GICv2 requirement. > > Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M pool") > Suggested-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > --- > This should also be backported to 4.13, 4.14, 4.15 and 4.16. > --- > xen/arch/arm/domain.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 2c84e6dbbb..e40e2bcba1 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -740,6 +740,20 @@ int arch_domain_create(struct domain *d, > BUG(); > } > > + spin_lock(&d->arch.paging.lock); > + /* > + * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area The wording suggests that this is only necessary for GICv2. But below this is done unconditionally. I am happy with this been done unconditionally, but I think this should be clarified here. > + * when the domain is created. Considering the worst case for page > + * tables and keep a buffer, populate 16 pages to the P2M pages pool here. > + */ > + if ( (rc = p2m_set_allocation(d, 16, NULL)) != 0 ) > + { > + p2m_set_allocation(d, 0, NULL); Shouldn't this be done in p2m_fiinal_teardown() to cover so the pages will be freed anything after this call will fail (include in the caller domain_create())? > + spin_unlock(&d->arch.paging.lock); > + goto fail; > + } > + spin_unlock(&d->arch.paging.lock); > + > if ( (rc = domain_vgic_register(d, &count)) != 0 ) > goto fail; > Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in > arch_domain_create() > > Hi Henry, > > On 13/10/2022 09:38, Henry Wang wrote: > > Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area > > when the domain is created. Considering the worst case of page tables > > Can you describe in the commit message what is the worst case scenario? The two pages will be consecutive but not necessarily in the same L3 page table so the worst case is 4 + 2, is that correct? > > > and keep a buffer, populate 16 pages as the default value to the P2M > > pages pool in arch_domain_create() at the domain creation stage to > > satisfy the GICv2 requirement. > > > > Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M > pool") > > Suggested-by: Julien Grall <jgrall@amazon.com> > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > > --- > > This should also be backported to 4.13, 4.14, 4.15 and 4.16. > > --- > > xen/arch/arm/domain.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > > index 2c84e6dbbb..e40e2bcba1 100644 > > --- a/xen/arch/arm/domain.c > > +++ b/xen/arch/arm/domain.c > > @@ -740,6 +740,20 @@ int arch_domain_create(struct domain *d, > > BUG(); > > } > > > > + spin_lock(&d->arch.paging.lock); > > + /* > > + * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 > area > > The wording suggests that this is only necessary for GICv2. But below > this is done unconditionally. I am happy with this been done > unconditionally, but I think this should be clarified here. Sure, I will add "For GICv3, the above-mentioned P2M mapping is not necessary, but since the allocated 16 pages here would not be lost, hence populate these pages unconditionally" if it is ok to you. > > > + * when the domain is created. Considering the worst case for page > > + * tables and keep a buffer, populate 16 pages to the P2M pages pool > here. > > + */ > > + if ( (rc = p2m_set_allocation(d, 16, NULL)) != 0 ) > > + { > > + p2m_set_allocation(d, 0, NULL); > > Shouldn't this be done in p2m_fiinal_teardown() to cover so the pages > will be freed anything after this call will fail (include in the caller > domain_create())? Hmm, yes, I will remove this p2m_set_allocation(d, 0, NULL); in v2. Kind regards, Henry > > > + spin_unlock(&d->arch.paging.lock); > > + goto fail; > > + } > > + spin_unlock(&d->arch.paging.lock); > > + > > if ( (rc = domain_vgic_register(d, &count)) != 0 ) > > goto fail; > > > > Cheers, > > -- > Julien Grall
On 13/10/2022 09:38, Henry Wang wrote: > Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area > when the domain is created. Considering the worst case of page tables > and keep a buffer, populate 16 pages as the default value to the P2M > pages pool in arch_domain_create() at the domain creation stage to > satisfy the GICv2 requirement. > > Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M pool") > Suggested-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > --- > This should also be backported to 4.13, 4.14, 4.15 and 4.16. > --- > xen/arch/arm/domain.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 2c84e6dbbb..e40e2bcba1 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -740,6 +740,20 @@ int arch_domain_create(struct domain *d, > BUG(); > } > > + spin_lock(&d->arch.paging.lock); > + /* > + * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area > + * when the domain is created. Considering the worst case for page > + * tables and keep a buffer, populate 16 pages to the P2M pages pool here. > + */ > + if ( (rc = p2m_set_allocation(d, 16, NULL)) != 0 ) > + { > + p2m_set_allocation(d, 0, NULL); > + spin_unlock(&d->arch.paging.lock); > + goto fail; > + } > + spin_unlock(&d->arch.paging.lock); Generally, this would be better written as spin_lock(); if ( rc = p2m_set_allocation(16) ) p2m_set_allocation(0) spin_unlock(); if ( rc ) goto fail; to reduce the number of spin_unlock() calls and make the error paths more clear. However... > + > if ( (rc = domain_vgic_register(d, &count)) != 0 ) > goto fail; > ... you've got a problem on this error path, so the set allocation to 0 needs to be in the fail: path with suitable locking. There are perhaps better ways of doing it in 4.15(?) and later, but not in earlier versions. As this is a fix to a bug in a security patch, simplicity is generally the better approach. ~Andrew
Hi Andrew, > -----Original Message----- > From: Andrew Cooper <Andrew.Cooper3@citrix.com> > Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in > arch_domain_create() > > On 13/10/2022 09:38, Henry Wang wrote: > > Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area > > when the domain is created. Considering the worst case of page tables > > and keep a buffer, populate 16 pages as the default value to the P2M > > pages pool in arch_domain_create() at the domain creation stage to > > satisfy the GICv2 requirement. > > > > Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M > pool") > > Suggested-by: Julien Grall <jgrall@amazon.com> > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > > --- > > This should also be backported to 4.13, 4.14, 4.15 and 4.16. > > --- > > xen/arch/arm/domain.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > > index 2c84e6dbbb..e40e2bcba1 100644 > > --- a/xen/arch/arm/domain.c > > +++ b/xen/arch/arm/domain.c > > @@ -740,6 +740,20 @@ int arch_domain_create(struct domain *d, > > BUG(); > > } > > > > + spin_lock(&d->arch.paging.lock); > > + /* > > + * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 > area > > + * when the domain is created. Considering the worst case for page > > + * tables and keep a buffer, populate 16 pages to the P2M pages pool > here. > > + */ > > + if ( (rc = p2m_set_allocation(d, 16, NULL)) != 0 ) > > + { > > + p2m_set_allocation(d, 0, NULL); > > + spin_unlock(&d->arch.paging.lock); > > + goto fail; > > + } > > + spin_unlock(&d->arch.paging.lock); > > Generally, this would be better written as > > spin_lock(); > if ( rc = p2m_set_allocation(16) ) > p2m_set_allocation(0) > spin_unlock(); > > if ( rc ) > goto fail; > > to reduce the number of spin_unlock() calls and make the error paths > more clear. However... I think in Arm's arch_domain_create(), all the error handling are the same style using: if ( (rc = <function>) !=0 ) goto fail; and we need to keep them the same? But I think I will drop the p2m_set_allocation(d, 0, NULL); as the arch_domain_destroy(d) in fail: d->is_dying = DOMDYING_dead; arch_domain_destroy(d); will clean-up the pool. Kind regards, Henry > > > + > > if ( (rc = domain_vgic_register(d, &count)) != 0 ) > > goto fail; > > > > ... you've got a problem on this error path, so the set allocation to 0 > needs to be in the fail: path with suitable locking. > > There are perhaps better ways of doing it in 4.15(?) and later, but not > in earlier versions. As this is a fix to a bug in a security patch, > simplicity is generally the better approach. > > ~Andrew
On 13/10/2022 10:21, Henry Wang wrote: > Hi Julien, Hi Henry, > >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in >> arch_domain_create() >> >> Hi Henry, >> >> On 13/10/2022 09:38, Henry Wang wrote: >>> Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area >>> when the domain is created. Considering the worst case of page tables >> >> Can you describe in the commit message what is the worst case scenario? > > The two pages will be consecutive but not necessarily in the same L3 page > table so the worst case is 4 + 2, is that correct? So I agree that the worse case is 6. But I don't understand what you mean by '4 + 2' here. > >> >>> and keep a buffer, populate 16 pages as the default value to the P2M >>> pages pool in arch_domain_create() at the domain creation stage to >>> satisfy the GICv2 requirement. >>> >>> Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M >> pool") >>> Suggested-by: Julien Grall <jgrall@amazon.com> >>> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >>> --- >>> This should also be backported to 4.13, 4.14, 4.15 and 4.16. >>> --- >>> xen/arch/arm/domain.c | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >>> >>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >>> index 2c84e6dbbb..e40e2bcba1 100644 >>> --- a/xen/arch/arm/domain.c >>> +++ b/xen/arch/arm/domain.c >>> @@ -740,6 +740,20 @@ int arch_domain_create(struct domain *d, >>> BUG(); >>> } >>> >>> + spin_lock(&d->arch.paging.lock); >>> + /* >>> + * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 >> area >> >> The wording suggests that this is only necessary for GICv2. But below >> this is done unconditionally. I am happy with this been done >> unconditionally, but I think this should be clarified here. > > Sure, I will add "For GICv3, the above-mentioned P2M mapping is not > necessary, but since the allocated 16 pages here would not be lost, hence > populate these pages unconditionally" if it is ok to you. Sounds good to me. > >> >>> + * when the domain is created. Considering the worst case for page >>> + * tables and keep a buffer, populate 16 pages to the P2M pages pool >> here. >>> + */ >>> + if ( (rc = p2m_set_allocation(d, 16, NULL)) != 0 ) >>> + { >>> + p2m_set_allocation(d, 0, NULL); >> >> Shouldn't this be done in p2m_fiinal_teardown() to cover so the pages >> will be freed anything after this call will fail (include in the caller >> domain_create())? > > Hmm, yes, I will remove this p2m_set_allocation(d, 0, NULL); in v2. Just to clarify, I meant that a call in p2m_final_teardown() *is* missing in p2m_final_teardown() (or wherever we decide to add). This would make this one redundant. Cheers,
On 13.10.2022 12:58, Julien Grall wrote: > On 13/10/2022 10:21, Henry Wang wrote: >>> -----Original Message----- >>> From: Julien Grall <julien@xen.org> >>> Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in >>> arch_domain_create() >>> >>> On 13/10/2022 09:38, Henry Wang wrote: >>>> Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area >>>> when the domain is created. Considering the worst case of page tables >>> >>> Can you describe in the commit message what is the worst case scenario? >> >> The two pages will be consecutive but not necessarily in the same L3 page >> table so the worst case is 4 + 2, is that correct? > > So I agree that the worse case is 6. But I don't understand what you > mean by '4 + 2' here. Assuming you have 4 (N) page table levels, isn't it 7 (1 + 2 * (N - 1))? Or is the root table not taken from the p2m pool? Jan
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in > arch_domain_create() > > On 13.10.2022 12:58, Julien Grall wrote: > > On 13/10/2022 10:21, Henry Wang wrote: > >>> -----Original Message----- > >>> From: Julien Grall <julien@xen.org> > >>> Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping > in > >>> arch_domain_create() > >>> > >>> On 13/10/2022 09:38, Henry Wang wrote: > >>>> Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 > area > >>>> when the domain is created. Considering the worst case of page tables > >>> > >>> Can you describe in the commit message what is the worst case scenario? > >> > >> The two pages will be consecutive but not necessarily in the same L3 page > >> table so the worst case is 4 + 2, is that correct? > > > > So I agree that the worse case is 6. But I don't understand what you > > mean by '4 + 2' here. > > Assuming you have 4 (N) page table levels, isn't it 7 (1 + 2 * (N - 1))? > Or is the root table not taken from the p2m pool? Correct, on arm the root is not taken from the pool. Kind regards, Henry > > Jan
On 13.10.2022 13:05, Henry Wang wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in >> arch_domain_create() >> >> On 13.10.2022 12:58, Julien Grall wrote: >>> On 13/10/2022 10:21, Henry Wang wrote: >>>>> -----Original Message----- >>>>> From: Julien Grall <julien@xen.org> >>>>> Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping >> in >>>>> arch_domain_create() >>>>> >>>>> On 13/10/2022 09:38, Henry Wang wrote: >>>>>> Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 >> area >>>>>> when the domain is created. Considering the worst case of page tables >>>>> >>>>> Can you describe in the commit message what is the worst case scenario? >>>> >>>> The two pages will be consecutive but not necessarily in the same L3 page >>>> table so the worst case is 4 + 2, is that correct? >>> >>> So I agree that the worse case is 6. But I don't understand what you >>> mean by '4 + 2' here. >> >> Assuming you have 4 (N) page table levels, isn't it 7 (1 + 2 * (N - 1))? >> Or is the root table not taken from the p2m pool? > > Correct, on arm the root is not taken from the pool. Isn't that a (perhaps just minor) mistake? Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > >> Assuming you have 4 (N) page table levels, isn't it 7 (1 + 2 * (N - 1))? > >> Or is the root table not taken from the p2m pool? > > > > Correct, on arm the root is not taken from the pool. > > Isn't that a (perhaps just minor) mistake? Not really, in the code review phase, the question of whether we include the root in the p2m pool was discussed and the conclusion at that time was not including this page for now, as this is supposed to require a lot of extra work/refactor. Probably there will be a series from my side to add the root to the pool, but at least not now. Kind regards, Henry > > Jan
Hi, On 13/10/2022 13:29, Henry Wang wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >>>> Assuming you have 4 (N) page table levels, isn't it 7 (1 + 2 * (N - 1))? >>>> Or is the root table not taken from the p2m pool? >>> >>> Correct, on arm the root is not taken from the pool. >> >> Isn't that a (perhaps just minor) mistake? > > Not really, in the code review phase, the question of whether we include > the root in the p2m pool was discussed and the conclusion at that time > was not including this page for now, as this is supposed to require a lot > of extra work/refactor. Probably there will be a series from my side to > add the root to the pool, but at least not now. The root page tables can be one of multiple concatenated pages (up to 8 pages). The P2M pool is allocating page by page and therefore wouldn't allow us to allocate contiguous pages. Therefore, we need to handle the root differently. At which point it doesn't seem to be worth it to allocate it from the P2M pool. Cheers,
Hi Henry, On 13/10/2022 11:58, Julien Grall wrote: >>> >>>> + * when the domain is created. Considering the worst case for page >>>> + * tables and keep a buffer, populate 16 pages to the P2M pages >>>> pool >>> here. >>>> + */ >>>> + if ( (rc = p2m_set_allocation(d, 16, NULL)) != 0 ) >>>> + { >>>> + p2m_set_allocation(d, 0, NULL); >>> >>> Shouldn't this be done in p2m_fiinal_teardown() to cover so the pages >>> will be freed anything after this call will fail (include in the caller >>> domain_create())? >> >> Hmm, yes, I will remove this p2m_set_allocation(d, 0, NULL); in v2. > > Just to clarify, I meant that a call in p2m_final_teardown() *is* > missing in p2m_final_teardown() (or wherever we decide to add). > > This would make this one redundant. While chatting with you on IRC, I realized that a call to p2m_set_allocation() will only freed unused P2M pages. If some of them are in the P2M then they would be skipped. This means we also need to call p2m_teardown() (which would need to be optionally preemptible). Cheers,
On 13/10/2022 10:40, Henry Wang wrote: > Hi Andrew, > >> -----Original Message----- >> From: Andrew Cooper <Andrew.Cooper3@citrix.com> >> Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in >> arch_domain_create() >> >> On 13/10/2022 09:38, Henry Wang wrote: >>> Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area >>> when the domain is created. Considering the worst case of page tables >>> and keep a buffer, populate 16 pages as the default value to the P2M >>> pages pool in arch_domain_create() at the domain creation stage to >>> satisfy the GICv2 requirement. >>> >>> Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M >> pool") >>> Suggested-by: Julien Grall <jgrall@amazon.com> >>> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >>> --- >>> This should also be backported to 4.13, 4.14, 4.15 and 4.16. >>> --- >>> xen/arch/arm/domain.c | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >>> >>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >>> index 2c84e6dbbb..e40e2bcba1 100644 >>> --- a/xen/arch/arm/domain.c >>> +++ b/xen/arch/arm/domain.c >>> @@ -740,6 +740,20 @@ int arch_domain_create(struct domain *d, >>> BUG(); >>> } >>> >>> + spin_lock(&d->arch.paging.lock); >>> + /* >>> + * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 >> area >>> + * when the domain is created. Considering the worst case for page >>> + * tables and keep a buffer, populate 16 pages to the P2M pages pool >> here. >>> + */ >>> + if ( (rc = p2m_set_allocation(d, 16, NULL)) != 0 ) >>> + { >>> + p2m_set_allocation(d, 0, NULL); >>> + spin_unlock(&d->arch.paging.lock); >>> + goto fail; >>> + } >>> + spin_unlock(&d->arch.paging.lock); >> >> Generally, this would be better written as >> >> spin_lock(); >> if ( rc = p2m_set_allocation(16) ) >> p2m_set_allocation(0) >> spin_unlock(); >> >> if ( rc ) >> goto fail; >> >> to reduce the number of spin_unlock() calls and make the error paths >> more clear. However... > > I think in Arm's arch_domain_create(), all the error handling are the > same style using: > > if ( (rc = <function>) !=0 ) > goto fail; > > and we need to keep them the same? We don't have too. I agree with Andrew's point. How about the following: spin_lock() rc = ... spin_unlock() if ( rc ) goto fail; Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in > arch_domain_create() > > I think in Arm's arch_domain_create(), all the error handling are the > > same style using: > > > > if ( (rc = <function>) !=0 ) > > goto fail; > > > > and we need to keep them the same? > > We don't have too. I agree with Andrew's point. How about the following: > > spin_lock() > rc = ... > spin_unlock() > > if ( rc ) > goto fail; Yep this is my current method of local v2. Thanks for the clarification. Working on the p2m_teardown part now... Kind regards, Henry > > Cheers, > > -- > Julien Grall
Hi Andrew, On 13/10/2022 10:32, Andrew Cooper wrote: > On 13/10/2022 09:38, Henry Wang wrote: >> Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area >> when the domain is created. Considering the worst case of page tables >> and keep a buffer, populate 16 pages as the default value to the P2M >> pages pool in arch_domain_create() at the domain creation stage to >> satisfy the GICv2 requirement. >> >> Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M pool") >> Suggested-by: Julien Grall <jgrall@amazon.com> >> Signed-off-by: Henry Wang <Henry.Wang@arm.com> >> --- >> This should also be backported to 4.13, 4.14, 4.15 and 4.16. >> --- >> xen/arch/arm/domain.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index 2c84e6dbbb..e40e2bcba1 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -740,6 +740,20 @@ int arch_domain_create(struct domain *d, >> BUG(); >> } >> >> + spin_lock(&d->arch.paging.lock); >> + /* >> + * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area >> + * when the domain is created. Considering the worst case for page >> + * tables and keep a buffer, populate 16 pages to the P2M pages pool here. >> + */ >> + if ( (rc = p2m_set_allocation(d, 16, NULL)) != 0 ) >> + { >> + p2m_set_allocation(d, 0, NULL); >> + spin_unlock(&d->arch.paging.lock); >> + goto fail; >> + } >> + spin_unlock(&d->arch.paging.lock); > > Generally, this would be better written as > > spin_lock(); > if ( rc = p2m_set_allocation(16) ) > p2m_set_allocation(0) > spin_unlock(); > > if ( rc ) > goto fail; > > to reduce the number of spin_unlock() calls and make the error paths > more clear. However... > >> + >> if ( (rc = domain_vgic_register(d, &count)) != 0 ) >> goto fail; >> > > ... you've got a problem on this error path, so the set allocation to 0 > needs to be in the fail: path with suitable locking. > > There are perhaps better ways of doing it in 4.15(?) and later, but not > in earlier versions. As this is a fix to a bug in a security patch, > simplicity is generally the better approach. I guess you are referring to domain_teardown()? I think it may end up to be quite large because we would have to move other bits of the arch_domain_destroy() in domain_teardown(). It is also not clear whether part of domain_relinquish_memory() would need to be moved there as well. So this sounds more like some work for post-4.17. Cheers,
On 10/13/22 15:39, Julien Grall wrote: > Hi, > > On 13/10/2022 13:29, Henry Wang wrote: >>> -----Original Message----- >>> From: Jan Beulich <jbeulich@suse.com> >>>>> Assuming you have 4 (N) page table levels, isn't it 7 (1 + 2 * (N - >>>>> 1))? >>>>> Or is the root table not taken from the p2m pool? >>>> >>>> Correct, on arm the root is not taken from the pool. >>> >>> Isn't that a (perhaps just minor) mistake? >> >> Not really, in the code review phase, the question of whether we include >> the root in the p2m pool was discussed and the conclusion at that time >> was not including this page for now, as this is supposed to require a lot >> of extra work/refactor. Probably there will be a series from my side to >> add the root to the pool, but at least not now. > > The root page tables can be one of multiple concatenated pages (up to 8 > pages). The P2M pool is allocating page by page and therefore wouldn't > allow us to allocate contiguous pages. Sorry that I 'm asking this so late (I was just going through the thread) but why 8? > > Therefore, we need to handle the root differently. At which point it > doesn't seem to be worth it to allocate it from the P2M pool. > > Cheers, >
Hi Xenia, On 14/10/2022 10:46, Xenia Ragiadakou wrote: > > On 10/13/22 15:39, Julien Grall wrote: >> Hi, >> >> On 13/10/2022 13:29, Henry Wang wrote: >>>> -----Original Message----- >>>> From: Jan Beulich <jbeulich@suse.com> >>>>>> Assuming you have 4 (N) page table levels, isn't it 7 (1 + 2 * (N >>>>>> - 1))? >>>>>> Or is the root table not taken from the p2m pool? >>>>> >>>>> Correct, on arm the root is not taken from the pool. >>>> >>>> Isn't that a (perhaps just minor) mistake? >>> >>> Not really, in the code review phase, the question of whether we include >>> the root in the p2m pool was discussed and the conclusion at that time >>> was not including this page for now, as this is supposed to require a >>> lot >>> of extra work/refactor. Probably there will be a series from my side to >>> add the root to the pool, but at least not now. >> >> The root page tables can be one of multiple concatenated pages (up to >> 8 pages). The P2M pool is allocating page by page and therefore >> wouldn't allow us to allocate contiguous pages. > > Sorry that I 'm asking this so late (I was just going through the > thread) but why 8? Rather than providing an extra level of page-tables, the architecture allows you to provide multiple pages at the root level. The number of concatenated pages depend on the maximum physical address and the start level. You can look at the table in setup_virt_paging() for more details. Cheers,
On 10/14/22 13:04, Julien Grall wrote: > Hi Xenia, > > On 14/10/2022 10:46, Xenia Ragiadakou wrote: >> >> On 10/13/22 15:39, Julien Grall wrote: >>> Hi, >>> >>> On 13/10/2022 13:29, Henry Wang wrote: >>>>> -----Original Message----- >>>>> From: Jan Beulich <jbeulich@suse.com> >>>>>>> Assuming you have 4 (N) page table levels, isn't it 7 (1 + 2 * (N >>>>>>> - 1))? >>>>>>> Or is the root table not taken from the p2m pool? >>>>>> >>>>>> Correct, on arm the root is not taken from the pool. >>>>> >>>>> Isn't that a (perhaps just minor) mistake? >>>> >>>> Not really, in the code review phase, the question of whether we >>>> include >>>> the root in the p2m pool was discussed and the conclusion at that time >>>> was not including this page for now, as this is supposed to require >>>> a lot >>>> of extra work/refactor. Probably there will be a series from my side to >>>> add the root to the pool, but at least not now. >>> >>> The root page tables can be one of multiple concatenated pages (up to >>> 8 pages). The P2M pool is allocating page by page and therefore >>> wouldn't allow us to allocate contiguous pages. >> >> Sorry that I 'm asking this so late (I was just going through the >> thread) but why 8? > Rather than providing an extra level of page-tables, the architecture > allows you to provide multiple pages at the root level. > > The number of concatenated pages depend on the maximum physical address > and the start level. You can look at the table in setup_virt_paging() > for more details. I think you are referring to this: [6] = { 52, 12/*12*/, 3, 3 }, Still I cannot understand why the maximum number of concatenated level 0 translation tables for t0sz 12 and 4KB granule is 8 and not 16? > > Cheers, >
On 10/14/22 13:22, Xenia Ragiadakou wrote: > > On 10/14/22 13:04, Julien Grall wrote: >> Hi Xenia, >> >> On 14/10/2022 10:46, Xenia Ragiadakou wrote: >>> >>> On 10/13/22 15:39, Julien Grall wrote: >>>> Hi, >>>> >>>> On 13/10/2022 13:29, Henry Wang wrote: >>>>>> -----Original Message----- >>>>>> From: Jan Beulich <jbeulich@suse.com> >>>>>>>> Assuming you have 4 (N) page table levels, isn't it 7 (1 + 2 * >>>>>>>> (N - 1))? >>>>>>>> Or is the root table not taken from the p2m pool? >>>>>>> >>>>>>> Correct, on arm the root is not taken from the pool. >>>>>> >>>>>> Isn't that a (perhaps just minor) mistake? >>>>> >>>>> Not really, in the code review phase, the question of whether we >>>>> include >>>>> the root in the p2m pool was discussed and the conclusion at that time >>>>> was not including this page for now, as this is supposed to require >>>>> a lot >>>>> of extra work/refactor. Probably there will be a series from my >>>>> side to >>>>> add the root to the pool, but at least not now. >>>> >>>> The root page tables can be one of multiple concatenated pages (up >>>> to 8 pages). The P2M pool is allocating page by page and therefore >>>> wouldn't allow us to allocate contiguous pages. >>> >>> Sorry that I 'm asking this so late (I was just going through the >>> thread) but why 8? >> Rather than providing an extra level of page-tables, the architecture >> allows you to provide multiple pages at the root level. >> >> The number of concatenated pages depend on the maximum physical >> address and the start level. You can look at the table in >> setup_virt_paging() for more details. > > I think you are referring to this: > [6] = { 52, 12/*12*/, 3, 3 }, > Still I cannot understand why the maximum number of concatenated level 0 > translation tables for t0sz 12 and 4KB granule is 8 and not 16? > Let me explain. Initially I got confused because according to the manual up to 16 translation tables can be concatenated. Then, looking at the code you have pointed out, I understood that you were referring to the maximum root order in the table below. pa_range_info[] = { /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */ /* PA size, t0sz(min), root-order, sl0(max) */ [0] = { 32, 32/*32*/, 0, 1 }, [1] = { 36, 28/*28*/, 0, 1 }, [2] = { 40, 24/*24*/, 1, 1 }, [3] = { 42, 22/*22*/, 3, 1 }, [4] = { 44, 20/*20*/, 0, 2 }, [5] = { 48, 16/*16*/, 0, 2 }, [6] = { 52, 12/*12*/, 3, 3 }, [7] = { 0 } /* Invalid */ }; I think that the pa range info for the 52-bit pa range is wrong. IMO, it could be either [6] = { 52, 12/*12*/, 0, 3 } i.e single level -1 table or [6] = { 52, 12/*12*/, 4, 2 } i.e 16 concatenated level 0 tables Hope I am not totally out of context. >> >> Cheers, >> >
Hi Xenia, On 14/10/2022 16:05, Xenia Ragiadakou wrote: > On 10/14/22 13:22, Xenia Ragiadakou wrote: >> >> On 10/14/22 13:04, Julien Grall wrote: >>> Hi Xenia, >>> >>> On 14/10/2022 10:46, Xenia Ragiadakou wrote: >>>> >>>> On 10/13/22 15:39, Julien Grall wrote: >>>>> Hi, >>>>> >>>>> On 13/10/2022 13:29, Henry Wang wrote: >>>>>>> -----Original Message----- >>>>>>> From: Jan Beulich <jbeulich@suse.com> >>>>>>>>> Assuming you have 4 (N) page table levels, isn't it 7 (1 + 2 * >>>>>>>>> (N - 1))? >>>>>>>>> Or is the root table not taken from the p2m pool? >>>>>>>> >>>>>>>> Correct, on arm the root is not taken from the pool. >>>>>>> >>>>>>> Isn't that a (perhaps just minor) mistake? >>>>>> >>>>>> Not really, in the code review phase, the question of whether we >>>>>> include >>>>>> the root in the p2m pool was discussed and the conclusion at that >>>>>> time >>>>>> was not including this page for now, as this is supposed to >>>>>> require a lot >>>>>> of extra work/refactor. Probably there will be a series from my >>>>>> side to >>>>>> add the root to the pool, but at least not now. >>>>> >>>>> The root page tables can be one of multiple concatenated pages (up >>>>> to 8 pages). The P2M pool is allocating page by page and therefore >>>>> wouldn't allow us to allocate contiguous pages. >>>> >>>> Sorry that I 'm asking this so late (I was just going through the >>>> thread) but why 8? >>> Rather than providing an extra level of page-tables, the architecture >>> allows you to provide multiple pages at the root level. >>> >>> The number of concatenated pages depend on the maximum physical >>> address and the start level. You can look at the table in >>> setup_virt_paging() for more details. >> >> I think you are referring to this: >> [6] = { 52, 12/*12*/, 3, 3 }, >> Still I cannot understand why the maximum number of concatenated level >> 0 translation tables for t0sz 12 and 4KB granule is 8 and not 16? >> > > Let me explain. Initially I got confused because according to the manual > up to 16 translation tables can be concatenated. Then, looking at the > code you have pointed out, I understood that you were referring to the > maximum root order in the table below. > > pa_range_info[] = { > /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */ > /* PA size, t0sz(min), root-order, sl0(max) */ > [0] = { 32, 32/*32*/, 0, 1 }, > [1] = { 36, 28/*28*/, 0, 1 }, > [2] = { 40, 24/*24*/, 1, 1 }, > [3] = { 42, 22/*22*/, 3, 1 }, > [4] = { 44, 20/*20*/, 0, 2 }, > [5] = { 48, 16/*16*/, 0, 2 }, > [6] = { 52, 12/*12*/, 3, 3 }, > [7] = { 0 } /* Invalid */ > }; > > I think that the pa range info for the 52-bit pa range is wrong. I was about to say the same and note that in Xen we don't support 52-bits. So effectively those values are ignored for now. > IMO, it could be either > [6] = { 52, 12/*12*/, 0, 3 } i.e single level -1 table I don't think Xen is ready to support -1 for the root level. So... > or > [6] = { 52, 12/*12*/, 4, 2 } i.e 16 concatenated level 0 > tables ... this would be better. But this is not related to this patch directly. So any fix should be sent separately. Cheers,
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 2c84e6dbbb..e40e2bcba1 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -740,6 +740,20 @@ int arch_domain_create(struct domain *d, BUG(); } + spin_lock(&d->arch.paging.lock); + /* + * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area + * when the domain is created. Considering the worst case for page + * tables and keep a buffer, populate 16 pages to the P2M pages pool here. + */ + if ( (rc = p2m_set_allocation(d, 16, NULL)) != 0 ) + { + p2m_set_allocation(d, 0, NULL); + spin_unlock(&d->arch.paging.lock); + goto fail; + } + spin_unlock(&d->arch.paging.lock); + if ( (rc = domain_vgic_register(d, &count)) != 0 ) goto fail;
Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area when the domain is created. Considering the worst case of page tables and keep a buffer, populate 16 pages as the default value to the P2M pages pool in arch_domain_create() at the domain creation stage to satisfy the GICv2 requirement. Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M pool") Suggested-by: Julien Grall <jgrall@amazon.com> Signed-off-by: Henry Wang <Henry.Wang@arm.com> --- This should also be backported to 4.13, 4.14, 4.15 and 4.16. --- xen/arch/arm/domain.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)