| Message ID | 20170103211705.27876-1-jcd@tribudubois.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 3 January 2017 at 21:17, Jean-Christophe Dubois <jcd@tribudubois.net> wrote: > Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> > --- > hw/block/m25p80.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c > index d29ff4c..6c374cf 100644 > --- a/hw/block/m25p80.c > +++ b/hw/block/m25p80.c > @@ -28,6 +28,7 @@ > #include "hw/ssi/ssi.h" > #include "qemu/bitops.h" > #include "qemu/log.h" > +#include "qemu/error-report.h" > #include "qapi/error.h" > > #ifndef M25P80_ERR_DEBUG > @@ -376,6 +377,8 @@ typedef enum { > MAN_GENERIC, > } Manufacturer; > > +#define _INTERNAL_DATA_SIZE 16 > + Don't use leading underscores, please. > typedef struct Flash { > SSISlave parent_obj; > > @@ -386,7 +389,7 @@ typedef struct Flash { > int page_size; > > uint8_t state; > - uint8_t data[16]; > + uint8_t data[_INTERNAL_DATA_SIZE]; > uint32_t len; > uint32_t pos; > uint8_t needed_bytes; > @@ -1114,6 +1117,12 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx) > > case STATE_COLLECTING_DATA: > case STATE_COLLECTING_VAR_LEN_DATA: > + > + if (s->len >= _INTERNAL_DATA_SIZE) { > + error_report("Bug - Write overrun internal data buffer"); > + abort(); > + } > + > s->data[s->len] = (uint8_t)tx; > s->len++; > > @@ -1123,6 +1132,12 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx) > break; > > case STATE_READING_DATA: > + > + if (s->pos >= _INTERNAL_DATA_SIZE) { > + error_report("Bug - Read overrun internal data buffer"); > + abort(); > + } > + If these are "can't happen unless some other part of QEMU is buggy" cases, then we can just assert(): assert(s->pos < ARRAY_SIZE(s->data)); A comment about what kind of other part of QEMU might be buggy if the assertion fires would also be helpful for future readers. (If they're "could happen if the guest does something wrong" cases, we shouldn't just abort(), but if I'm reading the previous mail thread correctly, that's not the situation here.) > r = s->data[s->pos]; > s->pos++; > if (s->pos == s->len) { > @@ -1195,7 +1210,7 @@ static const VMStateDescription vmstate_m25p80 = { > .pre_save = m25p80_pre_save, > .fields = (VMStateField[]) { > VMSTATE_UINT8(state, Flash), > - VMSTATE_UINT8_ARRAY(data, Flash, 16), > + VMSTATE_UINT8_ARRAY(data, Flash, _INTERNAL_DATA_SIZE), > VMSTATE_UINT32(len, Flash), > VMSTATE_UINT32(pos, Flash), > VMSTATE_UINT8(needed_bytes, Flash), > -- > 2.9.3 thanks -- PMM
Hi Peter, W dniu 05.01.2017 o 19:38, Peter Maydell pisze: > On 3 January 2017 at 21:17, Jean-Christophe Dubois <jcd@tribudubois.net> wrote: >> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> >> --- >> hw/block/m25p80.c | 19 +++++++++++++++++-- >> 1 file changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c >> index d29ff4c..6c374cf 100644 >> --- a/hw/block/m25p80.c >> +++ b/hw/block/m25p80.c >> @@ -28,6 +28,7 @@ >> #include "hw/ssi/ssi.h" >> #include "qemu/bitops.h" >> #include "qemu/log.h" >> +#include "qemu/error-report.h" >> #include "qapi/error.h" >> >> #ifndef M25P80_ERR_DEBUG >> @@ -376,6 +377,8 @@ typedef enum { >> MAN_GENERIC, >> } Manufacturer; >> >> +#define _INTERNAL_DATA_SIZE 16 >> + > Don't use leading underscores, please. > >> typedef struct Flash { >> SSISlave parent_obj; >> >> @@ -386,7 +389,7 @@ typedef struct Flash { >> int page_size; >> >> uint8_t state; >> - uint8_t data[16]; >> + uint8_t data[_INTERNAL_DATA_SIZE]; >> uint32_t len; >> uint32_t pos; >> uint8_t needed_bytes; >> @@ -1114,6 +1117,12 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx) >> >> case STATE_COLLECTING_DATA: >> case STATE_COLLECTING_VAR_LEN_DATA: >> + >> + if (s->len >= _INTERNAL_DATA_SIZE) { >> + error_report("Bug - Write overrun internal data buffer"); >> + abort(); >> + } >> + >> s->data[s->len] = (uint8_t)tx; >> s->len++; >> >> @@ -1123,6 +1132,12 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx) >> break; >> >> case STATE_READING_DATA: >> + >> + if (s->pos >= _INTERNAL_DATA_SIZE) { >> + error_report("Bug - Read overrun internal data buffer"); >> + abort(); >> + } >> + > If these are "can't happen unless some other part of QEMU > is buggy" cases, then we can just assert(): > > assert(s->pos < ARRAY_SIZE(s->data)); > > A comment about what kind of other part of QEMU might be buggy > if the assertion fires would also be helpful for future readers. > > (If they're "could happen if the guest does something wrong" > cases, we shouldn't just abort(), but if I'm reading the previous > mail thread correctly, that's not the situation here.) Indeed this case is about error in Qemu itself, but the same situation could be generated from the guest (guest deasert CS only once). IMHO we should reset m26p80 state in such case: s->len = 0; s->pos = 0; s->state = STATE_IDLE; This will be a bit closer to real HW behaviour too. Thanks, Marcin > >> r = s->data[s->pos]; >> s->pos++; >> if (s->pos == s->len) { >> @@ -1195,7 +1210,7 @@ static const VMStateDescription vmstate_m25p80 = { >> .pre_save = m25p80_pre_save, >> .fields = (VMStateField[]) { >> VMSTATE_UINT8(state, Flash), >> - VMSTATE_UINT8_ARRAY(data, Flash, 16), >> + VMSTATE_UINT8_ARRAY(data, Flash, _INTERNAL_DATA_SIZE), >> VMSTATE_UINT32(len, Flash), >> VMSTATE_UINT32(pos, Flash), >> VMSTATE_UINT8(needed_bytes, Flash), >> -- >> 2.9.3 > thanks > -- PMM >
Le 05/01/2017 à 21:04, mar.krzeminski a écrit : > Hi Peter, > > W dniu 05.01.2017 o 19:38, Peter Maydell pisze: >> On 3 January 2017 at 21:17, Jean-Christophe Dubois >> <jcd@tribudubois.net> wrote: >>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> >>> --- >>> hw/block/m25p80.c | 19 +++++++++++++++++-- >>> 1 file changed, 17 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c >>> index d29ff4c..6c374cf 100644 >>> --- a/hw/block/m25p80.c >>> +++ b/hw/block/m25p80.c >>> @@ -28,6 +28,7 @@ >>> #include "hw/ssi/ssi.h" >>> #include "qemu/bitops.h" >>> #include "qemu/log.h" >>> +#include "qemu/error-report.h" >>> #include "qapi/error.h" >>> >>> #ifndef M25P80_ERR_DEBUG >>> @@ -376,6 +377,8 @@ typedef enum { >>> MAN_GENERIC, >>> } Manufacturer; >>> >>> +#define _INTERNAL_DATA_SIZE 16 >>> + >> Don't use leading underscores, please. >> >>> typedef struct Flash { >>> SSISlave parent_obj; >>> >>> @@ -386,7 +389,7 @@ typedef struct Flash { >>> int page_size; >>> >>> uint8_t state; >>> - uint8_t data[16]; >>> + uint8_t data[_INTERNAL_DATA_SIZE]; >>> uint32_t len; >>> uint32_t pos; >>> uint8_t needed_bytes; >>> @@ -1114,6 +1117,12 @@ static uint32_t m25p80_transfer8(SSISlave >>> *ss, uint32_t tx) >>> >>> case STATE_COLLECTING_DATA: >>> case STATE_COLLECTING_VAR_LEN_DATA: >>> + >>> + if (s->len >= _INTERNAL_DATA_SIZE) { >>> + error_report("Bug - Write overrun internal data buffer"); >>> + abort(); >>> + } >>> + >>> s->data[s->len] = (uint8_t)tx; >>> s->len++; >>> >>> @@ -1123,6 +1132,12 @@ static uint32_t m25p80_transfer8(SSISlave >>> *ss, uint32_t tx) >>> break; >>> >>> case STATE_READING_DATA: >>> + >>> + if (s->pos >= _INTERNAL_DATA_SIZE) { >>> + error_report("Bug - Read overrun internal data buffer"); >>> + abort(); >>> + } >>> + >> If these are "can't happen unless some other part of QEMU >> is buggy" cases, then we can just assert(): >> >> assert(s->pos < ARRAY_SIZE(s->data)); >> >> A comment about what kind of other part of QEMU might be buggy >> if the assertion fires would also be helpful for future readers. >> >> (If they're "could happen if the guest does something wrong" >> cases, we shouldn't just abort(), but if I'm reading the previous >> mail thread correctly, that's not the situation here.) > Indeed this case is about error in Qemu itself, but the same situation > could > be generated from the guest (guest deasert CS only once). > IMHO we should reset m26p80 state in such case: > s->len = 0; > s->pos = 0; > s->state = STATE_IDLE; > This will be a bit closer to real HW behaviour too. So what would be the preferred behavior? * Asserting (and ending Qemu) * Resetting (and hiding the misbehavior). JC > > Thanks, > Marcin > >> >>> r = s->data[s->pos]; >>> s->pos++; >>> if (s->pos == s->len) { >>> @@ -1195,7 +1210,7 @@ static const VMStateDescription vmstate_m25p80 >>> = { >>> .pre_save = m25p80_pre_save, >>> .fields = (VMStateField[]) { >>> VMSTATE_UINT8(state, Flash), >>> - VMSTATE_UINT8_ARRAY(data, Flash, 16), >>> + VMSTATE_UINT8_ARRAY(data, Flash, _INTERNAL_DATA_SIZE), >>> VMSTATE_UINT32(len, Flash), >>> VMSTATE_UINT32(pos, Flash), >>> VMSTATE_UINT8(needed_bytes, Flash), >>> -- >>> 2.9.3 >> thanks >> -- PMM >> > >
On 5 January 2017 at 20:18, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote: > Le 05/01/2017 à 21:04, mar.krzeminski a écrit : >> Peter Maydell wrote: >>> If these are "can't happen unless some other part of QEMU >>> is buggy" cases, then we can just assert(): >>> (If they're "could happen if the guest does something wrong" >>> cases, we shouldn't just abort(), but if I'm reading the previous >>> mail thread correctly, that's not the situation here.) > >> Indeed this case is about error in Qemu itself, but the same situation could >> be generated from the guest (guest deasert CS only once). >> IMHO we should reset m26p80 state in such case: >> s->len = 0; >> s->pos = 0; >> s->state = STATE_IDLE; >> This will be a bit closer to real HW behaviour too. > So what would be the preferred behavior? > > Asserting (and ending Qemu) > Resetting (and hiding the misbehavior). If the guest can trigger this behaviour, then we should not assert or abort or otherwise cause QEMU to exit. The preferred behaviour is: * act like the real hardware does in this situation (whatever that is) * if this is something that only broken guest code would do, log it with qemu_log_mask(LOG_GUEST_ERROR, ...) thanks -- PMM
Le 05/01/2017 à 21:51, Peter Maydell a écrit : >> So what would be the preferred behavior? >> >> Asserting (and ending Qemu) >> Resetting (and hiding the misbehavior). > If the guest can trigger this behaviour, then we should > not assert or abort or otherwise cause QEMU to exit. > The preferred behaviour is: > * act like the real hardware does in this situation > (whatever that is) > * if this is something that only broken guest code would > do, log it with qemu_log_mask(LOG_GUEST_ERROR, ...) I guess a real SPI controllers should not trigger this behavior like I did in the i.MX SPI controller emulation. It was a bug in my code and the crash (SIGSEGV) forced me to try to find a solution. At first I tried to fix the SPI device instead of the SPI controller because a SIGSEGV did not seem like an expected reaction and I was missing hints that my code was misbehaving. But it is also possible for a guest to emulates the SPI controller through bit banging (for example). And in this case the guest could end up misbehaving in its software implementation. So I think this behavior could be triggered either by buggy SPI controller emulator or by buggy guest software. And it seems hard to determine where the fault comes from from within Qemu. Obviously if the fault is in a Qemu emulator we would not want to tag the error as a LOG_GUEST_ERROR. On the other hand, it would be nice to warn the user if the guest if it is indeed misbehaving so that he can fix his code. > > thanks > -- PMM > >
On 5 January 2017 at 21:39, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote: > So I think this behavior could be triggered either by buggy SPI controller > emulator or by buggy guest software. And it seems hard to determine where > the fault comes from from within Qemu. Obviously if the fault is in a Qemu > emulator we would not want to tag the error as a LOG_GUEST_ERROR. On the > other hand, it would be nice to warn the user if the guest if it is indeed > misbehaving so that he can fix his code. We should log it as a guest error. "warning might be wrong if there's a bug in QEMU" is true of pretty much any warning we emit... thanks -- PMM
Le 06/01/2017 à 11:18, Peter Maydell a écrit : > On 5 January 2017 at 21:39, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote: >> So I think this behavior could be triggered either by buggy SPI controller >> emulator or by buggy guest software. And it seems hard to determine where >> the fault comes from from within Qemu. Obviously if the fault is in a Qemu >> emulator we would not want to tag the error as a LOG_GUEST_ERROR. On the >> other hand, it would be nice to warn the user if the guest if it is indeed >> misbehaving so that he can fix his code. > We should log it as a guest error. "warning might be wrong if there's > a bug in QEMU" is true of pretty much any warning we emit... OK, I'll do this. JC > > thanks > -- PMM >
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index d29ff4c..6c374cf 100644 --- a/hw/block/m25p80.c +++ b/hw/block/m25p80.c @@ -28,6 +28,7 @@ #include "hw/ssi/ssi.h" #include "qemu/bitops.h" #include "qemu/log.h" +#include "qemu/error-report.h" #include "qapi/error.h" #ifndef M25P80_ERR_DEBUG @@ -376,6 +377,8 @@ typedef enum { MAN_GENERIC, } Manufacturer; +#define _INTERNAL_DATA_SIZE 16 + typedef struct Flash { SSISlave parent_obj; @@ -386,7 +389,7 @@ typedef struct Flash { int page_size; uint8_t state; - uint8_t data[16]; + uint8_t data[_INTERNAL_DATA_SIZE]; uint32_t len; uint32_t pos; uint8_t needed_bytes; @@ -1114,6 +1117,12 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx) case STATE_COLLECTING_DATA: case STATE_COLLECTING_VAR_LEN_DATA: + + if (s->len >= _INTERNAL_DATA_SIZE) { + error_report("Bug - Write overrun internal data buffer"); + abort(); + } + s->data[s->len] = (uint8_t)tx; s->len++; @@ -1123,6 +1132,12 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx) break; case STATE_READING_DATA: + + if (s->pos >= _INTERNAL_DATA_SIZE) { + error_report("Bug - Read overrun internal data buffer"); + abort(); + } + r = s->data[s->pos]; s->pos++; if (s->pos == s->len) { @@ -1195,7 +1210,7 @@ static const VMStateDescription vmstate_m25p80 = { .pre_save = m25p80_pre_save, .fields = (VMStateField[]) { VMSTATE_UINT8(state, Flash), - VMSTATE_UINT8_ARRAY(data, Flash, 16), + VMSTATE_UINT8_ARRAY(data, Flash, _INTERNAL_DATA_SIZE), VMSTATE_UINT32(len, Flash), VMSTATE_UINT32(pos, Flash), VMSTATE_UINT8(needed_bytes, Flash),
Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> --- hw/block/m25p80.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)