Re: [Qemu-devel] [PATCH 4/5] backdoor: [softmmu] Add QEMU-side proxy to "libbackdoor.a"

[Blue Swirl]

2011/9/29 Lluís Vilanova <address@hidden>:
> Blue Swirl writes:
>
>> 2011/9/29 Lluís Vilanova <address@hidden>:
>>> +static uint64_t control_io_read(void *opaque, target_phys_addr_t addr, 
>>> unsigned size)
>>> +{
>>> +    State *s = opaque;
>>> +
>>> +    uint64_t res = ldq_p(&s->size);
>>> +    uint8_t *resb = (uint8_t*)&res;
>>> +    return resb[addr % CTRL_BYTES];
>
>> I don't think these lines do what you mean, but I'm also not sure what
>> it is supposed to mean.
>
> Pre: only can read on a byte-per-byte basis (as stated in control_ops.impl),
> just because the code looks less ugly, and host performance should not be an
> issue here.
>
> The device is treated as a circular buffer of length CTRL_BYTES
>
> Reads are only used to get the size of the data channel.
>
> First line should handle guest/host endianess swapping, although I'm not sure 
> if
> that's the API I'm supposed to use.
>
> Then return the N'th byte of the uint64_t variable holding the 
> (endianess-aware)
> result.

That may be the intention, but the first line will load res from guest
memory using an address (&s->size) in host memory. I think the next
two lines are equal to
return res >> (addr % CTRL_BYTES);
but with some obfuscation.

It would be much clearer if the registers were byte arrays so you
could read and write the data directly without pointer arithmetic.

Byte accesses will be slower than larger word size accesses, I thought
performance was one of the goals with this?

>>> +}
>>> +
>>> +static void control_io_write(void *opaque, target_phys_addr_t addr, 
>>> uint64_t data, unsigned size)
>>> +{
>>> +    State *s = opaque;
>>> +
>>> +    uint8_t *cmdb = (uint8_t*)&s->cmd;
>>> +    cmdb[addr % CTRL_BYTES] = (uint8_t)data;
>>> +
>>> +    if ((addr + size) % CTRL_BYTES == 0) {
>>> +        qemu_backdoor(ldq_p(&s->cmd), s->data_ptr);
>>> +    }
>
>> Same here.
>
> Pre: same as during reads.
>
> Accumulates writes into s->cmd to build the command the guest is sending us 
> (in
> guest endianess).
>
> When CTRL_BYTES bytes have been written into the device, get the command value
> into host endianess and invoke the user-provided backdoor callback.
>
> This assumes that when executing in KVM, the device handling infrastructure 
> will
> get a lock and only one CPU will be sending a backdoor command until 
> completion.
>
>
> I'll add some comments there and prefix all structs and functions with
> "backdoor_", as otherwise debugging could get harder if everyone started
> avoiding the prefixes.
>
>
> Lluis
>
> --
>  "And it's much the same thing with knowledge, for whenever you learn
>  something new, the whole world becomes that much richer."
>  -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
>  Tollbooth
>