Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <184e253d-06c4-419e-b2b4-7cce1f875ba5@redhat.com>
Date:   Tue, 12 Dec 2023 11:39:38 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] KVM: selftests: fix supported_flags for aarch64
To:     Sean Christopherson <seanjc@google.com>,
        Shaoqin Huang <shahuang@redhat.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org
References: <20231208184628.2297994-1-pbonzini@redhat.com>
 <ZXPRGzgWFqFdI_ep@google.com>
Content-Language: en-US
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <ZXPRGzgWFqFdI_ep@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/9/23 03:29, Sean Christopherson wrote:
> On Fri, Dec 08, 2023, Paolo Bonzini wrote:
>> KVM/Arm supports readonly memslots; fix the calculation of
>> supported_flags in set_memory_region_test.c, otherwise the
>> test fails.
> 
> You got beat by a few hours, and by a better solution ;-)
> 
> https://lore.kernel.org/all/20231208033505.2930064-1-shahuang@redhat.com

Better but also wrong---and my patch has the debatable merit of more
clearly exposing the wrongness.  Testing individual architectures is bad,
but testing __KVM_HAVE_READONLY_MEM makes the test fail when running a new
test on an old kernel.

This scenario of course will fail when the test detects a bug, but readonly
memory is just new functionality (think of the case where RISC-V starts
defining __KVM_HAVE_READONLY_MEM in the future).  For new functionality,
the right thing to do is one of 1) skip the whole test 2) skip the individual
test case 3) code the test to adapt to the old kernel.  The third choice is
rarely possible, but this is one of the cases in which it _is_ possible.

So, the only good way to do this is to get _all_ supported_flags from
KVM_CHECK_EXTENSION(KVM_CAP_USER_MEMORY2).  We can change the value returned
by KVM_CHECK_EXTENSION because KVM_CAP_USER_MEMORY2 has not been included in
any released kernel.  Calling KVM_CHECK_EXTENSION subsumes

         supported_flags |= KVM_MEM_READONLY;
         if (kvm_check_cap(KVM_CAP_MEMORY_ATTRIBUTES) & KVM_MEMORY_ATTRIBUTE_PRIVATE)
                 supported_flags |= KVM_MEM_GUEST_MEMFD;

and v2_only_flags would be defined as

         const uint32_t v2_only_flags = ~(KVM_MEM_LOG_DIRTY_PAGES | KVM_MEM_READONLY);

(not guaranteed to work in the future, but good enough since new KVM_MEM_*
flags are a very rare occurrence).  Then, the test checks that the supported
flags are consistent with the value returned by KVM_CHECK_EXTENSION.

Shaoqin, would you give it a shot?

Thanks,

Paolo

> 

>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   tools/testing/selftests/kvm/set_memory_region_test.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
>> index 6637a0845acf..dfd1d1e22da3 100644
>> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
>> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c
>> @@ -333,9 +333,11 @@ static void test_invalid_memory_region_flags(void)
>>   	struct kvm_vm *vm;
>>   	int r, i;
>>   
>> -#ifdef __x86_64__
>> +#if defined __aarch64__ || defined __x86_64__
>>   	supported_flags |= KVM_MEM_READONLY;
>> +#endif
>>   
>> +#ifdef __x86_64__
>>   	if (kvm_check_cap(KVM_CAP_VM_TYPES) & BIT(KVM_X86_SW_PROTECTED_VM))
>>   		vm = vm_create_barebones_protected_vm();
>>   	else
>> -- 
>> 2.39.1
>>