On Thu, Oct 20, 2016 at 03:19:28PM +0200, Andrew Jones wrote:
[...]
> > +#include "pci-edu.h"
> > +
> > +#define PCI_VENDOR_ID_QEMU (0x1234)
> > +#define PCI_DEVICE_ID_EDU (0x11e8)
> > +
> > +/* The only bar used by EDU device */
> > +#define EDU_BAR_MEM (0)
> > +#define EDU_MAGIC (0xed)
> > +#define EDU_VERSION (0x100)
> > +#define EDU_DMA_BUF_SIZE (1 << 20)
> > +#define EDU_INPUT_BUF_SIZE (256)
> > +
> > +#define EDU_REG_ID (0x0)
> > +#define EDU_REG_ALIVE (0x4)
> > +#define EDU_REG_FACTORIAL (0x8)
> > +#define EDU_REG_STATUS (0x20)
> > +#define EDU_REG_DMA_SRC (0x80)
> > +#define EDU_REG_DMA_DST (0x88)
> > +#define EDU_REG_DMA_COUNT (0x90)
> > +#define EDU_REG_DMA_CMD (0x98)
> > +
> > +#define EDU_CMD_DMA_START (0x01)
> > +#define EDU_CMD_DMA_FROM (0x02)
> > +#define EDU_CMD_DMA_TO (0x00)
> > +
> > +#define EDU_STATUS_FACTORIAL (0x1)
> > +#define EDU_STATUS_INT_ENABLE (0x80)
> > +
> > +#define EDU_DMA_START (0x40000)
> > +#define EDU_DMA_SIZE_MAX (4096)
>
> shouldn't the above defines be in the header?
I put them here since these are macros that I think will only be used
by this file only. However I think it'll still be okay to move them
into header files, so I'll do that.
>
> > +
> > +uint64_t edu_reg_readq(pci_edu_dev_t *dev, int reg)
> > +{
> > + return *(volatile uint64_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg);
> > +}
> > +
> > +uint32_t edu_reg_read(pci_edu_dev_t *dev, int reg)
> > +{
> > + return *(volatile uint32_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg);
> > +}
> > +
> > +void edu_reg_writeq(pci_edu_dev_t *dev, int reg, uint64_t val)
> > +{
> > + *(volatile uint64_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg) = val;
> > +}
> > +
> > +void edu_reg_write(pci_edu_dev_t *dev, int reg, uint32_t val)
> > +{
> > + *(volatile uint32_t *)(dev->pci_dev.pci_bar[EDU_BAR_MEM] + reg) = val;
> > +}
>
> I'd put these accessors in the header as static inlines too
Sure. Will do.
>
> > +
> > +/* Return true if alive */
> > +bool edu_check_alive(pci_edu_dev_t *dev)
> > +{
> > + static uint32_t live_count = 1;
> > + uint32_t value;
> > +
> > + edu_reg_write(dev, EDU_REG_ALIVE, live_count++);
> > + value = edu_reg_read(dev, EDU_REG_ALIVE);
> > + return (live_count - 1 == ~value);
> > +}
>
> Even edu_check_alive could be a static inline in the header,
> if it's something you'll do frequently. If it's only needed
> for a sanity init test then I'd just inline it directly in
> the one caller, edu_init
Will do.
>
> > +
> > +int edu_init(pci_edu_dev_t *dev)
> > +{
> > + int ret;
> > +
> > + ret = pci_dev_init(&dev->pci_dev, PCI_VENDOR_ID_QEMU,
> > + PCI_DEVICE_ID_EDU);
> > + if (ret)
> > + return ret;
> > +
> > + if (!edu_check_alive(dev)) {
> > + printf("edu device not alive!\n");
> > + return -1;
>
> should this ever fail? Or would an assert by fine here?
> alive = edu_check_alive(dev)
> assert(alive);
Yep. It should not fail. Will take assert.
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +uint32_t edu_status(pci_edu_dev_t *dev)
> > +{
> > + return edu_reg_read(dev, EDU_REG_STATUS);
> > +}
>
> please, no unnecessary wrappers
Ok.
>
> > +
> > +void edu_dma(pci_edu_dev_t *dev, iova_t iova,
> > + size_t size, int dev_offset, pci_dma_dir_t dir)
> > +{
> > + uint64_t from, to;
> > + uint32_t cmd = EDU_CMD_DMA_START;
> > +
> > + assert(size <= EDU_DMA_SIZE_MAX);
> > + assert(dev_offset < EDU_DMA_SIZE_MAX &&
> > + dev_offset >= 0);
> > +
> > + printf("edu device DMA start %s addr %p size 0x%lu off 0x%x\n",
> > + dir == PCI_DMA_FROM_DEVICE ? "FROM" : "TO",
> > + (void *)iova, size, dev_offset);
>
> is pci_dma_dir_t just a binary enum? If so, then I find it
> sort of crufty. Can't we just have a 'bool write'?
Will replace with "bool from_device".
>
> > +
> > + if (dir == PCI_DMA_FROM_DEVICE) {
> > + from = dev_offset + EDU_DMA_START;
> > + to = iova;
> > + cmd |= EDU_CMD_DMA_FROM;
> > + } else {
> > + from = iova;
> > + to = EDU_DMA_START + dev_offset;
> > + cmd |= EDU_CMD_DMA_TO;
> > + }
> > +
> > + edu_reg_writeq(dev, EDU_REG_DMA_SRC, from);
> > + edu_reg_writeq(dev, EDU_REG_DMA_DST, to);
> > + edu_reg_writeq(dev, EDU_REG_DMA_COUNT, size);
> > + edu_reg_write(dev, EDU_REG_DMA_CMD, cmd);
> > +
> > + /* Wait until DMA finished */
> > + while (edu_reg_read(dev, EDU_REG_DMA_CMD) & EDU_CMD_DMA_START);
>
> 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?
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();
(I thought they are still the same? maybe I should assume variables in
xxx should have volatile keywords)
>
> > +}
> > 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.
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".
Thanks!
-- peterx
--
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
