Re: [Qemu-arm] [RFC v2 2/4] hw/intc/arm_gicv3_its: Implement a minimalist reset

[Peter Maydell]

On 23 October 2017 at 16:35, Eric Auger <address@hidden> wrote:
> At the moment the ITS is not properly reset and this causes
> various bugs on save/restore. We implement a minimalist cold reset
> through individual register writes. This does not void the ITS cache
> though. This full reset will be addressed through a specific ioctl.
>
> Signed-off-by: Eric Auger <address@hidden>

> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
> index 1ae205f..537cea1 100644
> --- a/hw/intc/arm_gicv3_its_kvm.c
> +++ b/hw/intc/arm_gicv3_its_kvm.c
> @@ -28,6 +28,16 @@
>
>  #define TYPE_KVM_ARM_ITS "arm-its-kvm"
>  #define KVM_ARM_ITS(obj) OBJECT_CHECK(GICv3ITSState, (obj), TYPE_KVM_ARM_ITS)
> +#define KVM_ARM_ITS_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(KVMARMITSClass, (klass), TYPE_KVM_ARM_ITS)
> +#define KVM_ARM_ITS_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(KVMARMITSClass, (obj), TYPE_KVM_ARM_ITS)
> +
> +typedef struct KVMARMITSClass {
> +    GICv3ITSCommonClass parent_class;
> +    void (*parent_reset)(DeviceState *dev);
> +} KVMARMITSClass;
> +

The infrastructure here for having a place to hang reset is
all fine, but it's a bit out of place because this patch doesn't
actually use any of it.

>  static int kvm_its_send_msi(GICv3ITSState *s, uint32_t value, uint16_t devid)
>  {
> @@ -153,12 +163,25 @@ static void kvm_arm_its_pre_save(GICv3ITSState *s)
>   */
>  static void kvm_arm_its_post_load(GICv3ITSState *s)
>  {
> +    bool cold_reset = !s->iidr;
>      int i;
>
> -    if (!s->iidr) {
> +    if (cold_reset) {
> +        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                          GITS_CTLR, &s->ctlr, true, &error_abort);
> +
> +        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                          GITS_CBASER, &s->cbaser, true, &error_abort);
> +
> +        for (i = 0; i < 8; i++) {
> +            kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                              GITS_BASER + i * 8, &s->baser[i], true,
> +                              &error_abort);
> +        }
>          return;
>      }

I don't understand why we need to treat the "iidr zero" case
differently here. Why can't we just restore the register state
like we do now? (Also, all resets in QEMU are cold resets,
and post_load isn't called for reset, so the flag naming is
a bit odd.)

(I don't understand why the code in master at the moment does
nothing for iidr == 0, for that matter. We should always tell
the kernel the state of the device regs.)

thanks
-- PMM