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;
From:   "Wang, Wei W" <wei.w.wang@intel.com>
To:     "Christopherson,, Sean" <seanjc@google.com>
CC:     "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "dmatlack@google.com" <dmatlack@google.com>,
        "vipinsh@google.com" <vipinsh@google.com>,
        "ajones@ventanamicro.com" <ajones@ventanamicro.com>,
        "eric.auger@redhat.com" <eric.auger@redhat.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v1 01/18] KVM: selftests/kvm_util: use array of pointers
 to maintain vcpus in kvm_vm
Thread-Topic: [PATCH v1 01/18] KVM: selftests/kvm_util: use array of pointers
 to maintain vcpus in kvm_vm
Thread-Index: AQHY55ylKGIU2b/Czky3MagHOAXjm64hW4iAgAB9rdA=
Date:   Thu, 27 Oct 2022 12:28:31 +0000
Message-ID: <DS0PR11MB6373E471C1378CBEEBD40A82DC339@DS0PR11MB6373.namprd11.prod.outlook.com>
References: <20221024113445.1022147-1-wei.w.wang@intel.com>
 <20221024113445.1022147-2-wei.w.wang@intel.com> <Y1nHL5BUoWPqUtt9@google.com>
In-Reply-To: <Y1nHL5BUoWPqUtt9@google.com>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.6.500.17
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?t08JEqmEbolPYgaVG2GI4yiEPeQQ1GeXefo/sNwr9B56jCM5yDaPTnqARO0b?=
 =?us-ascii?Q?kqHPFM/BVcjsStH/lq5X5SzWIL+ZoEKDKiGzka7io0S+onodIUqz6t3E7FD0?=
 =?us-ascii?Q?1xpyVNnpxVr1JOmXBrzc16FgeTAX+59oqu3t5yogKn54dUq5P7M7CSR4tARY?=
 =?us-ascii?Q?v8qR3B4/LLntI5H8XP52AYhHFfPGH6BhI7qCdRkWTfJjMS9AsXqn0El4/ptx?=
 =?us-ascii?Q?OxTW86WLiCwOvCwgG+0IcRbUqQwZKx67Uu5UuSvc5nQELmMPFSQXNy4xYxVk?=
 =?us-ascii?Q?HFUI9Ofis+IwTw69SspVNCU/I1oMnaha/itLyUI/ArGdajy6muq8Juqz9K3M?=
 =?us-ascii?Q?TLPjqSQdHc4nfEFsf+YLiiZ6Xu0Zxd/IXGy+0QIUvGchC5+KxWFrSitLeFma?=
 =?us-ascii?Q?9c5udobHQnb3ZH17dOo7gt6ytGXt/5jwpa0UJ+gBdk80Zo7X75ZvXDaRAvLi?=
 =?us-ascii?Q?n9CbXOUHZ3Pe9CeD+b8s6XBQ7iuXzMvH4tfp76LHHpdXUEGkG3XWbrP4ZpGC?=
 =?us-ascii?Q?PWMbwfZh73hHmf6FyKfxcNwnwnJnSiPrqF7tXDkjupbH0YtX7dmgNALzQyUP?=
 =?us-ascii?Q?KfDQMwYwlQlEtunATGgkPr0mGrtsY/MsoJ/G23vBqXQYQS/KIIRxmpWwUIoe?=
 =?us-ascii?Q?dhv/D2kopXorsvvFO0EGciDOOAeSt+1v/0YstQmGbz7Pk1Xa16aiVs2tFMMt?=
 =?us-ascii?Q?XLaD0Gkyp9aJHoUpilvPK3ws8BdP69paP+Bn7EWDWgRAeN3fTICB7k9jeyiP?=
 =?us-ascii?Q?x42zGUks9cLX+mEjYv8Irz9FDEZlstamvmDQ1Zj1DSTaWlV9qeOsMVLpeQ+P?=
 =?us-ascii?Q?ggF4t1TXBww5d5hlxhasFbFhAitS3qS0zAUcz+DQ/X1MblQnyiqtDWP+i6sf?=
 =?us-ascii?Q?L8ElX8Vf0StWzMvsx8P6cN8MPTtNEAMRcu1lfkXbdbE0e6/D2ll4rg+w+Mh0?=
 =?us-ascii?Q?RCVcZFFsCqrQrZNeRnT4Y1POLdtayRboSUqiT6JZCcAAnvC6spPISDRK1OxW?=
 =?us-ascii?Q?HFZOd939qR4oELVD1InnUvrzToZlRXkfAvyEX0urOdp6aPt0i8A6RxUKzdG1?=
 =?us-ascii?Q?YRs3daqowSzzhhMSsa68648kbrg7p7JYwbDLVlRMuF6Py++GbJu2jy1GYXm5?=
 =?us-ascii?Q?oiAPYjyJfy65eIOye6UWr2Aty29o63RpYtF/edxLSWAZlcwOfTTKtq8nNz0N?=
 =?us-ascii?Q?BjQVQIM2K1jjxloZiFpT0NElvOSNmWCSryNF5IjZvyPm4F27h6ta466KjcNd?=
 =?us-ascii?Q?zjHzAWD4NIkR3rwubMPAs59lnI/rw1V7fYlN2IUvb5DNur8jnc1GMQqOkEUf?=
 =?us-ascii?Q?WhLx5IwVjRfXoaxM5yaBJp2P4XRX4TMEv352b5V1XiMqy6FFpiwAdLfky6Ev?=
 =?us-ascii?Q?pL2CGZV82DxO5SvQ73jdbprd58+c7IXvCN/oea65L3qdebCfQPeHmYZQFfR7?=
 =?us-ascii?Q?ag1ugRJOsI157LDu2lQSCygUVy/fwVk/q45tRFOg3qXJjg+HlSu81yOMQ0iU?=
 =?us-ascii?Q?n6TmsY9/k6/j2HG4MnMSyPZ2ouTBpfYepGFIKoKFBVwRf4mZkOwBEjiLckoI?=
 =?us-ascii?Q?XspB3AntxhqDRZFAgRWgoSZlozBH3V1KzxbDyHNy?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB6373.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: edc025f7-e201-4631-5eea-08dab816c217
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Oct 2022 12:28:31.0979
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: g3xXbLiVWLh5sR8uRnNSm+UxX7ey9xWY1klxnFzXOsbbGDYiH2RY9UHSxWewPetp3YUWDC5wMy4jD8zRwrYNBQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB7471
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thursday, October 27, 2022 7:48 AM, Sean Christopherson wrote:
> > +	for (i =3D 0, vcpu =3D vm->vcpus[0];				\
> > +		vcpu && i < KVM_MAX_VCPUS; vcpu =3D vm->vcpus[++i])
>=20
> I hate pointer arithmetic more than most people, but in this case it avoi=
ds the
> need to pass in 'i', which in turn cuts down on boilerplate and churn.

