On 31.08.21 08:55, Z.Q. Hou wrote: > > >> -----Original Message----- >> From: Jan Kiszka <[email protected]> >> Sent: 2021年8月30日 20:03 >> To: Z.Q. Hou <[email protected]>; [email protected] >> Cc: Jiafei Pan <[email protected]>; Rui Sousa <[email protected]> >> Subject: Re: [PATCH] arm: irqchip: Fix the mask according to access address >> and size >> >> On 30.08.21 12:54, Z.Q. Hou wrote: >>> >>> >>>> -----Original Message----- >>>> From: Jan Kiszka [mailto:[email protected]] >>>> Sent: 2021年8月30日 13:43 >>>> To: Z.Q. Hou <[email protected]>; >> [email protected] >>>> Subject: Re: [PATCH] arm: irqchip: Fix the mask according to access >>>> address and size >>>> >>>> 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. >>> >>> Can you help understand why there is still potential unsafe with this fix >> patch? >>> >> >> The patch is fine for GICD_IPRIORITYR, so is accepting all accesses for that >> register. The problem is with other registers that do not support byte >> accesses. > > Don't worry, according to the programmer guide the Guest code must not access > those registers with Byte-width; even if there is someone do that by mistake, > it will trigger an exception before the access is trapped to EL2. >
I do worry: We trap the access to EL2 because the memory region is not backed by memory in the first stage translation. And then EL2 will issue that request and panic. 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/4d107c19-ba44-c5db-9452-1552e0b66c93%40siemens.com.
