> -----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?
>
> 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.
I think this fix way is more simple than converting to word-access
unconditionally, as this fix way only tweaks the access_mask, while the
word-access way need to tweak both the addr and value.
Thanks,
Zhiqiang
>
> 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/HE1PR0402MB3371C1CDCCD9EA8CC8938FC684CB9%40HE1PR0402MB3371.eurprd04.prod.outlook.com.
