From: kernel test robot <lkp@intel.com>
To: David Stevens <stevensd@chromium.org>,
Sean Christopherson <seanjc@google.com>,
David Woodhouse <dwmw@amazon.co.uk>
Cc: llvm@lists.linux.dev, oe-kbuild-all@lists.linux.dev,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
David Stevens <stevensd@chromium.org>
Subject: Re: [PATCH 1/3] KVM: Support sharing gpc locks
Date: Sun, 29 Jan 2023 08:46:45 +0800 [thread overview]
Message-ID: <202301290827.aRAvXuk9-lkp@intel.com> (raw)
In-Reply-To: <20230127044500.680329-2-stevensd@google.com>
Hi David,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on kvm/queue]
[also build test ERROR on linus/master v6.2-rc5 next-20230127]
[cannot apply to kvm/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/David-Stevens/KVM-Support-sharing-gpc-locks/20230128-161425
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
patch link: https://lore.kernel.org/r/20230127044500.680329-2-stevensd%40google.com
patch subject: [PATCH 1/3] KVM: Support sharing gpc locks
config: x86_64-randconfig-a014 (https://download.01.org/0day-ci/archive/20230129/202301290827.aRAvXuk9-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/14d62b317199184bb71256cc5f652b04fb2f9107
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review David-Stevens/KVM-Support-sharing-gpc-locks/20230128-161425
git checkout 14d62b317199184bb71256cc5f652b04fb2f9107
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> arch/x86/kvm/xen.c:319:31: error: member reference type 'rwlock_t *' is a pointer; did you mean to use '->'?
lock_set_subclass(gpc1->lock.dep_map, 1, _THIS_IP_);
~~~~~~~~~~^
->
>> arch/x86/kvm/xen.c:319:21: error: passing 'struct lockdep_map' to parameter of incompatible type 'struct lockdep_map *'; take the address with &
lock_set_subclass(gpc1->lock.dep_map, 1, _THIS_IP_);
^~~~~~~~~~~~~~~~~~
&
include/linux/lockdep.h:296:58: note: passing argument to parameter 'lock' here
static inline void lock_set_subclass(struct lockdep_map *lock,
^
2 errors generated.
vim +319 arch/x86/kvm/xen.c
172
173 static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
174 {
175 struct kvm_vcpu_xen *vx = &v->arch.xen;
176 struct gfn_to_pfn_cache *gpc1 = &vx->runstate_cache;
177 struct gfn_to_pfn_cache *gpc2 = &vx->runstate2_cache;
178 size_t user_len, user_len1, user_len2;
179 struct vcpu_runstate_info rs;
180 unsigned long flags;
181 size_t times_ofs;
182 uint8_t *update_bit = NULL;
183 uint64_t entry_time;
184 uint64_t *rs_times;
185 int *rs_state;
186
187 /*
188 * The only difference between 32-bit and 64-bit versions of the
189 * runstate struct is the alignment of uint64_t in 32-bit, which
190 * means that the 64-bit version has an additional 4 bytes of
191 * padding after the first field 'state'. Let's be really really
192 * paranoid about that, and matching it with our internal data
193 * structures that we memcpy into it...
194 */
195 BUILD_BUG_ON(offsetof(struct vcpu_runstate_info, state) != 0);
196 BUILD_BUG_ON(offsetof(struct compat_vcpu_runstate_info, state) != 0);
197 BUILD_BUG_ON(sizeof(struct compat_vcpu_runstate_info) != 0x2c);
198 #ifdef CONFIG_X86_64
199 /*
200 * The 64-bit structure has 4 bytes of padding before 'state_entry_time'
201 * so each subsequent field is shifted by 4, and it's 4 bytes longer.
202 */
203 BUILD_BUG_ON(offsetof(struct vcpu_runstate_info, state_entry_time) !=
204 offsetof(struct compat_vcpu_runstate_info, state_entry_time) + 4);
205 BUILD_BUG_ON(offsetof(struct vcpu_runstate_info, time) !=
206 offsetof(struct compat_vcpu_runstate_info, time) + 4);
207 BUILD_BUG_ON(sizeof(struct vcpu_runstate_info) != 0x2c + 4);
208 #endif
209 /*
210 * The state field is in the same place at the start of both structs,
211 * and is the same size (int) as vx->current_runstate.
212 */
213 BUILD_BUG_ON(offsetof(struct vcpu_runstate_info, state) !=
214 offsetof(struct compat_vcpu_runstate_info, state));
215 BUILD_BUG_ON(sizeof_field(struct vcpu_runstate_info, state) !=
216 sizeof(vx->current_runstate));
217 BUILD_BUG_ON(sizeof_field(struct compat_vcpu_runstate_info, state) !=
218 sizeof(vx->current_runstate));
219
220 /*
221 * The state_entry_time field is 64 bits in both versions, and the
222 * XEN_RUNSTATE_UPDATE flag is in the top bit, which given that x86
223 * is little-endian means that it's in the last *byte* of the word.
224 * That detail is important later.
225 */
226 BUILD_BUG_ON(sizeof_field(struct vcpu_runstate_info, state_entry_time) !=
227 sizeof(uint64_t));
228 BUILD_BUG_ON(sizeof_field(struct compat_vcpu_runstate_info, state_entry_time) !=
229 sizeof(uint64_t));
230 BUILD_BUG_ON((XEN_RUNSTATE_UPDATE >> 56) != 0x80);
231
232 /*
233 * The time array is four 64-bit quantities in both versions, matching
234 * the vx->runstate_times and immediately following state_entry_time.
235 */
236 BUILD_BUG_ON(offsetof(struct vcpu_runstate_info, state_entry_time) !=
237 offsetof(struct vcpu_runstate_info, time) - sizeof(uint64_t));
238 BUILD_BUG_ON(offsetof(struct compat_vcpu_runstate_info, state_entry_time) !=
239 offsetof(struct compat_vcpu_runstate_info, time) - sizeof(uint64_t));
240 BUILD_BUG_ON(sizeof_field(struct vcpu_runstate_info, time) !=
241 sizeof_field(struct compat_vcpu_runstate_info, time));
242 BUILD_BUG_ON(sizeof_field(struct vcpu_runstate_info, time) !=
243 sizeof(vx->runstate_times));
244
245 if (IS_ENABLED(CONFIG_64BIT) && v->kvm->arch.xen.long_mode) {
246 user_len = sizeof(struct vcpu_runstate_info);
247 times_ofs = offsetof(struct vcpu_runstate_info,
248 state_entry_time);
249 } else {
250 user_len = sizeof(struct compat_vcpu_runstate_info);
251 times_ofs = offsetof(struct compat_vcpu_runstate_info,
252 state_entry_time);
253 }
254
255 /*
256 * There are basically no alignment constraints. The guest can set it
257 * up so it crosses from one page to the next, and at arbitrary byte
258 * alignment (and the 32-bit ABI doesn't align the 64-bit integers
259 * anyway, even if the overall struct had been 64-bit aligned).
260 */
261 if ((gpc1->gpa & ~PAGE_MASK) + user_len >= PAGE_SIZE) {
262 user_len1 = PAGE_SIZE - (gpc1->gpa & ~PAGE_MASK);
263 user_len2 = user_len - user_len1;
264 } else {
265 user_len1 = user_len;
266 user_len2 = 0;
267 }
268 BUG_ON(user_len1 + user_len2 != user_len);
269
270 retry:
271 /*
272 * Attempt to obtain the GPC lock on *both* (if there are two)
273 * gfn_to_pfn caches that cover the region.
274 */
275 if (atomic) {
276 local_irq_save(flags);
277 if (!read_trylock(gpc1->lock)) {
278 local_irq_restore(flags);
279 return;
280 }
281 } else {
282 read_lock_irqsave(gpc1->lock, flags);
283 }
284 while (!kvm_gpc_check(gpc1, user_len1)) {
285 read_unlock_irqrestore(gpc1->lock, flags);
286
287 /* When invoked from kvm_sched_out() we cannot sleep */
288 if (atomic)
289 return;
290
291 if (kvm_gpc_refresh(gpc1, user_len1))
292 return;
293
294 read_lock_irqsave(gpc1->lock, flags);
295 }
296
297 if (likely(!user_len2)) {
298 /*
299 * Set up three pointers directly to the runstate_info
300 * struct in the guest (via the GPC).
301 *
302 * • @rs_state → state field
303 * • @rs_times → state_entry_time field.
304 * • @update_bit → last byte of state_entry_time, which
305 * contains the XEN_RUNSTATE_UPDATE bit.
306 */
307 rs_state = gpc1->khva;
308 rs_times = gpc1->khva + times_ofs;
309 if (v->kvm->arch.xen.runstate_update_flag)
310 update_bit = ((void *)(&rs_times[1])) - 1;
311 } else {
312 /*
313 * The guest's runstate_info is split across two pages and we
314 * need to hold and validate both GPCs simultaneously. We can
315 * declare a lock ordering GPC1 > GPC2 because nothing else
316 * takes them more than one at a time. Set a subclass on the
317 * gpc1 lock to make lockdep shut up about it.
318 */
> 319 lock_set_subclass(gpc1->lock.dep_map, 1, _THIS_IP_);
320 if (atomic) {
321 if (!read_trylock(gpc2->lock)) {
322 read_unlock_irqrestore(gpc1->lock, flags);
323 return;
324 }
325 } else {
326 read_lock(gpc2->lock);
327 }
328
329 if (!kvm_gpc_check(gpc2, user_len2)) {
330 read_unlock(gpc2->lock);
331 read_unlock_irqrestore(gpc1->lock, flags);
332
333 /* When invoked from kvm_sched_out() we cannot sleep */
334 if (atomic)
335 return;
336
337 /*
338 * Use kvm_gpc_activate() here because if the runstate
339 * area was configured in 32-bit mode and only extends
340 * to the second page now because the guest changed to
341 * 64-bit mode, the second GPC won't have been set up.
342 */
343 if (kvm_gpc_activate(gpc2, gpc1->gpa + user_len1,
344 user_len2))
345 return;
346
347 /*
348 * We dropped the lock on GPC1 so we have to go all the
349 * way back and revalidate that too.
350 */
351 goto retry;
352 }
353
354 /*
355 * In this case, the runstate_info struct will be assembled on
356 * the kernel stack (compat or not as appropriate) and will
357 * be copied to GPC1/GPC2 with a dual memcpy. Set up the three
358 * rs pointers accordingly.
359 */
360 rs_times = &rs.state_entry_time;
361
362 /*
363 * The rs_state pointer points to the start of what we'll
364 * copy to the guest, which in the case of a compat guest
365 * is the 32-bit field that the compiler thinks is padding.
366 */
367 rs_state = ((void *)rs_times) - times_ofs;
368
369 /*
370 * The update_bit is still directly in the guest memory,
371 * via one GPC or the other.
372 */
373 if (v->kvm->arch.xen.runstate_update_flag) {
374 if (user_len1 >= times_ofs + sizeof(uint64_t))
375 update_bit = gpc1->khva + times_ofs +
376 sizeof(uint64_t) - 1;
377 else
378 update_bit = gpc2->khva + times_ofs +
379 sizeof(uint64_t) - 1 - user_len1;
380 }
381
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
next prev parent reply other threads:[~2023-01-29 0:47 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-27 4:44 [PATCH 0/3] KVM: x86: replace kvm_vcpu_map usage in vmx David Stevens
2023-01-27 4:44 ` [PATCH 1/3] KVM: Support sharing gpc locks David Stevens
2023-01-28 21:42 ` kernel test robot
2023-01-29 0:46 ` kernel test robot [this message]
2023-01-29 10:08 ` David Woodhouse
2023-01-30 5:25 ` [PATCH 4/3] KVM: x86/xen: Make runstate cache gpcs share a lock David Stevens
2023-01-31 16:58 ` David Woodhouse
2023-01-27 4:44 ` [PATCH 2/3] KVM: use gfn=>pfn cache in nested_get_vmcs12_pages David Stevens
2023-01-27 4:45 ` [PATCH 3/3] KVM: use gfn=>pfn cache for evmcs David Stevens
2023-01-27 19:19 ` [PATCH 0/3] KVM: x86: replace kvm_vcpu_map usage in vmx Sean Christopherson
2023-03-06 1:45 ` David Stevens
2023-03-07 16:03 ` Sean Christopherson
2023-03-14 23:33 ` Sean Christopherson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202301290827.aRAvXuk9-lkp@intel.com \
--to=lkp@intel.com \
--cc=dwmw@amazon.co.uk \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=llvm@lists.linux.dev \
--cc=oe-kbuild-all@lists.linux.dev \
--cc=seanjc@google.com \
--cc=stevensd@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.