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 <asm-generic/barrier.h>
>
> 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 <peterx@xxxxxxxxxx>,
> > > + *
> > > + * 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
