Re: [kvm-unit-tests PATCH 11/14] pci: edu: introduce pci-edu helpers

[Andrew Jones <drjones@redhat.com>]

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@redhat.com>,
> > > + *
> > > + * 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