Re: [xen-unstable test] 162771: regressions - FAIL

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.06.2021 08:41, Juergen Gross wrote:
> On 14.06.21 04:21, osstest service owner wrote:
>> flight 162771 xen-unstable real [real]
>> flight 162783 xen-unstable real-retest [real]
>> http://logs.test-lab.xenproject.org/osstest/logs/162771/
>> http://logs.test-lab.xenproject.org/osstest/logs/162783/
>>
>> Regressions :-(
>>
>> Tests which did not succeed and are blocking,
>> including tests which could not be run:
>>   test-amd64-amd64-xl-qemuu-ovmf-amd64 15 guest-saverestore fail REGR. vs. 
>> 162533
>>   test-amd64-i386-xl-qemuu-ovmf-amd64 15 guest-saverestore fail REGR. vs. 
>> 162533
>>   test-amd64-amd64-i386-pvgrub 17 guest-localmigrate       fail REGR. vs. 
>> 162533
>>   test-amd64-amd64-amd64-pvgrub 17 guest-localmigrate      fail REGR. vs. 
>> 162533
> 
> Hmm, this is rather unfortunate.
> 
> Those last 2 tests failed due to commit 7bd8989ab77b6ade3b, but just
> reverting that patch doesn't seem right to me either.
> 
> The Linux kernel has a bug here: it will initially set max_pfn in the
> shared_info page to the size of the p2m_list (so my reasoning for above
> patch was wrong in this case), but when growing the p2m_list (e.g. due
> to ballooning or grant mapping), it will store a real pfn number in
> max_pfn. But even this pfn might be wrong, as only the pfn leading to
> allocation of a new p2m page will be stored in max_pfn, any higher new
> pfn having its p2m entry in the new p2m page won't result in a new
> max_pfn entry.
> 
> As a result I think the only sane handling would be to assume the
> max_pfn value read from the shared_info page is really a pfn.

This would be contrary to the public interface header having

    /*
     * Number of valid entries in the p2m table(s) anchored at
     * pfn_to_mfn_frame_list_list and/or p2m_vaddr.
     */
    unsigned long max_pfn;

IOW the name containing "max" is misleading (should be "num" or
alike), but there's no room imo to change this.

> This
> value should be adjusted to specify the last pfn of the related p2m
> page, and the resulting last p2m page should be tolerated to not be
> valid.
> 
> Another variant would be to just revert above commit and modify the
> semantics of max_pfn in the shared_info page to really mean max_pfn+1.

But that's what it is already, according to the comment. Are you
suggesting there was code prior to the change you've quoted that
already violated this (in Xen or the tool stack, that is, not
the issue you suggest has been present in Linux)?

Jan

> This would result in possible migration failures of ballooned Linux
> systems as today.
> 
> Additionally I'll fix the Linux kernel, of course.
> 
> Any thoughts?
> 
> 
> Juergen
>