Re: [PATCH v1 3/5] xen/arm: platforms: Add NXP S32CC platform code

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 11 Sep 2024, Andrei Cherechesu wrote:
> Hi Julien, Stefano,
> On 11/09/2024 08:19, Stefano Stabellini wrote:
> 
> On Tue, 10 Sep 2024, Julien Grall wrote:
> 
> Hi,
> 
> On 10/09/2024 15:34, Andrei Cherechesu (OSS) wrote:
> 
> From: Andrei Cherechesu <andrei.cherechesu@xxxxxxx>
> 
> Added support for NXP S32CC platforms (S32G2, S32G3, S32R45),
> which need SCMI over SMC calls forwarded to the firmware
> running at EL3 (TF-A). Linux relies on the SCMI Platform
> for system services such as clocking, reset, etc.
> 
> Is it SMCI as in the Arm spec? If so, this should not be platform specific.
> 
> Yes, it is SCMI as defined by Arm. I agree it shouldn't be platform specific.
> 
> 
> Furthermore, I was under the impression that we can't blindly forward
> everything from a domain to the firmware. While it might be okayish for dom0,
> you also seem to give access to all the domains on the system is it intended?
> 
> In our case, only Dom0 has access to the SCMI mailbox. Hence, the other 
> unprivileged domains are not aware of SCMI and do not make any SCMI
> requests to FW.
> 
> 
> Anyway, there is a series on the ML to add a mediator for SCMI in Xen (see
> [1]). I think it would be preferable to focus on getting it merged as it would
>             benefit everyone and increase the security posture (we could 
> restrict access).
> 
> I also asked a few months ago on the ML in a virtio related thread if there 
> are any updates regarding
> SCMI awareness for Xen and guests, then Stefano CCed Bertrand, but he did not 
> comment [0].
> I'm curious why the SCMI mediator patch series did not progress.
> [0] 
> https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2401111627360.3675@ubuntu-linux-20-04-desktop/
> 
> 
> Hi Andrei, Julien,
> 
> SCMI is very flexible and can be configured in a number of ways. In
> general, Julien has a point that typically forwarding to firmware all
> SCMI requests from Xen domains is not the desired behavior.
> 
> An an example, imagine the case where device1 is assigned to domain1 and
> device2 is assigned to domain2. Now imagine that they both share a
> clock. Domain1 and domain2 could fight over the clock frequency settings
> using SCMI to change it, without being aware of each other activities.
> It is likely that the system would malfunction.
> 
> I completely agree and we are aware of the possible resource contention. 
> Another (simpler?) scenario where access control is needed,
> besides the one you described, is when Domain1 would directly try to perform 
> some requests for some resources that affect Device2 (owned by
> Domain2). If Domain1 knows the clock IDs used by Device2, for example, 
> without any access control it could perform a SCMI clock request
> affecting Device2's clocks, which should be out of his control.
> 
> 
> If this kind of situations can happen on NXP S32CC platforms, then this
> patch might not be a good idea. As Julien suggested, you might want to
> have a look at Oleksii's approach. We could probably allow Dom0 to make
> all SCMI calls. If you think that is OK, you need to add a
> (is_hardware_domain(d)) check.
> 
> On the other hand, if your SCMI server implementation has a way to
> prevent possible harmful activities from happening, or maybe all clocks
> are fixed-clocks so there are actually no SCMI operations to control the
> clocks, then it could be possible that this patch might be fine. I admit
> it is unlikely because there is a number of ways SCMI could be used by
> one domain to hurt another domain.
> 
> Can you please give us a brief overview on how SCMI is expected to work
> on NXP S32CC?
> 
> Well, we normally rely on most SCMI protocols to access system-level 
> resources from agents: Base, Power Domain, System Power, Performance
> Domain, Clock, Reset Domain. Linux jumps to EL3 via SMC carrying an SCMI 
> message, and FW running at EL3 decides how to handle it.
> Basically, Linux cannot directly control most system-level resources.
> 
> With Xen, we currently don't allow unprivileged Domains to do SCMI requests. 
> The SMCs are of course trapped at EL2 and that's why we
> enabled forwarding to EL3 without any access control, knowing it shouldn't 
> break anything, and to let everything function as normal. In
> some passthrough scenarios the unprivileged domains rely on settings already 
> made by firmware (for clocks, pins, etc) that their assigned
> devices require, and in DT we replace them with e.g. fixed-clock for clocks.
> 
> An "is_hardware_domain(d)" check should be enough for the moment to harden 
> the code, but I agree that this should not be something
> platform-specific in the future, and the handling must be done in a generic 
> way.
> So I would proceed with this approach for this patch series, if that's ok for 
> you, and I will also take a look at Oleksii's approach.

I know how hard is to solve this properly and it might take months. I
wouldn't want to stop Xen from working on NXP S32CC until a proper SCMI
solution is found. For now, I would say to add the
"is_hardware_domain(d)" check.

Julien, do you also agree?