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
