Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 20 Sep 2022 19:51:11 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Gautam Menghani <gautammenghani201@gmail.com>
Cc:     pbonzini@redhat.com, shuah@kernel.org, guang.zeng@intel.com,
        kvm@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org, skhan@linuxfoundation.org,
        linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH] selftests/kvm: Use num_vcpus when testing ICR in the
 xapic_state_test
Message-ID: <YyoZr9rXSSMEtdh5@google.com>
References: <20220903073901.73862-1-gautammenghani201@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220903073901.73862-1-gautammenghani201@gmail.com>
Precedence: bulk

On Sat, Sep 03, 2022, Gautam Menghani wrote:
> A TODO in xapic_state_test asks to use number of vCPUs instead of
> vcpu.id + 1 in test_icr(). This patch adds support to get the number 
> of vCPUs from the VM created and use it.
> 
> Signed-off-by: Gautam Menghani <gautammenghani201@gmail.com>
> ---
>  .../selftests/kvm/x86_64/xapic_state_test.c     | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/xapic_state_test.c b/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
> index 6f7a5ef66718..de934e8e5e41 100644
> --- a/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
> @@ -96,9 +96,8 @@ static void __test_icr(struct xapic_vcpu *x, uint64_t val)
>  	____test_icr(x, val & ~(u64)APIC_ICR_BUSY);
>  }
>  
> -static void test_icr(struct xapic_vcpu *x)
> +static void test_icr(struct xapic_vcpu *x, int num_vcpus)
>  {
> -	struct kvm_vcpu *vcpu = x->vcpu;
>  	uint64_t icr, i, j;
>  
>  	icr = APIC_DEST_SELF | APIC_INT_ASSERT | APIC_DM_FIXED;
> @@ -110,11 +109,11 @@ static void test_icr(struct xapic_vcpu *x)
>  		__test_icr(x, icr | i);
>  
>  	/*
> -	 * Send all flavors of IPIs to non-existent vCPUs.  TODO: use number of
> -	 * vCPUs, not vcpu.id + 1.  Arbitrarily use vector 0xff.
> +	 * Send all flavors of IPIs to non-existent vCPUs. Arbitrarily use vector 0xff.
>  	 */
> +
>  	icr = APIC_INT_ASSERT | 0xff;
> -	for (i = vcpu->id + 1; i < 0xff; i++) {
> +	for (i = num_vcpus; i < 0xff; i++) {

Hrm, this is a bad TODO, which is 100% my fault.  I think past me wanted to play
nice with the possibility of creating multiple vCPUs, but using the number of
vCPUs to do that is the wrong approach as it makes assumptions on the vcpu_id.
One of the goals of the refactoring that led to vm_create_with_one_vcpu() was
to have tests stop making assumptions about vCPU IDs.

What would make sense though is to not assume vcpu_id == 0, i.e.

	if (i = 0; i < 0xff; i++) {
		if (i == vcpu->id)
			continue;

		for (j = 0; j < 8; j++)
			__test_icr(x, i << (32 + 24) | icr | (j << 8));

	}

>  		for (j = 0; j < 8; j++)
>  			__test_icr(x, i << (32 + 24) | icr | (j << 8));
>  	}
> @@ -137,9 +136,13 @@ int main(int argc, char *argv[])
>  		.is_x2apic = true,
>  	};
>  	struct kvm_vm *vm;
> +	struct list_head *iter;
> +	int nr_vcpus_created = 0;
>  
>  	vm = vm_create_with_one_vcpu(&x.vcpu, x2apic_guest_code);
> -	test_icr(&x);
> +	list_for_each(iter, &vm->vcpus)
> +		nr_vcpus_created++;
> +	test_icr(&x, nr_vcpus_created);

Computing the number of vCPUs created is rather silly, the test _knows_ that it
has created exacty one vCPU.  Should be a moot point though.