Re: [PULL 23/35] hw/intc: Rework Loongson LIOINTC

[BALATON Zoltan]

On Mon, 11 Jan 2021, Peter Maydell wrote:
> On Mon, 11 Jan 2021 at 10:20, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > On Mon, 11 Jan 2021, Jiaxun Yang wrote:
> > > On Mon, Jan 11, 2021, at 8:36 AM, Huacai Chen wrote:
> > > > I think R_END should be 0x60, Jiaxun, what do you think?
> > >
> > > U r right.
> > > The manual is misleading.
> >
> > The R_END constant is also used in loongson_liointc_init() for the length
> > of the memory region so you might want to revise that. If this is a 32 bit
> > register then you should decide what R_END means? Is it the end of the
> > memory region in which case the reg starts at R_END - 4 or is it the
> > address of the last reg in which case the memory region ends at R_END + 4.
> > From the above I think it's the address of the last reg so you'll probably
> > need to add 4 in loongson_liointc_init() when creating the memory region.
>
> Mmm, or check
>  (addr >= R_START && addr < (R_START + R_ISR_SIZE * NUM_CORES))

That was basically the original version just hiding the calculation in a 
macro so we could also just drop this part of the patch and be happy with 
it :-) "If it ain't broke, don't fix it"

> Side note: R_ISR_SIZE is 8, but the code makes both the
> 32-bit addresses you can read/write in that 8-byte range
> behave the same way. Is that really what the hardware does ?
> Or does it actually have 1 32-bit register per core, spaced
> 8 bytes apart ?

This might turn into a bike shed thread but I just thought: would 
range_covers_byte() or ranges_overlap() be useful here to test if addr is 
within the regs area? I've used that in vt82c686.c::pm_write_config(). 
That might actually be simpler if that worked.

Regards,
BALATON Zoltan