Re: [Qemu-devel] [RFC PATCH v3 1/5] hw/intc: Implement ITS base class

[Pavel Fedin]

 Hello!

> > +    /* Our two regions are always adjacent, therefore we now combine them
> > +     * into a single one in order to make our users' life easier.
> > +     */
> > +    memory_region_init(&s->iomem_main, OBJECT(s), "gicv3_its", ITS_SIZE);
> > +    memory_region_add_subregion(&s->iomem_main, 0, &s->iomem_its_cntrl);
> > +    memory_region_add_subregion(&s->iomem_main, ITS_CONTROL_SIZE,
> > +                                &s->iomem_its);
> > +    sysbus_init_mmio(sbd, &s->iomem_main);
> 
> So we have two memory subregions:
>  * the control register page, whose ops are passed in by the subclass
>  * the translation register page, whose only register is implemented
>    here by looking up the subclass and calling its send_msi method
> 
> Does that complexity gain us anything later? There doesn't really
> seem to be anything much actually in common here, which would
> suggest just having the subclasses create their own mmio region(s)
> (which could then just directly implement the right behaviour for
> GITS_TRANSLATER).

 I started from this, but decided to move region creation into base class, 
because:
1. We always have this region, both for KVM and for SW implementation, and it 
operates in the same way.
2. We always need to do address validation, as well as have 
gicv3_its_trans_read() stub.
3. Also, in the next revision i'll implement 16-bit handling here.

 So, i decided not to duplicate these things.

> > +
> > +const char *its_class_name(void)
> > +{
> > +    if (kvm_irqchip_in_kernel()) {
> > +#ifdef TARGET_AARCH64
> > +        /* KVM implementation requires this capability */
> > +        if (kvm_direct_msi_enabled()) {
> > +            return "arm-its-kvm";
> > +        }
> > +#endif
> 
> Why is this in an #ifdef? In theory we could support
> the GICv3 in a 32-bit system.

 Only for code consistency, because "arm-its-kvm" class is compiled only for 
TARGET_AARCH64, because only there we have the relevant kernel API definitions. 
So, i did not want to refer to nonexistent class.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia