Re: [Qemu-devel] [PATCH 2/8] virtio-console: qdev conversion, new virtio-serial-bus

[Amit Shah <amit.shah@redhat.com>]

Hey Anthony

[skipping the part Gerd already answered]

On (Tue) Jan 05 2010 [10:42:39], Anthony Liguori wrote:
>> +static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len)
>> +{
>> +    VirtQueueElement elem;
>> +    VirtQueue *vq;
>> +    struct virtio_console_control *cpkt;
>> +
>> +    vq = port->vser->c_ivq;
>> +    if (!virtio_queue_ready(vq)) {
>> +        return 0;
>> +    }
>> +    if (!virtqueue_pop(vq,&elem)) {
>> +        return 0;
>> +    }
>> +
>> +    cpkt = (struct virtio_console_control *)buf;
>> +    cpkt->id = cpu_to_le32(port->id);
>>    
>
> This is not the right way to deal with endianness.  The guest should  
> write these fields in whatever their native endianness is.  From within  
> qemu, we should access this fields with ldl_p/stl_p.  For x86-on-x86,  
> the effect is a nop.  For TCG with le-on-be, it becomes a byte  
> swap.

Yes, as indicated in a separate thread, I've done that in my dev tree.
I'm just waiting for comments here before sending a new version.

>> +static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
>> +{
>> +    VirtIOSerial *s = opaque;
>> +
>> +    if (version_id>  2) {
>> +        return -EINVAL;
>> +    }
>> +    /* The virtio device */
>> +    virtio_load(&s->vdev, f);
>> +
>> +    if (version_id<  2) {
>> +        return 0;
>> +    }
>> +    /* The config space */
>> +    qemu_get_be16s(f,&s->config.cols);
>> +    qemu_get_be16s(f,&s->config.rows);
>> +    s->config.nr_ports = qemu_get_be32(f);
>> +
>> +    /* Items in struct VirtIOSerial */
>> +    qemu_get_be32s(f,&s->guest_features);
>>    
>
> Why save a copy of this when you can access it through the virtio device?

Hm, not needed here I guess.

>> +static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
>> +{
>> +    VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev);
>> +    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev,&dev->qdev);
>> +
>> +    monitor_printf(mon, "%*s dev-prop-int: id: %u\n",
>> +                   indent, "", port->id);
>> +    monitor_printf(mon, "%*s dev-prop-int: is_console: %d\n",
>> +                   indent, "", port->is_console);
>> +}
>>    
>
> This will break the build since it's not referenced anywhere.

Again, as mentioned in the other thread, it gets used here:

static struct BusInfo virtser_bus_info = {
    .name      = "virtio-serial-bus",
    .size      = sizeof(VirtIOSerialBus),
    .print_dev = virtser_bus_dev_print,
};

(I've compile- and run- tested these patches.)

		Amit