[dpdk-dev] [PATCH 02/40] net/virtio: Introduce Virtio bus type

[Maxime Coquelin]

On 1/14/21 10:24 AM, Maxime Coquelin wrote:
> 
> 
> On 1/5/21 10:15 PM, David Marchand wrote:
>> On Sun, Dec 20, 2020 at 10:14 PM Maxime Coquelin
>> <maxime.coquelin at redhat.com> wrote:
>>>
>>> This patch is preliminary work for introducing a bus layer
>>> in Virtio PMD, in order to improve Virtio-user integration.
>>>
>>> A new bus type is added to provide a unified way to distinguish
>>> which bus type is used (PCI modern, PCI legacy or Virtio-user).
>>
>> In dpdk, we don't use a capital letter for starting a title.
>>
>>
>>> @@ -1883,15 +1883,14 @@ virtio_remap_pci(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
>>>  static void
>>>  virtio_set_vtpci_ops(struct virtio_hw *hw)
>>>  {
>>> -#ifdef RTE_VIRTIO_USER
>>
>> Too soon to remove this check, since virtio_user_ops comes from
>> virtio_user_ethdev.c.
>> This will break compilation on FreeBSD.
> 
> Agree, I restored it back in v2.
> 
>>
>>> -       if (hw->virtio_user_dev)
>>> +       if (hw->bus_type == VIRTIO_BUS_USER)
>>>                 VTPCI_OPS(hw) = &virtio_user_ops;
>>> -       else
>>> -#endif
>>> -       if (hw->modern)
>>> +       else if (hw->bus_type == VIRTIO_BUS_PCI_MODERN)
>>>                 VTPCI_OPS(hw) = &modern_ops;
>>> -       else
>>> +       else if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
>>>                 VTPCI_OPS(hw) = &legacy_ops;
>>> +
>>> +       return;
>>>  }
>>>
>>>  /*
>>> @@ -1919,7 +1918,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>>         eth_dev->rx_descriptor_done = virtio_dev_rx_queue_done;
>>>
>>>         if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
>>> -               if (!hw->virtio_user_dev) {
>>> +               if (hw->bus_type != VIRTIO_BUS_USER) {
>>
>> In the rest of the patch, we check for PCI types when dealing with PCI
>> code, so I'd rather be consistent and check for modern and legacy pci
>> types here too.
> 
> Agree, it is not consistent. I fixed it here and below for v2.

Wait, as we discussed off-list, we need hw->bus_type != VIRTIO_BUS_USER,
as the bus_type is still unknown for PCI bus at that stage.

>>
>>>                         ret = virtio_remap_pci(RTE_ETH_DEV_TO_PCI(eth_dev), hw);
>>>                         if (ret)
>>>                                 return ret;
>>> @@ -1950,7 +1949,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>>         /* For virtio_user case the hw->virtio_user_dev is populated by
>>>          * virtio_user_eth_dev_alloc() before eth_virtio_dev_init() is called.
>>>          */
>>> -       if (!hw->virtio_user_dev) {
>>> +       if (hw->bus_type != VIRTIO_BUS_USER) {
>>
>> Idem.

Same here.

>>
>>
>>>                 ret = vtpci_init(RTE_ETH_DEV_TO_PCI(eth_dev), hw);

bus_type is set to VIRTIO_BUS_PCI_MODERN or VIRTIO_BUS_PCI_LEGACY in
vtcpi_init().

>>>                 if (ret)
>>>                         goto err_vtpci_init;
>>> @@ -1982,9 +1981,9 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>>>         return 0;
>>>
>>>  err_virtio_init:
>>> -       if (!hw->virtio_user_dev) {
>>> +       if (hw->bus_type == VIRTIO_BUS_PCI_MODERN || hw->bus_type == VIRTIO_BUS_PCI_LEGACY) {
>>>                 rte_pci_unmap_device(RTE_ETH_DEV_TO_PCI(eth_dev));
>>> -               if (!hw->modern)
>>> +               if (hw->bus_type == VIRTIO_BUS_PCI_LEGACY)
>>>                         rte_pci_ioport_unmap(VTPCI_IO(hw));
>>>         }
>>>  err_vtpci_init:
>>
>>
>