Re: [Qemu-devel] [kvm-unit-tests PATCH v5 10/11] arm/arm64: gicv3: add an IPI test

[Alex Bennée]

Andrew Jones <address@hidden> writes:

> On Fri, Nov 11, 2016 at 10:02:59AM +0000, Alex Bennée wrote:
>>
>> Andrew Jones <address@hidden> writes:
>>
>> > On Thu, Nov 10, 2016 at 07:53:58PM +0000, Alex Bennée wrote:
>> > [...]
>> >> > +struct gic gicv2 = {
>> >> > +       .ipi = {
>> >> > +               .enable = gicv2_enable_defaults,
>> >> > +               .send_self = gicv2_ipi_send_self,
>> >> > +               .send_tlist = gicv2_ipi_send_tlist,
>> >> > +               .send_broadcast = gicv2_ipi_send_broadcast,
>> >> > +       },
>> >> > +       .read_iar = gicv2_read_iar,
>> >> > +       .irqnr = gicv2_irqnr,
>> >> > +       .write_eoi = gicv2_write_eoi,
>> >> > +};
>> >> > +
>> >> > +struct gic gicv3 = {
>> >> > +       .ipi = {
>> >> > +               .enable = gicv3_enable_defaults,
>> >> > +               .send_self = gicv3_ipi_send_self,
>> >> > +               .send_tlist = gicv3_ipi_send_tlist,
>> >> > +               .send_broadcast = gicv3_ipi_send_broadcast,
>> >> > +       },
>> >> > +       .read_iar = gicv3_read_iar,
>> >> > +       .irqnr = gicv3_irqnr,
>> >> > +       .write_eoi = gicv3_write_eoir,
>> >> > +};
>> >> > +
>> >>
>> >> So I was re-basing my kvm-unit-tests against your GIC rework and found
>> >> myself copy and pasting a bunch of this into my tests that fire IRQs.
>> >> That makes me think the abstraction should be in the library code so
>> >> other tests can fiddle with sending IRQs.
>> >>
>> >> What do you think?
>> >>
>> >
>> > I guess you mean moving the above two structs and their corresponding
>> > functions (all which aren't already common) to lib/arm/ ? Or do you
>> > just mean the one non-trivial function gicv3_ipi_send_tlist? I think
>> > agree with gicv3_ipi_send_tlist getting shared, but the others are
>> > mostly one-liners, so I'm not sure. I guess I'd have to see how you're
>> > using them first.
>>
>> So it looked like there were some functions in the common code for one
>> GIC which had local test defined functions for the other. They should at
>> least be consistent.
>
> gicv3_read_iar and gicv3_write_eoir being common already is a product of
> being sysreg wrappers, allowing for both arm32 and arm64 to use functions
> of the same names, not because I wanted gicv3 to be inconsistent with
> gicv2 (which uses MMIO and thus doesn't need wrappers)
>
>>
>> For my use case I could do with a common:
>>
>>   gic_enable
>
> OK, I can extend gic_init() to initialize a 'struct gic_common_ops' that
> includes an enable -> *_enable_defaults(void), ipi_send(int cpu),
> read_iar(void), iar_irqnr(u32 iar), and write_eoi(u32 irqstat). And also
> provide the wrappers gic_enable, gic_ipi_send(cpu), ...
>
>>   gic_send_spi(cpu, irq)
>
> I'll let you add this one to the new common ops struct :-)
>
>>   gic_irq_ack() which returns the iar.
>
> This one will be called read_iar.
>
> Would that work for you, Alex?

Sounds good to me :-)

>
> Andre,
>
> Would this also satisfy your needs for more common code?
>
> Thanks,
> drew
>
>>
>> See:
>>
>>   
>> https://github.com/stsquad/kvm-unit-tests/blob/mttcg/current-tests-v6/arm/tcg-test.c#L113
>>
>> >
>> > Thanks,
>> > drew
>>
>>
>> --
>> Alex Bennée
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to address@hidden
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
Alex Bennée