Hmm, indeed, this can be improved, how about this one:

+#define vm_iterate_over_vcpus(vm, vcpu)                         \
+       for (vcpu =3D vm->vcpus[0]; vcpu; vcpu =3D vm->vcpus[vcpu->id + 1])=
 \


>=20
> >  #endif /* SELFTEST_KVM_UTIL_H */
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > index e42a09cd24a0..c90a9609b853 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > @@ -45,7 +45,6 @@ struct userspace_mem_region {  };
> >
> >  struct kvm_vcpu {
> > -	struct list_head list;
> >  	uint32_t id;
> >  	int fd;
> >  	struct kvm_vm *vm;
> > @@ -75,7 +74,6 @@ struct kvm_vm {
> >  	unsigned int pa_bits;
> >  	unsigned int va_bits;
> >  	uint64_t max_gfn;
> > -	struct list_head vcpus;
> >  	struct userspace_mem_regions regions;
> >  	struct sparsebit *vpages_valid;
> >  	struct sparsebit *vpages_mapped;
> > @@ -92,6 +90,7 @@ struct kvm_vm {
> >  	int stats_fd;
> >  	struct kvm_stats_header stats_header;
> >  	struct kvm_stats_desc *stats_desc;
> > +	struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
>=20
> We can dynamically allocate the array without too much trouble, though I'=
m not
> sure it's worth shaving the few KiB of memory.  For __vm_create(), the nu=
mber
> of vCPUs is known when the VM is created.  For vm_create_barebones(), we
> could do the simple thing of allocating KVM_MAX_VCPU.

The issue with dynamic allocation is that some users start with __vm_create=
(nr_vcpus), and later
could add more cpus with vm_vcpu_add (e.g. x86_64/xapic_ipi_test.c). To sup=
port this we may
need to re-allocate the array for later vm_vcpu_add(), and also need to add=
 nr_vcpus to indicate
the size.
It's userspace memory, and not a problem to use a bit larger virtual memory=
 (memory are not really
allocated until we have that many vcpus to touch the array entries), I thin=
k.

>=20
> > @@ -534,6 +533,10 @@ __weak void vcpu_arch_free(struct kvm_vcpu *vcpu)
> > static void vm_vcpu_rm(struct kvm_vm *vm, struct kvm_vcpu *vcpu)  {
> >  	int ret;
> > +	uint32_t vcpu_id =3D vcpu->id;
> > +
> > +	TEST_ASSERT(!!vm->vcpus[vcpu_id], "vCPU%d wasn't added\n", vcpu_id);
>=20
> This is unecessary, there's one caller and it's iterating over the array =
of vCPUs.

That's right, thanks.