Re: [PATCH 09/19] hw/intc/xilinx_intc: Only expect big-endian accesses

+Michal for Linux driver On 5/11/24 23:08, Edgar E. Iglesias wrote: This model is specified as: - https://docs.amd.com/v/u/en-US/xps_intc LogiCORE IP XPS Interrupt Controller (v2.01a) DS572 April 19, 2010 Spec is from 2010, you added it in 2009: commit 17628bc642260df3a07b9df8b8a9ca7da2e7e87c Author: Edgar E. Iglesias <edgar.iglesias(a)gmail.com&gt; Date: Wed May 20 20:11:30 2009 +0200 xilinx: Add interrupt controller. The spec is explicit: "The XPS INTC registers are read as big-endian data" The other model you mention is: - https://docs.amd.com/v/u/en-US/ds747_axi_intc LogiCORE IP AXI INTC (v1.04a) DS747 June 19, 2013 The spec is more recent than your addition, and contains the Interrupt Vector Address Register (IVAR) which is not present in our model. Indeed the latter seems to extend the former, but they are not the same and we need some work to model the latter. The endianness explicit for each model (and is not listed in the "IP Configurable Design Parameters" tables). That said, let's look at Linux use. Driver was added in: commit eedbdab99fffb8ed71cac75a722088b8ace2583c Author: Michal Simek <monstr(a)monstr.eu&gt; Date: Fri Mar 27 14:25:49 2009 +0100 microblaze_v8: Interrupt handling and timer support Using explicit big-endian API: +void __init init_IRQ(void) +{ ... + /* + * Disable all external interrupts until they are + * explicity requested. + */ + out_be32(intc_baseaddr + IER, 0); + + /* Acknowledge any pending interrupts just in case. */ + out_be32(intc_baseaddr + IAR, 0xffffffff); + + /* Turn on the Master Enable. */ + out_be32(intc_baseaddr + MER, MER_HIE | MER_ME); Then the driver became clever in: commit 1aa1243c339d4c902c0f9c1ced45742729a86e6a Author: Michal Simek <michal.simek(a)amd.com&gt; Date: Mon Feb 24 14:56:32 2014 +0100 microblaze: Make intc driver endian aware Detect endianess directly on the hardware and use ioread/iowrite functions. +static void intc_write32(u32 val, void __iomem *addr) +{ + iowrite32(val, addr); +} + +static void intc_write32_be(u32 val, void __iomem *addr) +{ + iowrite32be(val, addr); +} @@ -140,17 +163,25 @@ static int __init xilinx_intc_of_init(struct device_node *intc, + write_fn = intc_write32; + read_fn = intc_read32; + /* * Disable all external interrupts until they are * explicity requested. */ - out_be32(intc_baseaddr + IER, 0); + write_fn(0, intc_baseaddr + IER); /* Acknowledge any pending interrupts just in case. */ - out_be32(intc_baseaddr + IAR, 0xffffffff); + write_fn(0xffffffff, intc_baseaddr + IAR); /* Turn on the Master Enable. */ - out_be32(intc_baseaddr + MER, MER_HIE | MER_ME); + write_fn(MER_HIE | MER_ME, intc_baseaddr + MER); + if (!(read_fn(intc_baseaddr + MER) & (MER_HIE | MER_ME))) { + write_fn = intc_write32_be; + read_fn = intc_read32_be; + write_fn(MER_HIE | MER_ME, intc_baseaddr + MER); + } Interestingly little endianness became the default, although the driver detect it on init. This is from 2014, maybe to work with the "LogiCORE IP AXI INTC" you mentioned, which spec date is 2013. Indeed when forcing different endianness [*], the Linux kernel used in our tests (tests/functional/test_microblaze*) does the check and ends up using the correct INTC endianness: LE CPU, LE INTC model pic_write addr=8 val=0 pic_write addr=c val=ffffffff pic_write addr=1c val=3 <- LE pic_read 1c=3 pic_write addr=c val=1 pic_write addr=10 val=1 pic_read 18=0 LE CPU, BE INTC model pic_write addr=8 val=0 pic_write addr=c val=ffffffff pic_write addr=1c val=3000000 <- LE test pic_read 1c=0 pic_write addr=1c val=3 <- BE pic_write addr=c val=1 pic_write addr=10 val=1 pic_read 18=0 BE CPU, BE INTC model pic_write addr=8 val=0 pic_write addr=c val=ffffffff pic_write addr=1c val=3000000 <- LE test pic_read 1c=0 pic_write addr=1c val=3 <- BE pic_write addr=c val=1 pic_write addr=10 val=1 pic_read 18=0 BE CPU, LE INTC model pic_write addr=8 val=0 pic_write addr=c val=ffffffff pic_write addr=1c val=3 <- LE pic_read 1c=3 pic_write addr=c val=1 pic_write addr=10 val=1 pic_read 18=0 IMHO this patch behavior is correct. Besides, I don't expect firmwares to be as clever as Linux. What you suggested is implemented in v3: https://lore.kernel.org/qemu-devel/20241108154317.12129-4-philmd@linaro.org/ but after the analysis, I wonder if it isn't safer to not make the endianness configurable, but expose as 2 models: - xlnx,xps_intc (2010) in BE - xlnx,axi_intc (2013) in LE [*] diff used: -- >8 -- @@ -32,7 +45,7 @@ diff --git a/hw/intc/xilinx_intc.c b/hw/intc/xilinx_intc.c index fc55c8afca..883ec685f4 100644 --- a/hw/intc/xilinx_intc.c +++ b/hw/intc/xilinx_intc.c #include "hw/qdev-properties.h" #include "qom/object.h" -#define D(x) +#define D(x) x #define R_ISR 0 #define R_IPR 1 @@ -105,7 +122,7 @@ static uint64_t pic_read(void *opaque, hwaddr addr, unsigned int size) break; } - D(printf("%s %x=%x\n", __func__, addr * 4, r)); + D(printf("%s %llx=%x\n", __func__, addr * 4, r)); return r; } @@ -116,7 +133,7 @@ static void pic_write(void *opaque, hwaddr addr, uint32_t value = val64; addr >>= 2; - D(qemu_log("%s addr=%x val=%x\n", __func__, addr * 4, value)); + D(printf("%s addr=%llx val=%x\n", __func__, addr * 4, value)); switch (addr) { case R_IAR: ---