On 30.08.21 05:02, Z.Q. Hou wrote: > Hi Jan, > >> -----Original Message----- >> From: Jan Kiszka [mailto:[email protected]] >> Sent: 2021年8月27日 15:53 >> To: Z.Q. Hou <[email protected]>; [email protected] >> Subject: Re: [PATCH] arm: irqchip: Fix the mask according to access address >> and >> size >> >> On 27.08.21 09:51, Zhiqiang Hou wrote: >>> From: Hou Zhiqiang <[email protected]> >>> >>> In function restrict_bitmask_access(), the current access_mask is >>> implicitly assuming that it always access the whole register, but some >>> registers are byte-accessible, the Guest may get/set wrong value when >>> it issue a byte or halfword access to these registers. >> >> Can you be more specific in the affected scenarios? At least one example >> would >> be good. > > Let take GICD_IPRIORITYR as an example, which is a byte-accessible register. > Thinking the SPI INTID 33 is assigned to an inmate cell, and Guest wants to > set the priority of 33 by byte-accessing. > > Assuming: > GICD_IPRIORITY8 original value: 0x00000000 > Guest set value: 0xa0 > > Then the current logic like: > The access_mask is calculated as 0xff00, GICD_IPRIORITY8 read back value is > 0x00. Then 0x00 & ~(0xff00) get 0x00, 0x00 | (0xa0 & 0xff00) get 0x00, then > finally 0x00 will be written back. > > With this fix, the access_mask will become (0xff00 >> 8) & 0xff = 0xff, so > finally the 0xa0 will be written back. >
OK, indeed a good point. We missed the byte-accessability of GICD_IPRIORITYR. But this reveals more: We perform a read-modify-write cycle on behalf of the cell with the cell's idea of access width. That is potentially unsafe. Handling of GICD_ITARGETSR has to resolve the same issue, and it does that by converting the request unconditionally into a word-access. I think we should do the same here as well. gic_handle_dist_access should provide a requested access_mask to restrict_bitmask_access. That will be size and address derived for GICD_IPRIORITYR and all-1s for the other registers. Then restrict_bitmask_access would cut this down (rather than building it up) based on the access rights of the cell. restrict_bitmask_access would furthermore ignore any accesses that are not word-sized, thus gic_handle_dist_access should convert the GICD_IPRIORITYR into a word access, just like gicv2_handle_irq_target does. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jailhouse-dev/dd785b10-ac8d-f483-7483-f8830ed6147f%40siemens.com.
