On Wed, 13 Nov 2019, 10:55 André Przywara, <andre.przyw...@arm.com> wrote:
> On 13/11/2019 01:08, Julien Grall wrote: > > Hi, > > > On Tue, 12 Nov 2019, 04:01 Stefano Stabellini, <sstabell...@kernel.org > > <mailto:sstabell...@kernel.org>> wrote: > > > > On Sat, 9 Nov 2019, Julien Grall wrote: > > > On Sat, 9 Nov 2019, 04:27 Stefano Stabellini, > > <sstabell...@kernel.org <mailto:sstabell...@kernel.org>> wrote: > > > On Thu, 7 Nov 2019, Peng Fan wrote: > > > > The end should be GICD_ISACTIVERN not GICD_ISACTIVER. > > > > > > > > Signed-off-by: Peng Fan <peng....@nxp.com > > <mailto:peng....@nxp.com>> > > > > > > Reviewed-by: Stefano Stabellini <sstabell...@kernel.org > > <mailto:sstabell...@kernel.org>> > > > > > > > > > To be honest, I am not sure the code is correct. A read to those > > registers should tell you the list of interrupts active. As we always > > > return 0, this will not return the correct state of the GIC. > > > > > > I know that returning the list of actives interrupts is > > complicated with the old vGIC, but I don't think silently ignoring > > it is a good > > > idea. > > > The question here is why the guest accessed those registers? What > > is it trying to figure out? > > > > We are not going to solve the general problem at this stage. At the > > moment the code: > > > > - ignore the first register only > > - print an error and return an IO_ABORT error for the other regs > > > > For the inconsistency alone the second option is undesirable. Also it > > doesn't match the write implementation, which does the same thing for > > all the GICD_ISACTIVER* regs instead of having a special treatment > for > > the first one only. It looks like a typo in the original patch to me. > > > > The proposed patch switches the behavior to: > > > > - silently ignore all the GICD_ISACTIVER* regs (as proposed) > > > > > > is an improvement. > > > > > > Peng mentioned that Linux is accessing it, so the worst thing we can do > > is lying to the guest (as you suggest here). I would definitely not call > > that an improvement. > > The ISACTIVER range is wrong in the description, it covers only one > register, not multiple. This is obviously a typo, since it's correct in > both GICv2 and in the high level switch/case in GICv3. Reading from > outside of any range will inject an abort into the guest, which runs in > kernel space. This will probably result in a guest crash. I would > consider not crashing an improvement. > It is not. Neither the current approach to silently doing it. > About "lying" to the guest: Typically an IRQ is just active for a very > short time, so 0 is a very good answer, actually. So why does Linux is checking it? What will happen if there were actually an active interrupt but don't report it? The old VGIC in KVM > did exactly the same: > vgic_reg_access(mmio, NULL, offset, > ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED); > > The proper solution would be: > 1) Track the state of the active bit when we can observe it, so when the > guest exits with an active IRQ. The new VGIC does that. > 2) Kick out all VCPUs that have IRQs in that given rank, and sample the > active bit from the LRs. Sounds pretty horrible, and chances are very > high you will get all 0s there. > > So if I compare "fix those two typos and preserve the state that the Xen > VGIC has been in for years" to "create a lot of racy code for a rare > corner case", the first one surely wins. > That doesn't mean we should never try it, but surely this fix needs to > go in meanwhile. > I don't believe this patch to go in is the correct solution not from a technical PoV but to get things properly fixed. > > In the current state, it is a Nack. If there were a warning, then I > > would be more inclined to see this patch going through. > > Do you mean a warning that we are about to lie to the guest? That sounds > pretty useless, since nobody can do anything about it. Plus we have > already those warnings on writing to these registers, and this always > confuses people and triggered pointless bug reports. > Well, the warning has the benefits to annoy people. If we do it silently, then we don't encourage to fix it. > I think the old VGIC has bigger problems ;-) > I agree, but nobody seems to be willing to fix it... My only leverage here is pushing for a warning to annoy the user. So I maintain my request for a warning. Cheers, > Cheers, > Andre >
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
