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

[Blue Swirl]

2011/9/30 Lluís Vilanova <address@hidden>:
> Blue Swirl writes:
>
>> 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.
>
> Ok, I think I found what I really wanted: tswap64
>
>
>> I think the next two lines are equal to
>> return res >> (addr % CTRL_BYTES);
>> but with some obfuscation.
>
> But I cannot assume any endianess on neither host or guest. The only thing I 
> can
> assume is that the generic device code handling the reads will read from lower
> to higher addresses.

Since this is your device, you can specify that the device works only
in little endian, like most if not all PCI devices. Then you can use
le64_to_cpu().

> In any case, take me with a grain of salt, endianess often confuses me.
>
>
>> It would be much clearer if the registers were byte arrays so you
>> could read and write the data directly without pointer arithmetic.
>
> Is that something present on the device API? Sorry I don't know what you mean 
> by
> byte array... for me 'resb' already is a byte array :)

I meant that instead of
    uint64_t size;
    uint64_t cmd;

you'd have
    uint8_t size[8];
    uint8_t cmd[8];

>> Byte accesses will be slower than larger word size accesses, I thought
>> performance was one of the goals with this?
>
> They will be slower on host time, but will not waste "guest time".
>
> BTW, will the current scheme in KVM provoke one VM exit for each byte or only
> one for the whole 64bits?
>
> But yes, I was just too lazy to add code for all the supported sizes from 1 to
> 8, and let the generic device code pick the best.
>
>
> 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
>