Re: [Qemu-devel] [RFC PATCH v2 7/8] pc-bios/s390-ccw: Add virtio-net driver code

[Thomas Huth]

On 07.07.2017 14:37, Cornelia Huck wrote:
> On Fri,  7 Jul 2017 12:27:03 +0200
> Thomas Huth <address@hidden> wrote:
> 
>> The driver provides the recv() and send() functions which will
>> be required by SLOF's libnet code for receiving and sending
>> packets.
[...]
>> +int virtio_net_init(void *mac_addr)
>> +{
>> +    VDev *vdev = virtio_get_device();
>> +    VRing *rxvq = &vdev->vrings[VQ_RX];
>> +    void *buf;
>> +    int i;
>> +
>> +    virtio_setup_ccw(vdev);
>> +
>> +    memcpy(mac_addr, vdev->config.net.mac, ETH_ALEN);
> 
> The mac address in the config space is only valid if the mac feature
> flag has been negotiated (although the field always exists).

QEMU seems to always set it ... but yes, let's better negotiate
VIRTIO_NET_F_MAC here.

>> +
>> +    for (i = 0; i < 64; i++) {
>> +        buf = malloc(ETH_MTU_SIZE + sizeof(VirtioNetHdr));
>> +        IPL_assert(buf != NULL, "Can not allocate memory for receive 
>> buffers");
>> +        vring_send_buf(rxvq, buf, ETH_MTU_SIZE + sizeof(VirtioNetHdr),
>> +                       VRING_DESC_F_WRITE);
>> +    }
>> +    vring_notify(rxvq);
>> +
>> +    return 0;
>> +}
>> +
>> +int send(int fd, const void *buf, int len, int flags)
>> +{
>> +    VirtioNetHdr tx_hdr;
>> +    VDev *vdev = virtio_get_device();
>> +    VRing *txvq = &vdev->vrings[VQ_TX];
>> +
>> +    /* Set up header - we do not use anything special, so simply clear it */
>> +    memset(&tx_hdr, 0, sizeof(tx_hdr));
>> +
>> +    vring_send_buf(txvq, &tx_hdr, sizeof(tx_hdr), VRING_DESC_F_NEXT);
>> +    vring_send_buf(txvq, (void *)buf, len, VRING_HIDDEN_IS_CHAIN);
>> +    while (!vr_poll(txvq)) {
>> +        yield();
>> +    }
>> +    if (drain_irqs(txvq->schid)) {
>> +        puts("send: drain irqs failed");
>> +        return -1;
> 
> Does the caller try again later? drain_irqs() errors may be transient.

Yes, the TFTP code retries to send packets after a certain amount of
time (since the packets could also be lost in the network somewhere).

>> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
>> index 09ab291..729d926 100644
>> --- a/pc-bios/s390-ccw/virtio.c
>> +++ b/pc-bios/s390-ccw/virtio.c
>> @@ -149,7 +149,7 @@ static void vring_init(VRing *vr, VqInfo *info)
>>      debug_print_addr("init vr", vr);
>>  }
>>  
>> -static bool vring_notify(VRing *vr)
>> +bool vring_notify(VRing *vr)
>>  {
>>      vr->cookie = virtio_notify(vr->schid, vr->id, vr->cookie);
>>      return vr->cookie >= 0;
>> @@ -188,7 +188,7 @@ ulong get_second(void)
>>      return (get_clock() >> 12) / 1000000;
>>  }
>>  
>> -static int vr_poll(VRing *vr)
>> +int vr_poll(VRing *vr)
>>  {
>>      if (vr->used->idx == vr->used_idx) {
>>          vring_notify(vr);
>> @@ -261,6 +261,11 @@ void virtio_setup_ccw(VDev *vdev)
>>      run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0);
>>  
>>      switch (vdev->senseid.cu_model) {
>> +    case VIRTIO_ID_NET:
>> +        vdev->nr_vqs = 3;
> 
> I think this should be 2, as you don't negotiate for the control vq.

OK.

>> +        vdev->cmd_vr_idx = 0;
>> +        cfg_size = sizeof(vdev->config.net);
>> +        break;
>>      case VIRTIO_ID_BLOCK:
>>          vdev->nr_vqs = 1;
>>          vdev->cmd_vr_idx = 0;
>> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
>> index d743919..d6af022 100644
>> --- a/pc-bios/s390-ccw/virtio.h
>> +++ b/pc-bios/s390-ccw/virtio.h
>> @@ -11,8 +11,6 @@
>>  #ifndef VIRTIO_H
>>  #define VIRTIO_H
>>  
>> -#include "s390-ccw.h"
>> -
>>  /* Status byte for guest to report progress, and synchronize features. */
>>  /* We have seen device and processed generic fields 
>> (VIRTIO_CONFIG_F_VIRTIO) */
>>  #define VIRTIO_CONFIG_S_ACKNOWLEDGE     1
>> @@ -254,6 +252,12 @@ struct ScsiDevice {
>>  };
>>  typedef struct ScsiDevice ScsiDevice;
>>  
>> +struct VirtioNetConfig {
>> +    uint8_t mac[6];
>> +    uint16_t status;
> 
> I don't think status exists if you don't negotiate the respective
> feature bit.

OK.

>> +};
>> +typedef struct VirtioNetConfig VirtioNetConfig;
>> +
>>  struct VDev {
>>      int nr_vqs;
>>      VRing *vrings;
>> @@ -266,6 +270,7 @@ struct VDev {
>>      union {
>>          VirtioBlkConfig blk;
>>          VirtioScsiConfig scsi;
>> +        VirtioNetConfig net;
>>      } config;
>>      ScsiDevice *scsi_device;
>>      bool is_cdrom;
>> @@ -291,10 +296,14 @@ struct VirtioCmd {
>>  };
>>  typedef struct VirtioCmd VirtioCmd;
>>  
>> +bool vring_notify(VRing *vr);
>>  int drain_irqs(SubChannelId schid);
>>  void vring_send_buf(VRing *vr, void *p, int len, int flags);
>> +int vr_poll(VRing *vr);
>>  int vring_wait_reply(void);
>>  int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd);
>>  void virtio_setup_ccw(VDev *vdev);
>>  
>> +int virtio_net_init(void *mac_addr);
>> +
>>  #endif /* VIRTIO_H */
> 

Thanks for the review!

 Thomas