| Message ID | 1477468040-21034-9-git-send-email-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 26, 2016 at 03:47:11PM +0800, Peter Xu wrote: > Since pci-testdev is a very specific device for QEMU, let's use the new > pci_scan_bars() helper, and selectively choose the bars we want. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > lib/pci.h | 2 ++ > x86/vmexit.c | 23 ++++++++--------------- > 2 files changed, 10 insertions(+), 15 deletions(-) > > diff --git a/lib/pci.h b/lib/pci.h > index 56b49f0..e48045c 100644 > --- a/lib/pci.h > +++ b/lib/pci.h > @@ -66,6 +66,8 @@ int pci_testdev(void); > * pci-testdev supports at least three types of tests (via mmio and > * portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd > */ > +#define PCI_TESTDEV_BAR_MEM 0 > +#define PCI_TESTDEV_BAR_IO 1 More defines that I think we could live without, but I'll try to stop complaining about self-documenting code... > #define PCI_TESTDEV_NUM_BARS 2 > #define PCI_TESTDEV_NUM_TESTS 3 > > diff --git a/x86/vmexit.c b/x86/vmexit.c > index 2736ab8..880466e 100644 > --- a/x86/vmexit.c > +++ b/x86/vmexit.c > @@ -373,7 +373,6 @@ int main(int ac, char **av) > int i; > unsigned long membar = 0; > struct pci_dev pcidev; > - int ret; > > smp_init(); > setup_vm(); > @@ -386,20 +385,14 @@ int main(int ac, char **av) > pm_tmr_blk = fadt->pm_tmr_blk; > printf("PM timer port is %x\n", pm_tmr_blk); > > - 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)) { > - continue; > - } > - 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); > - } > - } > + if (!pci_find_dev(&pcidev, PCI_VENDOR_ID_REDHAT, > + PCI_DEVICE_ID_REDHAT_TEST)) { > + pci_scan_bars(&pcidev); > + assert(pci_bar_is_memory(&pcidev, PCI_TESTDEV_BAR_MEM)); > + assert(!pci_bar_is_memory(&pcidev, PCI_TESTDEV_BAR_IO)); > + membar = pcidev.pci_bar[PCI_TESTDEV_BAR_MEM]; > + pci_test.memaddr = ioremap(membar, PAGE_SIZE); > + pci_test.iobar = pcidev.pci_bar[PCI_TESTDEV_BAR_IO]; > printf("pci-testdev at 0x%x membar %lx iobar %x\n", > pcidev.pci_bdf, membar, pci_test.iobar); > } > -- > 2.7.4 I'm not sure what we gain by hardcoding the mapping of these bars, besides slightly simpler code, but lib/pci-testdev.c does it too, and it should be quite safe, so OK. Reviewed-by: Andrew Jones <drjones@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 04, 2016 at 05:54:34PM +0100, Andrew Jones wrote: > On Wed, Oct 26, 2016 at 03:47:11PM +0800, Peter Xu wrote: > > Since pci-testdev is a very specific device for QEMU, let's use the new > > pci_scan_bars() helper, and selectively choose the bars we want. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > lib/pci.h | 2 ++ > > x86/vmexit.c | 23 ++++++++--------------- > > 2 files changed, 10 insertions(+), 15 deletions(-) > > > > diff --git a/lib/pci.h b/lib/pci.h > > index 56b49f0..e48045c 100644 > > --- a/lib/pci.h > > +++ b/lib/pci.h > > @@ -66,6 +66,8 @@ int pci_testdev(void); > > * pci-testdev supports at least three types of tests (via mmio and > > * portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd > > */ > > +#define PCI_TESTDEV_BAR_MEM 0 > > +#define PCI_TESTDEV_BAR_IO 1 > > More defines that I think we could live without, but I'll try to stop > complaining about self-documenting code... > > > #define PCI_TESTDEV_NUM_BARS 2 > > #define PCI_TESTDEV_NUM_TESTS 3 > > > > diff --git a/x86/vmexit.c b/x86/vmexit.c > > index 2736ab8..880466e 100644 > > --- a/x86/vmexit.c > > +++ b/x86/vmexit.c > > @@ -373,7 +373,6 @@ int main(int ac, char **av) > > int i; > > unsigned long membar = 0; > > struct pci_dev pcidev; > > - int ret; > > > > smp_init(); > > setup_vm(); > > @@ -386,20 +385,14 @@ int main(int ac, char **av) > > pm_tmr_blk = fadt->pm_tmr_blk; > > printf("PM timer port is %x\n", pm_tmr_blk); > > > > - 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)) { > > - continue; > > - } > > - 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); > > - } > > - } > > + if (!pci_find_dev(&pcidev, PCI_VENDOR_ID_REDHAT, > > + PCI_DEVICE_ID_REDHAT_TEST)) { > > + pci_scan_bars(&pcidev); I think if you attempt to cache all viable information in a pci_dev structure then pci_scan_bars() should be mandatory to call when the pci_dev is initilized. IOW, it needs to be called in pci_dev_init(). > > + assert(pci_bar_is_memory(&pcidev, PCI_TESTDEV_BAR_MEM)); > > + assert(!pci_bar_is_memory(&pcidev, PCI_TESTDEV_BAR_IO)); > > + membar = pcidev.pci_bar[PCI_TESTDEV_BAR_MEM]; > > + pci_test.memaddr = ioremap(membar, PAGE_SIZE); > > + pci_test.iobar = pcidev.pci_bar[PCI_TESTDEV_BAR_IO]; > > printf("pci-testdev at 0x%x membar %lx iobar %x\n", > > pcidev.pci_bdf, membar, pci_test.iobar); > > } > > -- > > 2.7.4 > > I'm not sure what we gain by hardcoding the mapping of these bars, besides > slightly simpler code, but lib/pci-testdev.c does it too, and it should be > quite safe, so OK. > > Reviewed-by: Andrew Jones <drjones@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 08, 2016 at 02:43:53PM +0100, Alexander Gordeev wrote: [...] > > > @@ -386,20 +385,14 @@ int main(int ac, char **av) > > > pm_tmr_blk = fadt->pm_tmr_blk; > > > printf("PM timer port is %x\n", pm_tmr_blk); > > > > > > - 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)) { > > > - continue; > > > - } > > > - 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); > > > - } > > > - } > > > + if (!pci_find_dev(&pcidev, PCI_VENDOR_ID_REDHAT, > > > + PCI_DEVICE_ID_REDHAT_TEST)) { > > > + pci_scan_bars(&pcidev); > > I think if you attempt to cache all viable information in a pci_dev > structure then pci_scan_bars() should be mandatory to call when the > pci_dev is initilized. IOW, it needs to be called in pci_dev_init(). Yeah, I can put it into pci_dev_init(). Anyway, it's only a caching operation. But I also like Drew's idea about pci_enable_defaults() and to make sure pci_dev_init() the minimum. Then, the caller will decide what to do. Again I think it's a matter of taste, and both work for me. :-) Thanks, -- peterx -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/lib/pci.h b/lib/pci.h index 56b49f0..e48045c 100644 --- a/lib/pci.h +++ b/lib/pci.h @@ -66,6 +66,8 @@ int pci_testdev(void); * pci-testdev supports at least three types of tests (via mmio and * portio BARs): no-eventfd, wildcard-eventfd and datamatch-eventfd */ +#define PCI_TESTDEV_BAR_MEM 0 +#define PCI_TESTDEV_BAR_IO 1 #define PCI_TESTDEV_NUM_BARS 2 #define PCI_TESTDEV_NUM_TESTS 3 diff --git a/x86/vmexit.c b/x86/vmexit.c index 2736ab8..880466e 100644 --- a/x86/vmexit.c +++ b/x86/vmexit.c @@ -373,7 +373,6 @@ int main(int ac, char **av) int i; unsigned long membar = 0; struct pci_dev pcidev; - int ret; smp_init(); setup_vm(); @@ -386,20 +385,14 @@ int main(int ac, char **av) pm_tmr_blk = fadt->pm_tmr_blk; printf("PM timer port is %x\n", pm_tmr_blk); - 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)) { - continue; - } - 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); - } - } + if (!pci_find_dev(&pcidev, PCI_VENDOR_ID_REDHAT, + PCI_DEVICE_ID_REDHAT_TEST)) { + pci_scan_bars(&pcidev); + assert(pci_bar_is_memory(&pcidev, PCI_TESTDEV_BAR_MEM)); + assert(!pci_bar_is_memory(&pcidev, PCI_TESTDEV_BAR_IO)); + membar = pcidev.pci_bar[PCI_TESTDEV_BAR_MEM]; + pci_test.memaddr = ioremap(membar, PAGE_SIZE); + pci_test.iobar = pcidev.pci_bar[PCI_TESTDEV_BAR_IO]; printf("pci-testdev at 0x%x membar %lx iobar %x\n", pcidev.pci_bdf, membar, pci_test.iobar); }
Since pci-testdev is a very specific device for QEMU, let's use the new pci_scan_bars() helper, and selectively choose the bars we want. Signed-off-by: Peter Xu <peterx@redhat.com> --- lib/pci.h | 2 ++ x86/vmexit.c | 23 ++++++++--------------- 2 files changed, 10 insertions(+), 15 deletions(-)