| Message ID | 1454359775-25959-1-git-send-email-wei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Wei, This still has a problem as I said before. If we execute "virsh reboot xxx" during VM booting(i.e. before the PL061 driver loaded), then after VM booting, the VM will not have any reaction to the "virsh reboot xxx". On 2016/2/2 4:49, Wei Huang wrote: > Current QEMU doesn't clear PL061 state after reset. This causes a > weird issue with guest reboot via GPIO. Here is the device state > description with two reboot requests: > > (PL061State fields) data old_in_data istate > VM boot 0 0 0 > After 1st ACPI reboot request 8 8 8 > After VM PL061 driver ACK 8 8 0 > After VM reboot 8 8 0 > ------------------------------------------------------------ > 2nd ACPI reboot request 8 > > In the second reboot request above, because old_in_data field is 8, > QEMU decides that there is a pending edge IRQ already (see > pl061_update()) in input; so it doesn't raise up IRQ again. As a result > the second reboot request is lost. The correct way is to clear PL061 > device state after reset. > > NOTE: The reset state is found from the following documentation: > - PL061 Technical Reference Manual > - Stellaris LM3S8962 Microcontroller Data Sheet > - Stellaris LM3S5P31 Microcontroller Data Sheet > > Signed-off-by: Wei Huang <wei@redhat.com> > --- > hw/gpio/pl061.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c > index e5a696e..342a70d 100644 > --- a/hw/gpio/pl061.c > +++ b/hw/gpio/pl061.c > @@ -284,8 +284,35 @@ static void pl061_write(void *opaque, hwaddr offset, > > static void pl061_reset(PL061State *s) > { > - s->locked = 1; > - s->cr = 0xff; > + /* reset values from PL061 TRM, Stellaris LM3S5P31 & LM3S8962 Data Sheet */ > + s->data = 0; > + s->old_out_data = 0; > + s->old_in_data = 0; > + s->dir = 0; > + s->isense = 0; > + s->ibe = 0; > + s->iev = 0; > + s->im = 0; > + s->istate = 0; > + s->afsel = 0; > + s->dr2r = 0xff; > + s->dr4r = 0; > + s->dr8r = 0; > + s->odr = 0; > + s->pur = 0; > + s->pdr = 0; > + s->slr = 0; > + s->den = 0; > + s->locked = 1; > + s->cr = 0xff; > + s->amsel = 0; > +} > + > +static void pl061_state_reset(DeviceState *dev) > +{ > + PL061State *s = PL061(dev); > + > + pl061_reset(s); > } > > static void pl061_set_irq(void * opaque, int irq, int level) > @@ -343,6 +370,7 @@ static void pl061_class_init(ObjectClass *klass, void *data) > > k->init = pl061_initfn; > dc->vmsd = &vmstate_pl061; > + dc->reset = &pl061_state_reset; > } > > static const TypeInfo pl061_info = { >
On 1 February 2016 at 20:49, Wei Huang <wei@redhat.com> wrote: > Current QEMU doesn't clear PL061 state after reset. This causes a > weird issue with guest reboot via GPIO. Here is the device state > description with two reboot requests: > > (PL061State fields) data old_in_data istate > VM boot 0 0 0 > After 1st ACPI reboot request 8 8 8 > After VM PL061 driver ACK 8 8 0 > After VM reboot 8 8 0 > ------------------------------------------------------------ > 2nd ACPI reboot request 8 > > In the second reboot request above, because old_in_data field is 8, > QEMU decides that there is a pending edge IRQ already (see > pl061_update()) in input; so it doesn't raise up IRQ again. As a result > the second reboot request is lost. The correct way is to clear PL061 > device state after reset. > > NOTE: The reset state is found from the following documentation: > - PL061 Technical Reference Manual > - Stellaris LM3S8962 Microcontroller Data Sheet > - Stellaris LM3S5P31 Microcontroller Data Sheet > > Signed-off-by: Wei Huang <wei@redhat.com> > --- > hw/gpio/pl061.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) > > diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c > index e5a696e..342a70d 100644 > --- a/hw/gpio/pl061.c > +++ b/hw/gpio/pl061.c > @@ -284,8 +284,35 @@ static void pl061_write(void *opaque, hwaddr offset, > > static void pl061_reset(PL061State *s) > { > - s->locked = 1; > - s->cr = 0xff; > + /* reset values from PL061 TRM, Stellaris LM3S5P31 & LM3S8962 Data Sheet */ > + s->data = 0; > + s->old_out_data = 0; > + s->old_in_data = 0; > + s->dir = 0; > + s->isense = 0; > + s->ibe = 0; > + s->iev = 0; > + s->im = 0; > + s->istate = 0; > + s->afsel = 0; > + s->dr2r = 0xff; > + s->dr4r = 0; > + s->dr8r = 0; > + s->odr = 0; > + s->pur = 0; > + s->pdr = 0; > + s->slr = 0; > + s->den = 0; > + s->locked = 1; > + s->cr = 0xff; > + s->amsel = 0; > +} These reset values are all OK... > + > +static void pl061_state_reset(DeviceState *dev) > +{ > + PL061State *s = PL061(dev); > + > + pl061_reset(s); > } ...but you don't need to have this wrapper function. You can just do the reset in a function called pl061_reset() with the function signature we need for dc->reset. The only place that currently calls the existing pl061_reset() is the device's init function, and you can delete that call because the Device framework automatically calls the dc->reset function after device initialization. thanks -- PMM
On 16 February 2016 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote: > On 1 February 2016 at 20:49, Wei Huang <wei@redhat.com> wrote: >> Current QEMU doesn't clear PL061 state after reset. This causes a >> weird issue with guest reboot via GPIO. Here is the device state >> description with two reboot requests: > > These reset values are all OK... > >> + >> +static void pl061_state_reset(DeviceState *dev) >> +{ >> + PL061State *s = PL061(dev); >> + >> + pl061_reset(s); >> } > > ...but you don't need to have this wrapper function. > You can just do the reset in a function called pl061_reset() > with the function signature we need for dc->reset. > The only place that currently calls the existing pl061_reset() > is the device's init function, and you can delete that call > because the Device framework automatically calls the dc->reset > function after device initialization. I know this patch doesn't (by itself) fix the issues with guest reboot, but I think it is worth having anyway because not resetting the PL061 state is a genuine bug. Can you do a v3 and resend, please? PS: please could you include a cover letter email next time round, since this is a multi patch series? Side note: half our "PL061" behaviour is actually specific to the TI variant in the Luminary, and for our plain old PL061 we ought to restrict access to the registers that are Stellaris only. But that's a different bug and not a very major one. thanks -- PMM
On 02/16/2016 08:39 AM, Peter Maydell wrote: > On 16 February 2016 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 1 February 2016 at 20:49, Wei Huang <wei@redhat.com> wrote: >>> Current QEMU doesn't clear PL061 state after reset. This causes a >>> weird issue with guest reboot via GPIO. Here is the device state >>> description with two reboot requests: > >> >> These reset values are all OK... >> >>> + >>> +static void pl061_state_reset(DeviceState *dev) >>> +{ >>> + PL061State *s = PL061(dev); >>> + >>> + pl061_reset(s); >>> } >> >> ...but you don't need to have this wrapper function. >> You can just do the reset in a function called pl061_reset() >> with the function signature we need for dc->reset. >> The only place that currently calls the existing pl061_reset() >> is the device's init function, and you can delete that call >> because the Device framework automatically calls the dc->reset >> function after device initialization. > > I know this patch doesn't (by itself) fix the issues with guest > reboot, but I think it is worth having anyway because not resetting > the PL061 state is a genuine bug. Can you do a v3 and resend, please? > > PS: please could you include a cover letter email next time round, > since this is a multi patch series? Done, please review. > > Side note: half our "PL061" behaviour is actually specific > to the TI variant in the Luminary, and for our plain old PL061 > we ought to restrict access to the registers that are Stellaris > only. But that's a different bug and not a very major one. Thanks for your suggestion. I was trying to fix it. The plan was to add a new field rsvd_addr in "struct PL061State". Then in pl061_read() and pl061_write(), we can check offset against [rsvd_addr, 0xfcc] (ignored if inside). While I was working on it, I realized that this is a benign issue. It is true that PL061 device can access Luminary registers in the reserved memory area. However QEMU doesn't use these Luminary registers anywhere else other than pl061_read() and pl061_write(). It basically passes the read/write requests through. I don't see a malicious driver can damage device state. Thoughts? Thanks, -Wei > > thanks > -- PMM >
On 17 February 2016 at 17:34, Wei Huang <wei@redhat.com> wrote: > On 02/16/2016 08:39 AM, Peter Maydell wrote: >> Side note: half our "PL061" behaviour is actually specific >> to the TI variant in the Luminary, and for our plain old PL061 >> we ought to restrict access to the registers that are Stellaris >> only. But that's a different bug and not a very major one. > > Thanks for your suggestion. I was trying to fix it. The plan was to add > a new field rsvd_addr in "struct PL061State". Then in pl061_read() and > pl061_write(), we can check offset against [rsvd_addr, 0xfcc] (ignored > if inside). > > While I was working on it, I realized that this is a benign issue. It is > true that PL061 device can access Luminary registers in the reserved > memory area. However QEMU doesn't use these Luminary registers anywhere > else other than pl061_read() and pl061_write(). It basically passes the > read/write requests through. I don't see a malicious driver can damage > device state. Thoughts? It's not a "malicious guest can do bad things" bug, it's a "modelled hardware doesn't behave like the real thing" bug. A non-Luminary PL061 should act like the hardware, which means that the registers that don't exist should be RAZ/WI (and should log guest-errors if the guest tries to access them), the same way we do in the "default" case of the case statements for other reserved registers. thanks -- PMM
diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c index e5a696e..342a70d 100644 --- a/hw/gpio/pl061.c +++ b/hw/gpio/pl061.c @@ -284,8 +284,35 @@ static void pl061_write(void *opaque, hwaddr offset, static void pl061_reset(PL061State *s) { - s->locked = 1; - s->cr = 0xff; + /* reset values from PL061 TRM, Stellaris LM3S5P31 & LM3S8962 Data Sheet */ + s->data = 0; + s->old_out_data = 0; + s->old_in_data = 0; + s->dir = 0; + s->isense = 0; + s->ibe = 0; + s->iev = 0; + s->im = 0; + s->istate = 0; + s->afsel = 0; + s->dr2r = 0xff; + s->dr4r = 0; + s->dr8r = 0; + s->odr = 0; + s->pur = 0; + s->pdr = 0; + s->slr = 0; + s->den = 0; + s->locked = 1; + s->cr = 0xff; + s->amsel = 0; +} + +static void pl061_state_reset(DeviceState *dev) +{ + PL061State *s = PL061(dev); + + pl061_reset(s); } static void pl061_set_irq(void * opaque, int irq, int level) @@ -343,6 +370,7 @@ static void pl061_class_init(ObjectClass *klass, void *data) k->init = pl061_initfn; dc->vmsd = &vmstate_pl061; + dc->reset = &pl061_state_reset; } static const TypeInfo pl061_info = {
Current QEMU doesn't clear PL061 state after reset. This causes a weird issue with guest reboot via GPIO. Here is the device state description with two reboot requests: (PL061State fields) data old_in_data istate VM boot 0 0 0 After 1st ACPI reboot request 8 8 8 After VM PL061 driver ACK 8 8 0 After VM reboot 8 8 0 ------------------------------------------------------------ 2nd ACPI reboot request 8 In the second reboot request above, because old_in_data field is 8, QEMU decides that there is a pending edge IRQ already (see pl061_update()) in input; so it doesn't raise up IRQ again. As a result the second reboot request is lost. The correct way is to clear PL061 device state after reset. NOTE: The reset state is found from the following documentation: - PL061 Technical Reference Manual - Stellaris LM3S8962 Microcontroller Data Sheet - Stellaris LM3S5P31 Microcontroller Data Sheet Signed-off-by: Wei Huang <wei@redhat.com> --- hw/gpio/pl061.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)