Re: [PATCH v2 00/13] arm gicv3 ITS: Various bug fixes and refactorings

[Andre Przywara <andre.przywara@arm.com>]

On Wed, 19 Jan 2022 10:15:52 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

Hi Peter,

> On Tue, 18 Jan 2022 at 23:30, Andre Przywara <andre.przywara@arm.com> wrote:
> > Looking at k-u-t's arm/gic.c and QEMU's hw/intc/arm_gic.c I see two
> > problems here: QEMU implements word accesses as four successive calls to
> > gic_dist_readb() - which is probably fine if that helps code design,
> > but it won't allow it to actually spot access size issues. I just
> > remember that we spent some brain cells and CPP macros on getting the
> > access size right in KVM - hence those tests in kvm-unit-tests.  
> 
> Thanks for looking at this. I should have read the code rather
> than dashing off a reply last thing in the evening based just
> on the test case output! I think I was confusing how our GICv3
> emulation handles register accesses (with separate functions for
> byte/halfword/word/quad accesses) with the GICv2 emulation
> (which as you say calls down into the byte emulation code
> wherever it can).

No worries!

> > But more importantly it looks like GICD_IIDR is actually not
> > implemented: There is a dubious "if (offset < 0x08) return 0;" line,
> > but IIDR (offset 0x8) would actually fall through, and hit the bad_reg
> > label, which would return 0 (and cause the message, if enabled).  
> 
> Mmm. I actually have a patch in target-arm.next from Petr Pavlu
> which implements GICC_IIDR, but we do indeed not implement the
> distributor equivalent.

Well, returning 0 is actually not the worst idea. Using proper ID
values might not even be feasible for QEMU, or would create some hassle
with versioning. With 0 all a user can assume is spec compliance.

> > If that helps: from a GIC MMIO perspective 8-bit accesses are actually
> > the exception rather than the norm (ARM IHI 0048B.b 4.1.4 GIC register
> > access).  
> 
> Yes. We got this right in the GICv3 emulation design, where almost
> all the logic is in the 32-bit accessor functions and the 8/16-bit
> functions deal only with the very few registers that have to
> permit non-word accesses.

Indeed. I dusted off my old GICv3 MMIO patches for kvm-unit-tests, and
QEMU passed with flying colours. With the debug switch I see it
reporting exactly the violating accesses we except to see.
Will send those patches ASAP.

> The GICv2 code is a lot older (and to
> be fair to it, started out as 11MPcore interrupt controller
> emulation, and I bet the docs of that were not very specific about
> what registers could or could not be accessed byte at a time).
> Unless we want to rewrite all that logic in the GICv2 emulation
> (which I at least do not :-))

... and I can't ...

> I think we'll have to live with
> the warnings about bad-offsets reporting for each byte rather
> than just once for the word access.

Yeah, if those warnings appear only with that debug switch, there is
probably little reason to change that code just because of this. At
least it seemed to work quite well over the years.

Cheers,
Andre

P.S. I changed k-u-t to special case the UP case, so that TCG passes.
But now KVM fails, of course. So I will have to make a patch for the
kernel ...