On Wed, Oct 26, 2016 at 03:47:09PM +0800, Peter Xu wrote:
> To extend current PCI framework, we need a per-device struct to store
> device specific information. Time to have a pci_dev struct. Most of the
> current PCI APIs are converted to use this pci_dev object as the first
> argument. Currently it only contains one field "pci_bdf", which is the
> bdf of current device.
>
> For a few APIs like pci_config_*() ops, I kept the old interface (use
> PCI BDF value rather than "struct pci_dev") since they can be used in a
> open context that without any specific PCI device.
>
> Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> ---
> lib/pci-host-generic.c | 13 ++++++----
> lib/pci-testdev.c | 13 +++++-----
> lib/pci.c | 64 ++++++++++++++++++++++++++++++--------------------
> lib/pci.h | 24 ++++++++++++-------
> x86/vmexit.c | 18 +++++++-------
> 5 files changed, 80 insertions(+), 52 deletions(-)
>
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index 96f4b4b..be3f9e7 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -194,6 +194,7 @@ static bool pci_alloc_resource(u64 *addr, u32 bar, u64 size)
>
> bool pci_probe(void)
> {
> + struct pci_dev pci_dev;
> pcidevaddr_t dev;
> u8 header;
> u32 cmd;
> @@ -208,6 +209,8 @@ bool pci_probe(void)
> if (!pci_dev_exists(dev))
> continue;
>
> + pci_dev_init(&pci_dev, dev);
> +
> /* We are only interested in normal PCI devices */
> header = pci_config_readb(dev, PCI_HEADER_TYPE);
> if ((header & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_NORMAL)
> @@ -219,21 +222,21 @@ bool pci_probe(void)
> u64 addr, size;
> u32 bar;
>
> - size = pci_bar_size(dev, i);
> + size = pci_bar_size(&pci_dev, i);
> if (!size)
> continue;
>
> - bar = pci_bar_get(dev, i);
> + bar = pci_bar_get(&pci_dev, i);
> if (pci_alloc_resource(&addr, bar, size)) {
> - pci_bar_set_addr(dev, i, addr);
> + pci_bar_set_addr(&pci_dev, i, addr);
>
> - if (pci_bar_is_memory(dev, i))
> + if (pci_bar_is_memory(&pci_dev, i))
> cmd |= PCI_COMMAND_MEMORY;
> else
> cmd |= PCI_COMMAND_IO;
> }
>
> - if (pci_bar_is64(dev, i))
> + if (pci_bar_is64(&pci_dev, i))
> i++;
> }
>
> diff --git a/lib/pci-testdev.c b/lib/pci-testdev.c
> index ad482d3..caa31bd 100644
> --- a/lib/pci-testdev.c
> +++ b/lib/pci-testdev.c
> @@ -163,26 +163,27 @@ static int pci_testdev_all(struct pci_test_dev_hdr *test,
>
> int pci_testdev(void)
> {
> + struct pci_dev pci_dev;
> phys_addr_t addr;
> void __iomem *mem, *io;
> - pcidevaddr_t dev;
> int nr_tests = 0;
> bool ret;
>
> - dev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
> - if (dev == PCIDEVADDR_INVALID) {
> + ret = pci_find_dev(&pci_dev, PCI_VENDOR_ID_REDHAT,
> + PCI_DEVICE_ID_REDHAT_TEST);
> + if (ret == -1) {
> printf("'pci-testdev' device is not found, "
> "check QEMU '-device pci-testdev' parameter\n");
> return -1;
> }
>
> - ret = pci_bar_is_valid(dev, 0) && pci_bar_is_valid(dev, 1);
> + ret = pci_bar_is_valid(&pci_dev, 0) && pci_bar_is_valid(&pci_dev, 1);
> assert(ret);
>
> - addr = pci_bar_get_addr(dev, 0);
> + addr = pci_bar_get_addr(&pci_dev, 0);
> mem = ioremap(addr, PAGE_SIZE);
>
> - addr = pci_bar_get_addr(dev, 1);
> + addr = pci_bar_get_addr(&pci_dev, 1);
> io = (void *)(unsigned long)addr;
>
> nr_tests += pci_testdev_all(mem, &pci_testdev_mem_ops);
> diff --git a/lib/pci.c b/lib/pci.c
> index 2dbbba4..bb41e0f 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -13,18 +13,26 @@ bool pci_dev_exists(pcidevaddr_t dev)
> pci_config_readw(dev, PCI_DEVICE_ID) != 0xffff);
> }
>
> +void pci_dev_init(struct pci_dev *dev, pcidevaddr_t bdf)
> +{
> + memset(dev, 0, sizeof(*dev));
> + dev->pci_bdf = bdf;
Hmm, bdf means bus-device-function, correct? We're not passing a "bdf"
in as the second argument to this function though. We're only passing
devid, assuming bus=0, fn=0. If you'd prefer we stop the bus,fn zero
assumption, then I think a precursor patch to this should be to change
our current handle type, pcidevaddr_t, to a "bdf_t".
> +}
> +
> /* Scan bus look for a specific device. Only bus 0 scanned for now. */
> -pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
> +int pci_find_dev(struct pci_dev *pci_dev, uint16_t vendor_id, uint16_t device_id)
> {
> pcidevaddr_t dev;
>
> - for (dev = 0; dev < 256; ++dev) {
> + for (dev = 0; dev < PCI_DEVFN_MAX; ++dev) {
Why introduce this PCI_DEVFN_MAX define?
> if (pci_config_readw(dev, PCI_VENDOR_ID) == vendor_id &&
> - pci_config_readw(dev, PCI_DEVICE_ID) == device_id)
> - return dev;
> + pci_config_readw(dev, PCI_DEVICE_ID) == device_id) {
> + pci_dev_init(pci_dev, dev);
> + return 0;
> + }
> }
>
> - return PCIDEVADDR_INVALID;
> + return -1;
Why not use bool for the ret type; true=good, false=bad?
> }
>
> uint32_t pci_bar_mask(uint32_t bar)
> @@ -33,12 +41,13 @@ uint32_t pci_bar_mask(uint32_t bar)
> PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
> }
>
> -uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
> +uint32_t pci_bar_get(struct pci_dev *dev, int bar_num)
> {
> - return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> + return pci_config_readl(dev->pci_bdf, PCI_BASE_ADDRESS_0 +
> + bar_num * 4);
> }
>
> -phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
> +phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num)
> {
> uint32_t bar = pci_bar_get(dev, bar_num);
> uint32_t mask = pci_bar_mask(bar);
> @@ -47,17 +56,18 @@ phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
> if (pci_bar_is64(dev, bar_num))
> addr |= (uint64_t)pci_bar_get(dev, bar_num + 1) << 32;
>
> - return pci_translate_addr(dev, addr);
> + return pci_translate_addr(dev->pci_bdf, addr);
> }
>
> -void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
> +void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr)
> {
> int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
>
> - pci_config_writel(dev, off, (uint32_t)addr);
> + pci_config_writel(dev->pci_bdf, off, (uint32_t)addr);
>
> if (pci_bar_is64(dev, bar_num))
> - pci_config_writel(dev, off + 4, (uint32_t)(addr >> 32));
> + pci_config_writel(dev->pci_bdf, off + 4,
> + (uint32_t)(addr >> 32));
> }
>
> /*
> @@ -70,20 +80,21 @@ void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
> * The following pci_bar_size_helper() and pci_bar_size() functions
> * implement the algorithm.
> */
> -static uint32_t pci_bar_size_helper(pcidevaddr_t dev, int bar_num)
> +static uint32_t pci_bar_size_helper(struct pci_dev *dev, int bar_num)
> {
> int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
> + uint16_t bdf = dev->pci_bdf;
> uint32_t bar, val;
>
> - bar = pci_config_readl(dev, off);
> - pci_config_writel(dev, off, ~0u);
> - val = pci_config_readl(dev, off);
> - pci_config_writel(dev, off, bar);
> + bar = pci_config_readl(bdf, off);
> + pci_config_writel(bdf, off, ~0u);
> + val = pci_config_readl(bdf, off);
> + pci_config_writel(bdf, off, bar);
>
> return val;
> }
>
> -phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> +phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num)
> {
> uint32_t bar, size;
>
> @@ -104,19 +115,19 @@ phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
> }
> }
>
> -bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
> +bool pci_bar_is_memory(struct pci_dev *dev, int bar_num)
> {
> uint32_t bar = pci_bar_get(dev, bar_num);
>
> return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
> }
>
> -bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
> +bool pci_bar_is_valid(struct pci_dev *dev, int bar_num)
> {
> return pci_bar_get(dev, bar_num);
> }
>
> -bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
> +bool pci_bar_is64(struct pci_dev *dev, int bar_num)
> {
> uint32_t bar = pci_bar_get(dev, bar_num);
>
> @@ -135,6 +146,7 @@ static void pci_dev_print(pcidevaddr_t dev)
> uint8_t progif = pci_config_readb(dev, PCI_CLASS_PROG);
> uint8_t subclass = pci_config_readb(dev, PCI_CLASS_DEVICE);
> uint8_t class = pci_config_readb(dev, PCI_CLASS_DEVICE + 1);
> + struct pci_dev pci_dev;
> int i;
>
> printf("dev %2d fn %d vendor_id %04x device_id %04x type %02x "
> @@ -145,18 +157,20 @@ static void pci_dev_print(pcidevaddr_t dev)
> if ((header & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_NORMAL)
> return;
>
> + pci_dev_init(&pci_dev, dev);
> +
> for (i = 0; i < 6; i++) {
> phys_addr_t size, start, end;
> uint32_t bar;
>
> - size = pci_bar_size(dev, i);
> + size = pci_bar_size(&pci_dev, i);
> if (!size)
> continue;
>
> - start = pci_bar_get_addr(dev, i);
> + start = pci_bar_get_addr(&pci_dev, i);
> end = start + size - 1;
>
> - if (pci_bar_is64(dev, i)) {
> + if (pci_bar_is64(&pci_dev, i)) {
> printf("\tBAR#%d,%d [%" PRIx64 "-%" PRIx64 " ",
> i, i + 1, start, end);
> i++;
> @@ -165,7 +179,7 @@ static void pci_dev_print(pcidevaddr_t dev)
> i, (uint32_t)start, (uint32_t)end);
> }
>
> - bar = pci_bar_get(dev, i);
> + bar = pci_bar_get(&pci_dev, i);
>
> if (bar & PCI_BASE_ADDRESS_SPACE_IO) {
> printf("PIO]\n");
> diff --git a/lib/pci.h b/lib/pci.h
> index 40e11a8..af20dc5 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -15,10 +15,18 @@ enum {
> PCIDEVADDR_INVALID = 0xffff,
> };
>
> +#define PCI_DEVFN_MAX (256)
> +
> +struct pci_dev {
> + uint16_t pci_bdf;
No need for 'pci_' in the bdf name. It's whole name already has it,
pci_dev.bdf
> +};
> +
> +void pci_dev_init(struct pci_dev *dev, pcidevaddr_t bdf);
> +
> extern bool pci_probe(void);
> extern void pci_print(void);
> extern bool pci_dev_exists(pcidevaddr_t dev);
> -extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> +extern int pci_find_dev(struct pci_dev *dev, uint16_t vendor_id, uint16_t device_id);
>
> /*
> * @bar_num in all BAR access functions below is the index of the 32-bit
> @@ -32,14 +40,14 @@ extern pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> * It is expected the caller is aware of the device BAR layout and never
> * tries to address the middle of a 64-bit register.
> */
> -extern phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num);
> -extern void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr);
> -extern phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
> -extern uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num);
> +extern phys_addr_t pci_bar_get_addr(struct pci_dev *dev, int bar_num);
> +extern void pci_bar_set_addr(struct pci_dev *dev, int bar_num, phys_addr_t addr);
> +extern phys_addr_t pci_bar_size(struct pci_dev *dev, int bar_num);
> +extern uint32_t pci_bar_get(struct pci_dev *dev, int bar_num);
> extern uint32_t pci_bar_mask(uint32_t bar);
> -extern bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
> -extern bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
> -extern bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> +extern bool pci_bar_is64(struct pci_dev *dev, int bar_num);
> +extern bool pci_bar_is_memory(struct pci_dev *dev, int bar_num);
> +extern bool pci_bar_is_valid(struct pci_dev *dev, int bar_num);
>
> int pci_testdev(void);
>
> diff --git a/x86/vmexit.c b/x86/vmexit.c
> index 2d99d5f..2736ab8 100644
> --- a/x86/vmexit.c
> +++ b/x86/vmexit.c
> @@ -372,7 +372,8 @@ int main(int ac, char **av)
> struct fadt_descriptor_rev1 *fadt;
> int i;
> unsigned long membar = 0;
> - pcidevaddr_t pcidev;
> + struct pci_dev pcidev;
> + int ret;
>
> smp_init();
> setup_vm();
> @@ -385,21 +386,22 @@ int main(int ac, char **av)
> pm_tmr_blk = fadt->pm_tmr_blk;
> printf("PM timer port is %x\n", pm_tmr_blk);
>
> - pcidev = pci_find_dev(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_TEST);
> - if (pcidev != PCIDEVADDR_INVALID) {
> + ret = pci_find_dev(&pcidev, PCI_VENDOR_ID_REDHAT,
> + PCI_DEVICE_ID_REDHAT_TEST);
> + if (ret == 0) {
> for (i = 0; i < PCI_TESTDEV_NUM_BARS; i++) {
> - if (!pci_bar_is_valid(pcidev, i)) {
> + if (!pci_bar_is_valid(&pcidev, i)) {
> continue;
> }
> - if (pci_bar_is_memory(pcidev, i)) {
> - membar = pci_bar_get_addr(pcidev, i);
> + if (pci_bar_is_memory(&pcidev, i)) {
> + membar = pci_bar_get_addr(&pcidev, i);
> pci_test.memaddr = ioremap(membar, PAGE_SIZE);
> } else {
> - pci_test.iobar = pci_bar_get_addr(pcidev, i);
> + pci_test.iobar = pci_bar_get_addr(&pcidev, i);
> }
> }
> printf("pci-testdev at 0x%x membar %lx iobar %x\n",
> - pcidev, membar, pci_test.iobar);
> + pcidev.pci_bdf, membar, pci_test.iobar);
> }
>
> for (i = 0; i < ARRAY_SIZE(tests); ++i)
> --
> 2.7.4
I'm OK with introducing the pci_dev struct, but let's not switch over to
it from our other handle for everything all at once. I'd rather only
switch functions that will need more state than just devid/bdf. Anywhere
we never do then I think expecting the caller to call like
pci_foo(pcidev->bdf, ...) is reasonable.
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
