Hi Tetsuya, On 1/14/2016 12:27 PM, Tetsuya Mukawa wrote: > On 2016/01/12 15:58, Yuanhan Liu wrote: > Hi Yuanhan and Jianfeng, > > Thanks for great patches. > I want to use VIRTIO-1.0 feature for my virtio container patch, because > it will solve 44 bit memory address limitation. > (So far, legacy virtio-net device only receives queue address under (1 > << (32 + 12)).)
I suppose you are specifying the code below: /* * Virtio PCI device VIRTIO_PCI_QUEUE_PF register is 32bit, * and only accepts 32 bit page frame number. * Check if the allocated physical memory exceeds 16TB. */ if ((mz->phys_addr + vq->vq_ring_size - 1) >> (VIRTIO_PCI_QUEUE_ADDR_SHIFT + 32)) { PMD_INIT_LOG(ERR, "vring address shouldn't be above 16TB!"); rte_free(vq); return -ENOMEM; } So you don't need to add extra cmd option, right? > > I have a few comments to rebase virtio container patches on this patches. > > 1. VIRTIO_READ_REG_X > > So far, VIRTIO_READ_REG_1/2/4 are defined in virtio_pci.h. > But these macros are only referred by virtio_pci.c. > How about moving the macros to virtio_pci.c? +1 for this. > > 2. Abstraction of read/write accesses. > > It may be difficult to cleanly rebase my patches on this patches, > because virtio_read_caps() is not abstracted. > Let me describe it more. > So far, we need to handle below 3 virtio-net devices.. > - physical virtio-net device. > - virtual virtio-net device in virtio-net PMD. (Jianfeng's patch) > - virtual virtio-net device in QEMU. (my patch) > > Almost all code of the virtio-net PMD can be shared between above > different cases. > Probably big difference is how to access to configuration space. > > Yuanhan's patch introduces an abstraction layer to hide configuration > space layout and how to access it. > Is it possible to separate? > I guess "access method" will be nice to be abstracted separately from > "configuration space layout". > Probably access method will be defined by "eth_dev->dev_type" and the > PMD name like "eth_cvio". > And "configuration space layout" will be defined by capability list of > PCI configuration layout. > > For example, if access method like below are abstracted separately and > current "virtio_pci.c" is implemented on this abstraction, we can easily > re-use virtio_read_caps(). > - how to read/write virtio configuration space. > - how to mmap PCI configuration space. > - how to read/(write) PCI configuration space. I basically agree with you. We have two dimensions here: legacy modern physical virtio device: Use virtio_read_caps_phys() to distinguish virtual virtio device (Tetsuya): Use virtio_read_caps_virt() to distinguish virtual virtio device (Jianfeng): does not need a "configuration space layout", no need to distinguish So in vtpci_init(), we needs to test "eth_dev->dev_type" firstly vtpci_init() { if (eth_dev->dev_type == RTE_ETH_DEV_PCI) { if (virtio_read_caps_phys()) { // modern } else { // legacy } } else { if (Tetsuya's way) { if (virtio_read_caps_virt()) { // modern } else { // legacy } } else { // Jianfeng's way } } } And from Yuanhan's angle, I think he does not need to address this problem. How do you think? Thanks, Jianfeng > > Thanks, > Tetsuya