On Thu, Jan 21, 2016 at 08:07:58PM +0900, Tetsuya Mukawa wrote: > +static int > +virt_read_pci_cfg(struct virtio_hw *hw, void *buf, size_t len, off_t offset) > +{ > + qtest_read_pci_cfg(hw, "virtio-net", buf, len, offset); > + return 0; > +} > + > +static void * > +virt_get_mapped_addr(struct virtio_hw *hw, uint8_t bar, > + uint32_t offset, uint32_t length) > +{ > + uint64_t base; > + uint64_t size; > + > + if (qtest_get_bar_size(hw, "virtio-net", bar, &size) < 0) { > + PMD_INIT_LOG(ERR, "invalid bar: %u", bar); > + return NULL; > + } > + > + if (offset + length < offset) { > + PMD_INIT_LOG(ERR, "offset(%u) + lenght(%u) overflows", > + offset, length); > + return NULL; > + } > + > + if (offset + length > size) { > + PMD_INIT_LOG(ERR, > + "invalid cap: overflows bar space: %u > %"PRIu64, > + offset + length, size); > + return NULL; > + } > + > + if (qtest_get_bar_addr(hw, "virtio-net", bar, &base) < 0) { > + PMD_INIT_LOG(ERR, "invalid bar: %u", bar); > + return NULL; > + }
So, I understood the usage now, and the cfg_ops abstraction doesn't look good yet necessary to me. For EAL managed pci device, bar length and addr are stored at memory_resources[], and for your case, it's from the qtest. And judging that it's compile time decision, I'd like it to be: #ifdef /* RTE_LIBRTE_VIRTIO_HOST_MODE */ static uint32_t get_bar_size(...) { return qtest_get_bar_size(..); } static uint64-t get_bar_addr(...) { return qtest_get_bar_addr(..); } ... ... #else static uint32_t get_bar_size(...) { return dev->mem_resource[bar].addr; } ... } #endif And then you just need do related changes at virtio_read_caps() and get_cfg_addr(). That'd be much simpler, without introducing duplicate code and uncessary complex. What do you think of that? --yliu > + > + return (void *)(base + offset); > +} > + > +static const struct virtio_pci_cfg_ops virt_cfg_ops = { > + .map = virt_map_pci_cfg, > + .unmap = virt_unmap_pci_cfg, > + .get_mapped_addr = virt_get_mapped_addr, > + .read = virt_read_pci_cfg, > +}; > +#endif /* RTE_LIBRTE_VIRTIO_HOST_MODE */