| Message ID | 8c4effc5d537fe0a35791e2f970ede8ed400b5bd.1302246567.git.amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 08.04.2011 09:15, schrieb Amit Shah: > Handle GET_EVENT_STATUS_NOTIFICATION's No Event Available status in its > own function. > > Also ensure the buffer the driver sent us is big enough to fill in all > the data we have -- else just fill in as much as the buffer can hold. This is unnecessary and in fact none of the IDE code does this. s->io_buffer isn't guest memory, but an internal buffer that is allocated like this: s->io_buffer = qemu_memalign(2048, IDE_DMA_BUF_SECTORS*512 + 4); So that's more than enough for storing four bytes. ide_atapi_cmd_reply() takes care of making only max_size bytes visible to the guest. Kevin
On (Fri) 08 Apr 2011 [15:31:49], Kevin Wolf wrote: > Am 08.04.2011 09:15, schrieb Amit Shah: > > Handle GET_EVENT_STATUS_NOTIFICATION's No Event Available status in its > > own function. > > > > Also ensure the buffer the driver sent us is big enough to fill in all > > the data we have -- else just fill in as much as the buffer can hold. > > This is unnecessary and in fact none of the IDE code does this. > s->io_buffer isn't guest memory, but an internal buffer that is > allocated like this: > > s->io_buffer = qemu_memalign(2048, IDE_DMA_BUF_SECTORS*512 + 4); OK - so all the code paths will be much easier then. But by my reading of (the kernel) code, it looks as if the kernel allocates the memory and passes it on. What am I missing? > So that's more than enough for storing four bytes. ide_atapi_cmd_reply() > takes care of making only max_size bytes visible to the guest. OK - but in some cases we just do a ide_set_irq() instead of ide_atapi_cmd_reply() so what happens in that case? Amit
Am 09.04.2011 12:36, schrieb Amit Shah: > On (Fri) 08 Apr 2011 [15:31:49], Kevin Wolf wrote: >> Am 08.04.2011 09:15, schrieb Amit Shah: >>> Handle GET_EVENT_STATUS_NOTIFICATION's No Event Available status in its >>> own function. >>> >>> Also ensure the buffer the driver sent us is big enough to fill in all >>> the data we have -- else just fill in as much as the buffer can hold. >> >> This is unnecessary and in fact none of the IDE code does this. >> s->io_buffer isn't guest memory, but an internal buffer that is >> allocated like this: >> >> s->io_buffer = qemu_memalign(2048, IDE_DMA_BUF_SECTORS*512 + 4); > > OK - so all the code paths will be much easier then. > > But by my reading of (the kernel) code, it looks as if the kernel > allocates the memory and passes it on. What am I missing? That the data is copied. All command work on the internal s->io_buffer. Once the command is completed, the response can be transferred to the guest, either using DMA (see bmdma_rw_buf) or PIO (ide_data_readl). >> So that's more than enough for storing four bytes. ide_atapi_cmd_reply() >> takes care of making only max_size bytes visible to the guest. > > OK - but in some cases we just do a ide_set_irq() instead of > ide_atapi_cmd_reply() so what happens in that case? I can't find any ATAPI command that calls ide_set_irq directly. Anyway, the important thing is that s->io_buffer_size is set correctly. Kevin
diff --git a/hw/ide/core.c b/hw/ide/core.c index c4a5a13..730587e 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1084,14 +1084,45 @@ static int ide_dvd_read_structure(IDEState *s, int format, } } +static unsigned int event_status_nea(uint8_t *buf, unsigned int max_len) +{ + unsigned int used_len; + + /* + * Ensure we don't write on memory we don't have. + * max_len of 0 should not produce an error as well. + */ + used_len = 0; + + /* No event descriptor returned */ + if (max_len > 0) { + buf[0] = 0; + used_len++; + } + if (max_len > 1) { + buf[1] = 0; + used_len++; + } + if (max_len > 2) { + buf[2] = 0x80; /* No Event Available (NEA) */ + used_len++; + } + if (max_len > 3) { + buf[3] = 0x00; /* Empty supported event classes */ + used_len++; + } + return used_len; +} + static void handle_get_event_status_notification(IDEState *s, uint8_t *buf, const uint8_t *packet) { - unsigned int max_len; + unsigned int max_len, used_len; max_len = ube16_to_cpu(packet + 7); + /* It is fine by the MMC spec to not support async mode operations */ if (!(packet[1] & 0x01)) { /* asynchronous mode */ /* Only polling is supported, asynchronous mode is not. */ ide_atapi_cmd_error(s, SENSE_ILLEGAL_REQUEST, @@ -1099,12 +1130,11 @@ static void handle_get_event_status_notification(IDEState *s, return; } - /* polling */ + /* polling mode operation */ + /* We don't support any event class (yet). */ - cpu_to_ube16(buf, 0x00); /* No event descriptor returned */ - buf[2] = 0x80; /* No Event Available (NEA) */ - buf[3] = 0x00; /* Empty supported event classes */ - ide_atapi_cmd_reply(s, 4, max_len); + used_len = event_status_nea(buf, max_len); + ide_atapi_cmd_reply(s, used_len, max_len); } static void ide_atapi_cmd(IDEState *s)
Handle GET_EVENT_STATUS_NOTIFICATION's No Event Available status in its own function. Also ensure the buffer the driver sent us is big enough to fill in all the data we have -- else just fill in as much as the buffer can hold. Signed-off-by: Amit Shah <amit.shah@redhat.com> --- hw/ide/core.c | 42 ++++++++++++++++++++++++++++++++++++------ 1 files changed, 36 insertions(+), 6 deletions(-)