Re: [xen-unstable-smoke test] 173492: regressions - FAIL

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 12/10/2022 11:49, Julien Grall wrote:
> Hi Andrew,
>
> On 12/10/2022 11:29, Andrew Cooper wrote:
>> On 12/10/2022 11:01, Julien Grall wrote:
>>> (+ Bertrand & Stefano)
>>>
>>> Hi Henry,
>>>
>>> On 12/10/2022 07:39, Henry Wang wrote:
>>>>> -----Original Message-----
>>>>> Subject: Re: [xen-unstable-smoke test] 173492: regressions - FAIL
>>>>>
>>>>> On 11.10.2022 18:29, osstest service owner wrote:
>>>>>> flight 173492 xen-unstable-smoke real [real]
>>>>>> http://logs.test-lab.xenproject.org/osstest/logs/173492/
>>>>>>
>>>>>> Regressions :-(
>>>>>>
>>>>>> Tests which did not succeed and are blocking,
>>>>>> including tests which could not be run:
>>>>>>    test-arm64-arm64-xl-xsm      14 guest-start              fail
>>>>>> REGR. vs. 173457
>>>>>
>>>>> Parsing config from /etc/xen/debian.guest.osstest.cfg
>>>>> libxl: debug: libxl_create.c:2079:do_domain_create: ao
>>>>> 0xaaaacaccf680:
>>>>> create: how=(nil) callback=(nil) poller=0xaaaacaccefd0
>>>>> libxl: detail: libxl_create.c:661:libxl__domain_make: passthrough:
>>>>> disabled
>>>>> libxl: debug: libxl_arm.c:148:libxl__arch_domain_prepare_config:
>>>>> Configure
>>>>> the domain
>>>>> libxl: debug: libxl_arm.c:151:libxl__arch_domain_prepare_config:  -
>>>>> Allocate
>>>>> 0 SPIs
>>>>> libxl: error: libxl_create.c:709:libxl__domain_make: domain creation
>>>>> fail: No
>>>>> such file or directory
>>>
>>> So this is -ENOENT which could be returned by the P2M is it can't
>>> allocate a page table (see p2m_set_entry()).
>>>
>>>>> libxl: error: libxl_create.c:1294:initiate_domain_create: cannot
>>>>> make domain:
>>>>> -3
>>>>>
>>>>> Later flights don't fail here anymore, though.
>>>>>
>>>>>>    test-armhf-armhf-xl          14 guest-start              fail
>>>>>> REGR. vs. 173457
>>>>>
>>>>> Similar log contents here, but later flights continue to fail the
>>>>> same way.
>>>>>
>>>>> I'm afraid I can't draw conclusions from this; I haven't been able
>>>>> to spot
>>>>> anything helpful in the hypervisor logs. My best guess right now is
>>>>> the use
>>>>> of some uninitialized memory, which just happened to go fine in the
>>>>> later
>>>>> flights for 64-bit.
>>>
>>> It looks like the smoke flight failed on laxton0 but passed on
>>> rochester{0, 1}. The former is using GICv2 whilst the latter are using
>>> GICv3.
>>>
>>> In the case of GICv2, we will create a P2M mapping when the domain is
>>> created. This is not necessary in the GICv3.
>>>
>>> IIRC the P2M pool is only populated later on (we don't add a few pages
>>> like on x86). So I am guessing this is why we are seen failure.
>>>
>>> If that's correct, then this is a complete oversight from me (I
>>> haven't done any GICv2 testing) while reviewing the series.
>>>
>>> The easy way to solve it would be to add a few pages in the pool when
>>> the domain is created. I don't like it, but I think there other
>>> possible solutions would require more work as we would need to delay
>>> the mappings.
>>
>> Honestly, I've considered doing this on x86 too.
>
> AFAICT, this is already the case for HAP (see call to
> hap_set_allocation() in hap_enable()). 256 pages will be pre-allocated.

Right, but it's asymmetric with shadow.  This wants fixing and simplifying.

>
>>
>> There are several things which want allocating in domain_create(), but
>> are deferred to max_vcpus() because they require the P2M having a
>> non-zero allocation.  This in turn means we've got a load of checks in
>> paths where we'd ideally not have them.
>>
>> We already have a calculation of the absolutely minimum we will ever
>> permit the p2m pool to be.  IMO we ought to allocate this minimum size
>> in domain_create().
>
> It depends on the number. At the moment domain_create() is not
> preemptible, so we don't want to allocate too many pages (I think even
> 256 pages could be risky on some Arm platform).
>
> Maybe the solution is to have domain_create() preemptible. But it is
> not something that could be done in the 4.17 time frame.

domain_create() can't be pre-emptible in its current form, because it
depends on "atomically" taking the domid from not existing to existing. 
Specifically, until the hypercall completes, other hypercalls can't find
a struct domain* for the domid.

This is necessary, because we guarantee that when you can look up a
domain by domid, e.g. the predicates work on it, and d->max_vcpus is
nonzero, etc.

In some future where the error paths have been made idempotent and we
have a clean split between teardown and destroy, we probably can alter
the existing creation path to do a more basic initial setup (which can
be cleaned up by the destroy logic), then insert the domain into dom
hashtable and automatically continue into a different subop and perform
more long-running setup.

But yeah - absolutely definitely not 4.17 content.

~Andrew