Re: [dpdk-dev] [PATCH v2 0/7] virtio 1.0 enabling for virtio pmd driver

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2016/01/14 15:09, Tan, Jianfeng wrote:
>
> 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?

Yes, this is the code.
In our case, instead of using physical address, virtual address will be
used, right?
Problem is that virtual address will be over (1 << 44) without
specifying some mmap options.
Probably we need to specify "MAP_FIXED" option while mmapping EAL memory
to get lower address.

But if we can use VIRTIO-1.0, we can specify 64bit address, then we
don't need to do something tricky mmaping.

>
>>
>> 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?

Yes, I agree he doesn't need.

Firstly, I have implemented like above, then I noticed that
virtio_read_caps_phy() and virtio_read_caps_virt() are same except for
access method.
Anyway, I guess abstracting access method is not so difficult.
If you are OK, I want to send RFC on Yuanhan's patch. Is it OK?

Thanks,
Tetsuya