Re: [PATCH v2] xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create()

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 09/12/2022 18:51, Julien Grall wrote:
> Hi Henry,
>
> On 08/12/2022 03:06, Henry Wang wrote:
>> I am trying to work on the follow-up improvements about the Arm P2M
>> code,
>> and while trying to address the comment below, I noticed there was an
>> unfinished
>> discussion between me and Julien which I would like to continue and here
>> opinions from all of you (if possible).
>>
>>> -----Original Message-----
>>> From: Julien Grall <julien@xxxxxxx>
>>> Subject: Re: [PATCH v2] xen/arm: p2m: Populate pages for GICv2
>>> mapping in
>>> arch_domain_create()
>>>>> I also noticed that relinquish_p2m_mapping() is not called. This
>>>>> should
>>>>> be fine for us because arch_domain_create() should never create a
>>>>> mapping that requires p2m_put_l3_page() to be called.
>>
>> For the background of the discussion, this was about the failure path of
>> arch_domain_create(), where we only call p2m_final_teardown() which does
>> not call relinquish_p2m_mapping()...
> So all this mess with the P2M is necessary because we are mapping the
> GICv2 CPU interface in arch_domain_create(). I think we should
> consider to defer the mapping to later.
>
> Other than it makes the code simpler, it also means we could also the
> P2M root on the same numa node the domain is going to run (those
> information are passed later on).
>
> There is a couple of options:
>  1. Introduce a new hypercall to finish the initialization of a domain
> (e.g. allocating the P2M and map the GICv2 CPU interface). This option
> was briefly discussed with Jan (see [2])./
>  2. Allocate the P2M when populate the P2M pool and defer the GICv2
> CPU interface mapping until the first access (similar to how with deal
> with MMIO access for ACPI).
>
> I find the second option is neater but it has the drawback that it
> requires on more trap to the hypervisor and we can't report any
> mapping failure (which should not happen if the P2M was correctly
> sized). So I am leaning towards option 2.
>
> Any opinions?

A DOMCTL_creation_finished hypercall (name subject to improvement) is
mandatory for encrypted VM support in x86 (there's crypto needed at this
point to complete the measurement of the guest and create an attestation
report), so we are going to gain something to this effect one way or
another.

Such a hypercall also removes the giant bodge of using
domain_unpause_by_systemcontroller() for this purpose.

But it seems like ARM already has arch_domain_creation_finished() so you
can do 1) independently of adding a new hypercall.  x86 uses that hook
for hooking up the magic APICv sinc page into the p2m, so you're already
in good(?) company with a GIC bodge.


As to where the logic *should* be, it should be done in the hypercall(s)
where the toolstack is describing the guests phymap to Xen.  The fact
that these don't exist is yet another problem needing to be worked on.


That said, moving the creation side of things doesn't change the
teardown requirements.  When I get time to respin the fault_ttl series,
Gitlab CI will be able to demonstrate the bug I keep on telling you is
still there, the fix for which is taking the patch I already wrote for
you.  There is no correct way to move the final loop out of
complete_domain_destroy(), even if in the general case you can make it
more preemptible by moving the allocation later.

~Andrew