On Fri, Oct 14, 2016 at 08:40:42PM +0800, Peter Xu wrote:
> pci_find_dev() is not sufficient for further tests for Intel IOMMU. This
> patch introduced pci_dev struct, as a abstract of a specific PCI device.
>
> All the rest of current PCI APIs are changed to leverage the pci_dev
> struct.
>
> x86/vmexit.c is the only user of the old PCI APIs. Changing it to use
> the new ones.
>
> (Indentation is fixed from using 4 spaces into tab in pci.c)
We need to get Alex's series in and then revisit this. He's maintained
the API's use of a devid handle. It's possible a struct would be better,
but I'm not so sure. In any case we need to break this patch up in order
to see the goals step-by-step.
1) cleanup style - Alex's series does that already
2) replace the devid handle with struct pci_dev - should be done
everywhere or nowhere
3) simplify x86/vmexit's by making better use of the API
4) extend the API for IOMMU
I've added few more comments below.
>
> Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> ---
> lib/pci.c | 75 +++++++++++++++++++++++++++++++++++++++++-------------------
> lib/pci.h | 29 ++++++++++++++++++-----
> x86/vmexit.c | 22 ++++--------------
> 3 files changed, 80 insertions(+), 46 deletions(-)
>
> diff --git a/lib/pci.c b/lib/pci.c
> index 0058d70..ef0b02a 100644
> --- a/lib/pci.c
> +++ b/lib/pci.c
> @@ -7,37 +7,66 @@
> #include "pci.h"
> #include "asm/pci.h"
>
> -/* 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)
> +/*
> + * Scan bus look for a specific device. Only bus 0 scanned for now.
> + * After the scan, a pci_dev is returned with correct BAR information.
> + */
> +int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id)
> +{
> + unsigned i;
> + uint32_t id;
> +
> + memset(dev, 0, sizeof(*dev));
> +
> + for (i = 0; i < PCI_DEVFN_MAX; ++i) {
> + id = pci_config_read(i, 0);
Hmm, pci_config_read still uses devid, but everything else now uses
pci_dev. I don't really like that inconsistency.
> + if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id)
> + break;
> + }
> +
> + if (i == PCI_DEVFN_MAX) {
> + printf("PCI: failed init dev (vendor: 0x%04x, "
> + "device: 0x%04x)\n", vendor_id, device_id);
> + return -1;
> + }
> +
> + dev->pci_addr = i;
but i is the devid, not the addr. Unless I misunderstand what 'addr' means
here.
> + for (i = 0; i < PCI_BAR_NUM; i++) {
> + if (!pci_bar_is_valid(dev, i))
> + continue;
> + dev->pci_bar[i] = pci_bar_addr(dev, i);
> + printf("PCI: init dev 0x%04x BAR %d [%s] addr 0x%lx\n",
> + dev->pci_addr, i, pci_bar_is_memory(dev, i) ?
> + "MEM" : "IO", dev->pci_bar[i]);
> + }
> + dev->inited = true;
> +
> + return 0;
> +}
> +
> +static inline uint32_t pci_config_read_bar(pci_dev *dev, int n)
> {
> - unsigned dev;
> - for (dev = 0; dev < 256; ++dev) {
> - uint32_t id = pci_config_read(dev, 0);
> - if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id) {
> - return dev;
> - }
> - }
> - return PCIDEVADDR_INVALID;
> + return pci_config_read(dev->pci_addr,
> + PCI_BASE_ADDRESS_0 + n * 4);
> }
>
> -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
> +phys_addr_t pci_bar_addr(pci_dev *dev, int bar_num)
> {
> - uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> - if (bar & PCI_BASE_ADDRESS_SPACE_IO) {
> - return bar & PCI_BASE_ADDRESS_IO_MASK;
> - } else {
> - return bar & PCI_BASE_ADDRESS_MEM_MASK;
> - }
> + uint32_t bar = pci_config_read_bar(dev, bar_num);
> + if (bar & PCI_BASE_ADDRESS_SPACE_IO) {
> + return bar & PCI_BASE_ADDRESS_IO_MASK;
> + } else {
> + return bar & PCI_BASE_ADDRESS_MEM_MASK;
> + }
> }
>
> -bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
> +bool pci_bar_is_memory(pci_dev *dev, int bar_num)
> {
> - uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> - return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
> + uint32_t bar = pci_config_read_bar(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(pci_dev *dev, int bar_num)
> {
> - uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
> - return bar;
> + return pci_config_read_bar(dev, bar_num);
> }
> diff --git a/lib/pci.h b/lib/pci.h
> index 9160cfb..b8755ff 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -14,10 +14,21 @@ typedef uint16_t pcidevaddr_t;
> enum {
> PCIDEVADDR_INVALID = 0xffff,
> };
> -pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
> -unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num);
> -bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
> -bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> +
> +#define PCI_BAR_NUM (6)
> +#define PCI_DEVFN_MAX (256)
> +
> +struct pci_dev {
> + bool inited;
> + uint16_t pci_addr;
> + phys_addr_t pci_bar[PCI_BAR_NUM];
> +};
> +typedef struct pci_dev pci_dev;
> +
> +int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id);
> +phys_addr_t pci_bar_addr(pci_dev *dev, int bar_num);
> +bool pci_bar_is_memory(pci_dev *dev, int bar_num);
> +bool pci_bar_is_valid(pci_dev *dev, int bar_num);
>
> /*
> * pci-testdev is a driver for the pci-testdev qemu pci device. The
> @@ -27,8 +38,6 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
> #define PCI_VENDOR_ID_REDHAT 0x1b36
> #define PCI_DEVICE_ID_REDHAT_TEST 0x0005
>
> -#define PCI_TESTDEV_NUM_BARS 2
Why remove this? I could be used to test that we only find this many
BARs on the testdev, no?
> -
> struct pci_test_dev_hdr {
> uint8_t test;
> uint8_t width;
> @@ -39,4 +48,12 @@ struct pci_test_dev_hdr {
> uint8_t name[];
> };
>
> +enum pci_dma_dir {
> + PCI_DMA_FROM_DEVICE = 0,
> + PCI_DMA_TO_DEVICE,
> +};
> +typedef enum pci_dma_dir pci_dma_dir_t;
> +
> +typedef uint64_t iova_t;
stray changes
> +
> #endif /* PCI_H */
> diff --git a/x86/vmexit.c b/x86/vmexit.c
> index c2e1e49..908d2dc 100644
> --- a/x86/vmexit.c
> +++ b/x86/vmexit.c
> @@ -371,8 +371,7 @@ int main(int ac, char **av)
> {
> struct fadt_descriptor_rev1 *fadt;
> int i;
> - unsigned long membar = 0;
> - pcidevaddr_t pcidev;
> + pci_dev dev;
>
> smp_init();
> setup_vm();
> @@ -385,21 +384,10 @@ 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) {
> - for (i = 0; i < PCI_TESTDEV_NUM_BARS; i++) {
> - if (!pci_bar_is_valid(pcidev, i)) {
> - continue;
> - }
> - if (pci_bar_is_memory(pcidev, i)) {
> - membar = pci_bar_addr(pcidev, i);
> - pci_test.memaddr = ioremap(membar, PAGE_SIZE);
> - } else {
> - pci_test.iobar = pci_bar_addr(pcidev, i);
> - }
> - }
> - printf("pci-testdev at 0x%x membar %lx iobar %x\n",
> - pcidev, membar, pci_test.iobar);
> + if (!pci_dev_init(&dev, PCI_VENDOR_ID_REDHAT,
> + PCI_DEVICE_ID_REDHAT_TEST)) {
> + pci_test.memaddr = ioremap(dev.pci_bar[0], PAGE_SIZE);
> + pci_test.iobar = dev.pci_bar[1];
Before we didn't require BAR0 to be MEM and BAR1 to be IO, now we do. It's
probably safe, but less flexible. I think we should at least provide
asserts that our assumptions are correct.
> }
>
> for (i = 0; i < ARRAY_SIZE(tests); ++i)
> --
> 2.7.4
>
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
