On Fri, 8 Jan 2021 at 19:10, Hao Wu <[email protected]> wrote: > > The ADC is part of NPCM7XX Module. Its behavior is controled by the > ADC_CON register. It converts one of the eight analog inputs into a > digital input and stores it in the ADC_DATA register when enabled. > > Users can alter input value by using qom-set QMP command. > > Reviewed-by: Havard Skinnemoen <[email protected]> > Reviewed-by: Tyrone Ting <[email protected]> > Signed-off-by: Hao Wu <[email protected]>
Hi; I was looking at a division-by-zero bug in the NPCM7xx ADC device which was reported ages ago and is still present: https://gitlab.com/qemu-project/qemu/-/issues/550 The problem is that the guest requests an ADC conversion using the "vref" reference, but vref is zero. Could a Nuvoton-aware person say what is supposed to happen here ? > +static uint32_t npcm7xx_adc_convert(uint32_t input, uint32_t ref) > +{ > + uint32_t result; > + > + result = input * (NPCM7XX_ADC_MAX_RESULT + 1) / ref; In this function we divide by ref... > + if (result > NPCM7XX_ADC_MAX_RESULT) { > + result = NPCM7XX_ADC_MAX_RESULT; > + } > + > + return result; > +} > +static void npcm7xx_adc_convert_done(void *opaque) > +{ > + NPCM7xxADCState *s = opaque; > + uint32_t input = NPCM7XX_ADC_CON_MUX(s->con); > + uint32_t ref = (s->con & NPCM7XX_ADC_CON_REFSEL) > + ? s->iref : s->vref; ...which we set to either s->iref or s->vref, depending on whether the guest set the REFSEL bit in the register or not... > + > + if (input >= NPCM7XX_ADC_NUM_INPUTS) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid input: %u\n", > + __func__, input); > + return; > + } > + s->data = npcm7xx_adc_convert(s->adci[input], ref); > + if (s->con & NPCM7XX_ADC_CON_INT_EN) { > + s->con |= NPCM7XX_ADC_CON_INT; > + qemu_irq_raise(s->irq); > + } > + s->con &= ~NPCM7XX_ADC_CON_CONV; > +} > +static void npcm7xx_adc_init(Object *obj) > +{ > + NPCM7xxADCState *s = NPCM7XX_ADC(obj); > + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > + int i; > + > + sysbus_init_irq(sbd, &s->irq); > + > + timer_init_ns(&s->conv_timer, QEMU_CLOCK_VIRTUAL, > + npcm7xx_adc_convert_done, s); > + memory_region_init_io(&s->iomem, obj, &npcm7xx_adc_ops, s, > + TYPE_NPCM7XX_ADC, 4 * KiB); > + sysbus_init_mmio(sbd, &s->iomem); > + s->clock = qdev_init_clock_in(DEVICE(s), "clock", NULL, NULL); > + > + for (i = 0; i < NPCM7XX_ADC_NUM_INPUTS; ++i) { > + object_property_add_uint32_ptr(obj, "adci[*]", > + &s->adci[i], OBJ_PROP_FLAG_WRITE); > + } > + object_property_add_uint32_ptr(obj, "vref", > + &s->vref, OBJ_PROP_FLAG_WRITE); ...but we don't do anything here to arrange for the "vref" property to have a default value. The board doesn't set "vref" on the device, so it is zero, and if the guest tries to do a conversion with the vref clock QEMU crashes. > +static Property npcm7xx_timer_properties[] = { > + DEFINE_PROP_UINT32("iref", NPCM7xxADCState, iref, > NPCM7XX_ADC_DEFAULT_IREF), Here we set a default value for "iref", so that doesn't have the same problem. > + DEFINE_PROP_END_OF_LIST(), > +}; We should also define a realize method so that we can sanity check the iref and vref property values (notably not allowing them to be set to zero, but also any other sanitization that makes sense for the hardware.) thanks -- PMM
