Re: Policy: A release acks for the release manager's patches (was Re: [PATCH v5 2/2] xen/arm: p2m: Populate pages for GICv2 mapping in p2m_init())

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 19/10/2022 16:28, George Dunlap wrote:

On Tue, Oct 18, 2022 at 3:24 PM Henry Wang <Henry.Wang@xxxxxxx> 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
which requires 6 P2M pages as the two pages will be consecutive but not
necessarily in the same L3 page table and keep a buffer, populate 16
pages as the default value to the P2M pages pool in p2m_init() at the
domain creation stage to satisfy the GICv2 requirement. 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.

With the default 16 P2M pages populated, there would be a case that
failures would happen in the domain creation with P2M pages already in
use. To properly free the P2M for this case, firstly support the
optionally preemption of p2m_teardown(), then call p2m_teardown() and
p2m_set_allocation(d, 0, NULL) non-preemptively in p2m_final_teardown().
As non-preemptive p2m_teardown() should only return 0, use a
BUG_ON to confirm that.

Since p2m_final_teardown() is called either after
domain_relinquish_resources() where relinquish_p2m_mapping() has been
called, or from failure path of domain_create()/arch_domain_create()
where mappings that require p2m_put_l3_page() should never be created,
relinquish_p2m_mapping() is not added in p2m_final_teardown(), add
in-code comments to refer this.

Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M pool")
Suggested-by: Julien Grall <jgrall@xxxxxxxxxx>
Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>

Henry brought this patch to my attention because it needs a release ack

Actually this one doesn't.  It's a security patch, and the only reason its on xen-devel is because OSSTest discovered that XSA-409 is DoA after the fact.  And on all security supported branches too.

When the bugs have been fixed, it will cause force a re-issue of XSA-409.

, but it doesn't seem proper for Henry to be the one to release-ack his own patches. :-)

I don't see an issue with an RM R-ack-ing their own patch.  There's past form for self-R-ack, and the patch still needs one other person and/or a maintainer/committer and the usual resolution of outstanding concerns.

There's administrivia which the RM typically does closer to the release, and we've never had cross-R-ack for the docs/process side of things.

I propose that a suitable rule would be:

"If the release manager themselves have submitted a patch which needs a release ack, then the patch needs a release ack from one of the Committers who is not involved in the patch."

Given the time-critical nature of this patch, I propose that we adopt the rule as an expediency now, and we can discuss afterwards whether to make it permanent.

With that in mind, it looks like this patch is critical for fixing a release issue; it's in core code, but has also has a lot of scrutiny.  So with that in mind:

Release-acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>

At the end of the day, R-ack means "I have deemed this important for the release", and the committers are the fallback for all corner cases.  I'd say that's already covered in the existing rules and conventions, given the expectation that committers wouldn't
 tread on the toes of the RM in the first place.

~Andrew