From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Jones Subject: Re: [kvm-unit-tests PATCH 11/14] pci: edu: introduce pci-edu helpers Date: Tue, 25 Oct 2016 12:43:41 +0200 Message-ID: <20161025104341.rvyr4eo7grteoq3j@kamzik.brq.redhat.com> References: <1476448852-30062-1-git-send-email-peterx@redhat.com> <1476448852-30062-12-git-send-email-peterx@redhat.com> <20161020131928.shuxwj7u7wyctoy3@kamzik.brq.redhat.com> <20161025033422.GA15168@pxdev.xzpeter.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, jan.kiszka@web.de, agordeev@redhat.com, rkrcmar@redhat.com, pbonzini@redhat.com To: Peter Xu Return-path: Received: from mx1.redhat.com ([209.132.183.28]:47780 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753152AbcJYKnp (ORCPT ); Tue, 25 Oct 2016 06:43:45 -0400 Content-Disposition: inline In-Reply-To: <20161025033422.GA15168@pxdev.xzpeter.org> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Oct 25, 2016 at 11:34:22AM +0800, Peter Xu wrote: > > You may consider adding a cpu_relax() to lib/x86/asm/barrier.h, like > > arm and ppc have. I noticed a big difference when running with tcg > > on how fast a 'while (state-not-changed);' loop can complete when > > adding that barrier. > > Should I just do: > > #include > > in x86/asm/barrier.h, just like ppc? Nope. See below > > Btw, could you elaborate what would be the difference? I see > cpu_relax() is defined as: > > #define cpu_relax() asm volatile("" : : : "memory") > > Then how would the two differ: > > (1) while (xxx); > (2) while (xxx) cpu_relax(); It inserts a compiler-generated memory barrier, which for ARM TCG helps relinquish some time to QEMU, allowing QEMU to do what it needs to do in order for the condition to be fulfilled. I just checked Linux code for x86's definition of cpu_relax(), though, and see that it's /* REP NOP (PAUSE) is a good thing to insert into busy-wait loops. */ static __always_inline void rep_nop(void) { asm volatile("rep; nop" ::: "memory"); } static __always_inline void cpu_relax(void) { rep_nop(); } So that may work better for you. > > (I thought they are still the same? maybe I should assume variables in > xxx should have volatile keywords) Don't assume they do, make sure they do. Without volatile for those variables you may never stop looping. > > > > > > +} > > > diff --git a/lib/pci-edu.h b/lib/pci-edu.h > > > new file mode 100644 > > > index 0000000..6b7dbfd > > > --- /dev/null > > > +++ b/lib/pci-edu.h > > > @@ -0,0 +1,29 @@ > > > +/* > > > + * Edu PCI device header. > > > + * > > > + * Copyright (C) 2016 Red Hat, Inc. > > > + * > > > + * Authors: > > > + * Peter Xu , > > > + * > > > + * This work is licensed under the terms of the GNU LGPL, version 2 or > > > + * later. > > > + * > > > + * Edu device is a virtualized device in QEMU. Please refer to > > > + * docs/specs/edu.txt in QEMU repository for EDU device manual. > > > + */ > > > +#ifndef __PCI_EDU_H__ > > > +#define __PCI_EDU_H__ > > > + > > > +#include "pci.h" > > > + > > > +struct pci_edu_dev { > > > + pci_dev pci_dev; > > > +}; > > > +typedef struct pci_edu_dev pci_edu_dev_t; > > > > Why wrap pci_dev in this pci_edu_dev struct? Will there ever > > be more state? Should we just wait and see? Also, why use a > > typedef for pci_dev (assuming we want it)? 'struct pci_dev' > > would be more kernel like. > > My own preference is to use typedef just like how QEMU is using it, to > avoid typing "struct", and with more flexibility (e.g., I can just > change a struct into union without touching other part of code, as > long as I keep all the existing fields' name there). However I think I > should follow how kvm-unit-tests/Linux's coding style to use struct. I would. Note, the kernel tends to wrap unions in structs, avoiding a need to switch the outer type. > > But should I still keep the wrapper even if there is only one field > which is "struct pci_dev"? Benefits: > > (1) people won't be able to edu_dma() a PCI device which is not a edu > device, or say "struct pci_edu_dev" type is checked during > compilation. > > (2) in case edu device will need more fields, we won't need to touch > everywhere and change the old "struct pci_dev" into "struct > pci_edu_dev". Fair enough, but I think we need to decide if we even need struct pci_dev first :-) There's no need to introduce any structs if all we really need to do is pass around devid. Thanks, drew