Re: [PATCH 2/2] virtio-console: Add interface for generic guest-host communication

[Juan Quintela <quintela@trasno.org>]

>> > +    for (i = 0; i < (le32_to_cpu(virtcon_config.max_nr_ports) + 31) / 32; i++) {
>> > +        qemu_put_be32s(f, &virtcon_config.ports_map[i]);
>> > +    }
>> 
>> Don't even try to save a couple of bytes, please.
>
> Sorry, don't understand this.
>
>> 
>> for (i = 0; i <  (MAX_VIRTIO_CONSOLES + 31) / 32; i++) {
>>     qemu_put_be32s(f, &virtcon_config.ports_map[i]);
>> }
>> 
>> Same for load, please.  Makes the translation to table based way easier.
>
> OK, maybe I now get it. You're asking me not to le32_to_cpu() here,
> correct?

>> > +    /* Items in struct VirtIOConsolePort */
>> > +    for (i = 0; i < le32_to_cpu(virtcon_config.max_nr_ports); i++) {
>> > +        qemu_put_byte(f, virtcon_ports[i].guest_connected);
>> > +    }
>> 
>> If you want to optimize saving of this field, it is possible that it is
>> a better idea to create another bitmap for it.  What do you think?
>
> Yes, I wanted to do that as well. The same functions that compute the
> modulo/division earlier can give us this thing as well. But it'll only
> be used for loadvm/savevm. I expect the bool to continue to reside in
> this struct for normal operation.
>
>> I can't see VMState adding support for bools any time soon, but arrays
>> of bool is a possibility.
>
> Consider the RFE filed :-)
>
>> > @@ -128,19 +610,29 @@ VirtIODevice *virtio_console_init(DeviceState *dev)
>> >      VirtIOConsole *s;
>> >      s = (VirtIOConsole *)virtio_common_init("virtio-console",
>> >                                              VIRTIO_ID_CONSOLE,
>> > -                                            0, sizeof(VirtIOConsole));
>> > +                                            sizeof(struct virtio_console_config),
>> > +                                            sizeof(VirtIOConsole));
>> 
>> Here, you mean:
>> 
>> VirtIODevice *d = virtio_common_init(....);
>> VirtIOConsole *s = DO_UPCAST(VirtIOConsole, vdev, d);
>> 
>> :)
>
> Again original code, not my contributions :-)
>
>> Creation of VirtIODevice *d is not necesary, I found it easier to
>> understand  but it is up to you.
>
> For virtio-serial, I statically allocated VirtIOSerial and had a vdev
> pointer inside it which virtio_common_init gave me. I can do the same
> here.
>
>> >      if (s == NULL)
>> >          return NULL;
>> >  
>> 
>> Once you are there, virtio_common_init() can't return NULL, fix it please.
>
> Original code. But I will take it.
>
>> > -#define MAX_VIRTIO_CONSOLES 1
>> > +#define MAX_VIRTIO_CONSOLE_PORTS 64
>> 
>> Require that this number is a multiple of 32, and you can simplify all
>> the (foo + 31/32) staff.
>
> Sure, but it's better to be careful in a couple of places just because
> we can be. Does (foo+31)/32 make it very difficult to read?

It is easier the other way around, and one assert in an init function
seeing that FOO%32 == 0, and you get the same result.

Later, Juan.