Re: [Qemu-devel] [Qemu-arm] [RFC PATCH 15/17] Convert zynq's slcr to 3-phases reset

[Damien Hedde]

Hi Philippe,

On 3/25/19 2:28 PM, Philippe Mathieu-Daudé wrote:
> Hi Damien,
> 
> Le lun. 25 mars 2019 12:16, Damien Hedde  > a écrit :
> 
> Change the legacy reset function into the init phase and test the
> resetting flag in register accesses.
> 
> Signed-off-by: Damien Hedde  >
> ---
>  hw/misc/zynq_slcr.c | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> [...]
> @@ -346,6 +346,10 @@ static uint64_t zynq_slcr_read(void *opaque,
> hwaddr offset,
>      offset /= 4;
>      uint32_t ret = s->regs[offset];
> 
> +    if (qdev_is_resetting((DeviceState *) opaque)) {
> +        return 0;
> +    }
> +
>      if (!zynq_slcr_check_offset(offset, true)) {
>          qemu_log_mask(LOG_GUEST_ERROR, "zynq_slcr: Invalid read
> access to "
>                        " addr %" HWADDR_PRIx "\n", offset * 4);
> @@ -361,6 +365,10 @@ static void zynq_slcr_write(void *opaque,
> hwaddr offset,
>      ZynqSLCRState *s = (ZynqSLCRState *)opaque;
>      offset /= 4;
> 
> +    if (qdev_is_resetting((DeviceState *) opaque)) {
> +        return;
> 
> 
> I wonder if that could be moved to generic parent, as an abstract method. 
> But then I'm not sure we can use a generic read() return value. 
> 

I agree it would be better not have to add this kind of test everywhere.
It is one my unresolved problem but I am not sure what is the best solution.

There is two approachs here I think:
Either we disable/enable explicitely the memory regions in the reset
handlers. But then we have to handle the migration of the memory regions
flags.
Or the memory region handlers somehow check the resetting state (like
here) and we have only to handle the resetting state migration.

Memory region are added using the following code in sysbus:
```
memory_region_init_io(>iomem, obj, _ops, s, "uart", 0x1000);
sysbus_init_mmio(sbd, >iomem);
sysbus_mmio_map(s, mmio_index, addr);
```
To intercept the read and write without needing a modification of every
read/write handlers, I see two options:

+ we can either find a way to "override" the read and write handlers of
the memory regions. For example, this could be done in sysbus_init_mmio,
or in a dedicated version of the function (eg sysbus_init_mmio_with_reset)

+ or we can use the MemoryRegion owner field (here _obj_ in
memory_region_init_io call) to test if the owner is Resettable and
whether it is resetting or not before calling the handlers. In that
case, this will not be limited to sysbus devices.

--
Thanks
Damien

Re: [Qemu-devel] [Qemu-arm] [RFC PATCH 15/17] Convert zynq's slcr to 3-phases reset

[Philippe Mathieu-Daudé]

Hi Damien,

Le lun. 25 mars 2019 12:16, Damien Hedde  a
écrit :

> Change the legacy reset function into the init phase and test the
> resetting flag in register accesses.
>
> Signed-off-by: Damien Hedde 
> ---
>  hw/misc/zynq_slcr.c | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
> index baa13d1316..47f43e1d8d 100644
> --- a/hw/misc/zynq_slcr.c
> +++ b/hw/misc/zynq_slcr.c
> @@ -180,9 +180,9 @@ typedef struct ZynqSLCRState {
>  uint32_t regs[ZYNQ_SLCR_NUM_REGS];
>  } ZynqSLCRState;
>
> -static void zynq_slcr_reset(DeviceState *d)
> +static void zynq_slcr_reset_init(Object *obj, bool cold)
>  {
> -ZynqSLCRState *s = ZYNQ_SLCR(d);
> +ZynqSLCRState *s = ZYNQ_SLCR(obj);
>  int i;
>
>  DB_PRINT("RESET\n");
> @@ -346,6 +346,10 @@ static uint64_t zynq_slcr_read(void *opaque, hwaddr
> offset,
>  offset /= 4;
>  uint32_t ret = s->regs[offset];
>
> +if (qdev_is_resetting((DeviceState *) opaque)) {
> +return 0;
> +}
> +
>  if (!zynq_slcr_check_offset(offset, true)) {
>  qemu_log_mask(LOG_GUEST_ERROR, "zynq_slcr: Invalid read access to
> "
>" addr %" HWADDR_PRIx "\n", offset * 4);
> @@ -361,6 +365,10 @@ static void zynq_slcr_write(void *opaque, hwaddr
> offset,
>  ZynqSLCRState *s = (ZynqSLCRState *)opaque;
>  offset /= 4;
>
> +if (qdev_is_resetting((DeviceState *) opaque)) {
> +return;
>

I wonder if that could be moved to generic parent, as an abstract method.
But then I'm not sure we can use a generic read() return value.

+}
> +
>  DB_PRINT("addr: %08" HWADDR_PRIx " data: %08" PRIx64 "\n", offset *
> 4, val);
>
>  if (!zynq_slcr_check_offset(offset, false)) {
> @@ -441,7 +449,7 @@ static void zynq_slcr_class_init(ObjectClass *klass,
> void *data)
>  DeviceClass *dc = DEVICE_CLASS(klass);
>
>  dc->vmsd = _zynq_slcr;
> -dc->reset = zynq_slcr_reset;
> +dc->reset_phases.init = zynq_slcr_reset_init;
>  }
>
>  static const TypeInfo zynq_slcr_info = {
> --
> 2.21.0
>

Reviewed-by: Philippe Mathieu-Daudé 

>