Re: [PATCH v3 7/7] xen/arm: Sanitize CTR_EL0 and emulate it if needed

[Julien Grall <julien@xxxxxxx>]

On 31/08/2021 14:17, Bertrand Marquis wrote:
> Hi Julien,
Hi Bertrand,

> > On 27 Aug 2021, at 16:05, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi Bertrand,
> >
> > On 25/08/2021 14:18, Bertrand Marquis wrote:
> > > Sanitize CTR_EL0 value between cores.
> > > In most cases different values will taint Xen but if different
> > > i-cache policies are found, we choose the one which will be compatible
> > > between all cores in terms of invalidation/data cache flushing strategy.
> > I understand that all the CPUs in Xen needs to agree on the cache flush 
> > strategy. However...
> >
> > > In this case we need to activate the TID2 bit in HCR to emulate the
> > > TCR_EL0 register for guests. This patch is not activating TID2 bit all
> > > the time to limit the overhead when possible.
> > as we discussed in an earlier version, a vCPU is unlikely (at least in 
> > short/medium) to be able move across pCPU of different type. So the vCPU would 
> > be pinned to a set of pCPUs. IOW, the guest would have to be big.LITTLE aware 
> > and therefore would be able to do its own strategy decision.
> >
> > So I think we should be able to get away from trappings the registers.
> I do agree that we should be able to get away from that in the long term once
> we have cpupools properly set but right now this is the only way to have
> something useable (I will not say right).
> I will work on finding a way to setup properly cpupools (or something else as
> we discussed earlier) but in the short term I think this is the best we can do.
My concern is you are making look like Xen will be able to deal nicely 
with big.LITTLE when in fact there are a lot more potential issue by 
allow a vCPU moving accross pCPU of different type (the errata is one 
example).
> An other solution would be to discard this patch from the serie for now until
> I have worked a proper solution for this case.
>
> Should we discard or merge or do you have an other idea ?
Please correct me if I am wrong, at the moment, it doesn't look like 
this patch will be part of the longer plan. If so, then I think it 
should be parked for now.
This would also have the advantage to avoid spending too much time on 
resolving the emulation issue I mentioned in my previous answer.
No need to resend a new version of this series yet. You can wait until 
the rest of the series get more feedback.
[...]

> If we get interrupted, someone could program CSSELR differently and the next 
> read
> will not be reflecting what the guest actually wants to do
AFAICT, CSSELR is preserved during the context switch of vCPU. So that 
someone would have to be Xen, right?
If so, what you describe would also be an issue even if we didn't trap 
the register. Therefore, if Xen would ever use CSSELR, then that code 
would need to save the value, use the register and then restore the 
value with preemption disabled.
> The code is not preemptible right now so this cannot be an issue but I added the
>   comment more as a warning.
>
> This is not something from the documentation, this is because value written
> in CSSELR is defining what is read from CCSIDR
>
> > > +            WRITE_SYSREG(v->arch.csselr, CSSELR_EL1);
> > > +            set_user_reg(regs, regidx, READ_SYSREG(CCSIDR_EL1));
> > > +        }
Cheers,

--
Julien Grall