On 08/01/2022 17:28, BALATON Zoltan wrote:
On Sat, 8 Jan 2022, BALATON Zoltan wrote:
On Sat, 8 Jan 2022, Mark Cave-Ayland wrote:
On 06/11/2021 16:52, BALATON Zoltan wrote:
Signed-off-by: BALATON Zoltan balaton@eik.bme.hu
drivers/usbohci.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/usbohci.c b/drivers/usbohci.c index 774164b..c2c0f78 100644 --- a/drivers/usbohci.c +++ b/drivers/usbohci.c @@ -232,12 +232,12 @@ ohci_init (void *bar) usb_debug("HCCA addr %p\n", OHCI_INST(controller)->hcca); /* Initialize interrupt table. */ - u32 *const intr_table = OHCI_INST(controller)->hcca->HccaInterruptTable; + ohci_t *const ohci = OHCI_INST(controller); ed_t *const periodic_ed; ofmem_posix_memalign((void **)&periodic_ed, sizeof(ed_t), sizeof(ed_t)); memset((void *)periodic_ed, 0, sizeof(*periodic_ed)); for (i = 0; i < 32; ++i) - intr_table[i] = __cpu_to_le32(virt_to_phys(periodic_ed)); + ohci->hcca->HccaInterruptTable[i] = __cpu_to_le32(virt_to_phys(periodic_ed)); OHCI_INST (controller)->periodic_ed = periodic_ed;
The changes to intr_table appear to make things less readable to me.
Yes but I could not find a simpler way to fix this easily.
Presumably the error is that the existing code is attempting to write through a const pointer? In which case I think simply dropping the const from
No, it's about possible unalligned access, see my answer to Philippe.
intr_table both here and below should fix the issue, since it is clearly intended to be used for writing.
I don't think that would fix the problem.
Ah I see, I hadn't spotted the warnings in the reply (I think our replies must have crossed on the mailing list).
So indeed it is related to unaligned accesses. A bit of research reveals that the issue is caused because when a struct is marked as packed, gcc doesn't guarantee that even the first member of the struct is aligned which is something I was not aware of, and was clearly an assumption made by the existing code.
The outcome of this is that the struct access must be done using a -> reference since presumably if the compiler decides to align the struct arbitrarily, it can emit code to correctly handle unaligned reads/writes.
OHCI_INST (controller)->opreg->HcHCCA = __cpu_to_le32(virt_to_phys(OHCI_INST(controller)->hcca)); @@ -607,7 +607,7 @@ struct _intrq_td { u8 *data; struct _intrq_td *next; struct _intr_queue *intrq; -}; +} __attribute__ ((packed));
This looks like a different fix that should be in a separate patch (wasn't there a similar patch for QEMU recently?).
And due to the ast this struct has the same issue which is why it needs to packed too.
It's also fixing a build issue with gcc 10 and the problem is similar so I've
Maybe this wasn't clear enough so I try again to explain. This is only fixing the gcc 10 error, it's not related to any QEMU change. If there was a similar change in QEMU then that's unrelated, I don't remember any change in QEMU that needed this fix apart from the error from gcc which is solely within openbios code so I think it belongs to this patch but as I said I'm OK with you commiting it in a different patch as well.
No, that all makes sense in the context of the warnings. I'll give it a quick test and if the USB mouse/keyboard seem okay on mac99 then I'll push it to master. If you find a similar issue in future, please do include the generated warnings in the commit message as it makes patches much easier to review.
ATB,
Mark.