Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 26 Aug 2019 16:18:47 +0800
From:   Peter Xu <peterx@redhat.com>
To:     Thomas Huth <thuth@redhat.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Andrew Jones <drjones@redhat.com>,
        Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Subject: Re: [PATCH] KVM: selftests: Detect max PA width from cpuid
Message-ID: <20190826081847.GB1785@xz-x1>
References: <20190826075728.21646-1-peterx@redhat.com>
 <00533992-f6e9-3c06-3342-b2b8a95b61d7@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <00533992-f6e9-3c06-3342-b2b8a95b61d7@redhat.com>
User-Agent: Mutt/1.11.4 (2019-03-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Aug 26, 2019 at 10:11:34AM +0200, Thomas Huth wrote:
> On 26/08/2019 09.57, Peter Xu wrote:
> > The dirty_log_test is failing on some old machines like Xeon E3-1220
> > with tripple faults when writting to the tracked memory region:
> > 
> >   Test iterations: 32, interval: 10 (ms)
> >   Testing guest mode: PA-bits:52, VA-bits:48, 4K pages
> >   guest physical test memory offset: 0x7fbffef000
> >   ==== Test Assertion Failure ====
> >   dirty_log_test.c:138: false
> >   pid=6137 tid=6139 - Success
> >      1  0x0000000000401ca1: vcpu_worker at dirty_log_test.c:138
> >      2  0x00007f3dd9e392dd: ?? ??:0
> >      3  0x00007f3dd9b6a132: ?? ??:0
> >   Invalid guest sync status: exit_reason=SHUTDOWN
> > 
> > It's because previously we moved the testing memory region from a
> > static place (1G) to the top of the system's physical address space,
> > meanwhile we stick to 39 bits PA for all the x86_64 machines.  That's
> > not true for machines like Xeon E3-1220 where it only supports 36.
> > 
> > Let's unbreak this test by dynamically detect PA width from CPUID
> > 0x80000008.  Meanwhile, even allow kvm_get_supported_cpuid_index() to
> > fail.  I don't know whether that could be useful because I think
> > 0x80000008 should be there for all x86_64 hosts, but I also think it's
> > not really helpful to assert in the kvm_get_supported_cpuid_index().
> [...]
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > index 6cb34a0fa200..9de2fd310ac8 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > @@ -760,9 +760,6 @@ kvm_get_supported_cpuid_index(uint32_t function, uint32_t index)
> >  			break;
> >  		}
> >  	}
> > -
> > -	TEST_ASSERT(entry, "Guest CPUID entry not found: (EAX=%x, ECX=%x).",
> > -		    function, index);
> >  	return entry;
> >  }
> 
> You should also adjust the comment of the function. It currently says
> "Never returns NULL". Not it can return NULL.

Yeh that's better.

> 
> And maybe add a TEST_ASSERT() to the other callers instead, which do not
> expect a NULL to be returned?

I think it's fine because it's the same as moving the assert from here
to the callers because when the caller uses entry->xxx it'll assert. :)

Thanks,

-- 
Peter Xu