Re: [Qemu-devel] [PATCH 2/2] Add AT24Cxx I2C EEPROM device model

[Jan Kiszka]

On 2012-11-20 15:06, Stefan Hajnoczi wrote:
> On Tue, Nov 20, 2012 at 02:37:07PM +0100, Jan Kiszka wrote:
>> On 2012-11-20 14:27, Stefan Hajnoczi wrote:
>>> On Mon, Nov 19, 2012 at 03:24:39PM +0100, Jan Kiszka wrote:
>>>> +static void at24_flush_transfer_buffer(AT24State *s)
>>>> +{
>>>> +    if (s->cached_sector < 0 || !s->cache_dirty) {
>>>> +        return;
>>>> +    }
>>>> +    bdrv_write(s->bs, s->cached_sector, s->sector_buffer, 1);
>>> [...]
>>>> +static int at24_cache_sector(AT24State *s, int sector)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    if (s->cached_sector == sector) {
>>>> +        return 0;
>>>> +    }
>>>> +    ret = bdrv_read(s->bs, sector, s->sector_buffer, 1);
>>>
>>> Can you use bdrv_aio_writev()/bdrv_aio_readv()?  We should avoid adding
>>> new synchronous block I/O.  Because it forces us to run a nested event
>>> loop that blocks the guest until I/O completes.
>>
>> The call is synchronous as the I2C bus model is as well. How do I model
>> this with bdrv_aio_*?
> 
> The bus model needs to be asynchronous.

That's not easy, even in theory. I2C allows a slave to defer its answer
by holding the clock line down, but not all masters support this, and
some (e.g. SMBus) even demand an upper limit. So we risk timeouts of the
guest when deferring normally synchronous accesses like on these EEPROMs.

IOW: There will likely be a need for synchronously waiting on block
layer I/O requests also in the future.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux