Re: [Qemu-arm] [RFC 3/3] hw/intc/arm_gicv3_its: Implement reset

[Peter Maydell]

On 27 September 2017 at 15:56, Eric Auger <address@hidden> wrote:
> Currently the ITS is not reset and this causes trouble
> when state backup is initiated before the guest has initialized
> the ITS registers and especially GITS_CBASER<n>.
>
> We are likely to save register values set before the reset/
> restart. The register values may not be consistent with the
> data structures in RAM.
>
> So let's use the ITS KVM device new combo,
> KVM_DEV_ARM_VGIC_GRP_CTRL/KVM_DEV_ARM_ITS_CTRL_RESET
> to explicitly force the in-kernel emulated reset.
>
> Signed-off-by: Eric Auger <address@hidden>
> ---
>  hw/intc/arm_gicv3_its_common.c         |  5 ++---
>  hw/intc/arm_gicv3_its_kvm.c            | 22 ++++++++++++++++++----
>  include/hw/intc/arm_gicv3_its_common.h |  1 +
>  3 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
> index 68b20fc..a2fe561 100644
> --- a/hw/intc/arm_gicv3_its_common.c
> +++ b/hw/intc/arm_gicv3_its_common.c
> @@ -129,15 +129,14 @@ static void gicv3_its_common_reset(DeviceState *dev)
>      s->creadr = 0;
>      s->iidr = 0;
>      memset(&s->baser, 0, sizeof(s->baser));
> -
> -    gicv3_its_post_load(s, 0);

This doesn't look right as it means we won't write the QEMU
initial device register values up to the kernel. I think we
want to do that as well as call the specific reset ioctl,
so that both userspace and the kernel are consistent in
their idea of what's going on.

>  }
>
>  static void gicv3_its_common_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_CLASS(klass);
>
> -    dc->reset = gicv3_its_common_reset;
> +    c->parent_reset = gicv3_its_common_reset;
>      dc->vmsd = &vmstate_its;
>  }

This isn't how we handle this for the arm_gicv3_kvm.c and arm_gic_kvm.c
code which has a subclass reset/parent class reset. What we do there is:
 * the parent_reset field is in the subclass's Class struct
 * the subclass's reset function calls the parent_reset function
 * the subclass's class_init function sets parent_reset to whatever
   the old dc->reset was before setting dc->reset to its own reset
   function

I think we should be consistent in how we do this.

> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
> index 120b86d..3c2e724 100644
> --- a/hw/intc/arm_gicv3_its_kvm.c
> +++ b/hw/intc/arm_gicv3_its_kvm.c
> @@ -156,10 +156,6 @@ static void kvm_arm_its_post_load(GICv3ITSState *s)
>      Error *err = NULL;
>      int i;
>
> -    if (!s->iidr) {
> -        return;
> -    }
> -

This looks like an unrelated change, or at least not one mentioned
in the commit message?

>      kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>                        GITS_IIDR, &s->iidr, true, &error_abort);
>
> @@ -195,6 +191,23 @@ static void kvm_arm_its_post_load(GICv3ITSState *s)
>                        GITS_CTLR, &s->ctlr, true, &error_abort);
>  }
>
> +static void kvm_arm_its_reset(DeviceState *dev)
> +{
> +    GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
> +    GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_GET_CLASS(s);
> +
> +    c->parent_reset(dev);
> +
> +    if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +                               KVM_DEV_ARM_ITS_CTRL_RESET)) {
> +        error_report("ITS KVM: reset is not supported by the kernel");
> +        return;
> +    }
> +
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> +                      KVM_DEV_ARM_ITS_CTRL_RESET, NULL, true, &error_abort);
> +}
> +
>  static Property kvm_arm_its_props[] = {
>      DEFINE_PROP_LINK("parent-gicv3", GICv3ITSState, gicv3, "kvm-arm-gicv3",
>                       GICv3State *),
> @@ -211,6 +224,7 @@ static void kvm_arm_its_class_init(ObjectClass *klass, 
> void *data)
>      icc->send_msi = kvm_its_send_msi;
>      icc->pre_save = kvm_arm_its_pre_save;
>      icc->post_load = kvm_arm_its_post_load;
> +    dc->reset = kvm_arm_its_reset;
>  }
>
>  static const TypeInfo kvm_arm_its_info = {
> diff --git a/include/hw/intc/arm_gicv3_its_common.h 
> b/include/hw/intc/arm_gicv3_its_common.h
> index fd1fe64..c158e9f 100644
> --- a/include/hw/intc/arm_gicv3_its_common.h
> +++ b/include/hw/intc/arm_gicv3_its_common.h
> @@ -79,6 +79,7 @@ struct GICv3ITSCommonClass {
>      int (*send_msi)(GICv3ITSState *s, uint32_t data, uint16_t devid);
>      void (*pre_save)(GICv3ITSState *s);
>      void (*post_load)(GICv3ITSState *s);
> +    void (*parent_reset)(DeviceState *dev);
>  };
>
>  typedef struct GICv3ITSCommonClass GICv3ITSCommonClass;
> --
> 2.5.5
>

thanks
-- PMM