Re: [RFC v1 3/5] xen/arm: introduce SCMI-SMC mediator driver

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

Hi Julien,

On Fri, Dec 17, 2021 at 04:38:31PM +0000, Julien Grall wrote:
> 
> 
> On 17/12/2021 13:58, Oleksii Moisieiev wrote:
> > Hi Julien,
> 
> Hi,
> 
> > On Fri, Dec 17, 2021 at 01:37:35PM +0000, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 17/12/2021 13:23, Oleksii Moisieiev wrote:
> > > > > > +static int map_memory_to_domain(struct domain *d, uint64_t addr, 
> > > > > > uint64_t len)
> > > > > > +{
> > > > > > +    return iomem_permit_access(d, paddr_to_pfn(addr),
> > > > > > +                paddr_to_pfn(PAGE_ALIGN(addr + len -1)));
> > > > > > +}
> > > > > > +
> > > > > > +static int unmap_memory_from_domain(struct domain *d, uint64_t 
> > > > > > addr,
> > > > > > +                                     uint64_t len)
> > > > > > +{
> > > > > > +    return iomem_deny_access(d, paddr_to_pfn(addr),
> > > > > > +                paddr_to_pfn(PAGE_ALIGN(addr + len -1)));
> > > > > > +}
> > > > > 
> > > > > I wonder, why we need an extra level of indirection here. And if this 
> > > > > is
> > > > > really needed, I wonder why map(unmap)_memory* name was chosen, as 
> > > > > there is
> > > > > no memory mapping/unmapping really happens here.
> > > > > 
> > > > 
> > > > I've added extra indirection to hide math like
> > > > paddr_to_pfn(PAGE_ALIGN(addr + len -1)
> > > > so you don't have to math in each call. unmap_memory_from_domain called
> > > > from 2 places, so I moved both calls to separate function.
> > > > Although, I agree that map/unmap is not perfect name. I consider
> > > > renaming it to mem_permit_acces and mam_deny_access.
> > > 
> > > I haven't looked at the rest of the series. But this discussion caught my
> > > eye. This code implies that the address is page-aligned but the length 
> > > not.
> > > Is that intended?
> > > 
> > > That said, if you give permission to the domain on a full page then it 
> > > means
> > > it may be able to access address it should not. Can you explain why this 
> > > is
> > > fine?
> > > 
> > 
> > The idea was that xen receives some memory from the dt_node linux,scmi_mem,
> > then we split memory between the agents, so each agent get 1 page (we
> > allocate 0x10 pages right now).
> 
> Thanks for the clarification. Does this imply the guest will be able to
> write message directly to the firmware?

We used DEN0056C Specification as base. Available on: 
https://developer.arm.com/documentation/den0056/latest.
SCMI transport is described in Section 5.1. We implemented Shared Memory 
transport.
Firmware has N pages of the shared memory, used to communicate with Agents.
It allocates N agents and assign a page for each agent, such as:
-------------------------------------
| Agent H | Agent 1 | Agent 2 | ... |
-------------------------------------
Agent H is the privilleged Hypervisor agent, which is used to do the base 
commands,
such as getting Agent list, set\unset permissions etc.
Hypervisor assign agent to the guest and maps page, related to the agent to the 
Guest.
So the Guest, which is Agent 1 will get an access to Agent 1 page.

Guest places SCMI message to Agent 1 memory, then sends SMC message.
Hypervisor process SMC request, add agent id to the message parameters and 
redirects it to the Firmware.
Based on the agent_id Firmware knows which page it should read. 
Then after permission check ( if the resetId/clockID/powerID etc from message
is assigned to agent_id ) it does changes to the HW and places response to Agent
shared memory and marks channel as FREE ( by setting free bit in shared memory 
).
Once channel is marked as free - Guest read response from the shared memory.

Non-virtualized systems will work as well. OS should send SMC directly to the 
Firmware. 

> 
> If so, this brings a few more questions:
>   1) What will the guest write in it? Can it contains addresses?
Guest can write scmi request to the shared memory, which include the following 
data: 
1) protocol_id - which protocol is requested, such as clock, power, reset etc
2) message_id - action that should be done to HW, such as do_reset or get_clock
3) message data - which includes reset_id/clock_id/power_id etc. that should be 
changed.
Reset IDs and Clock IDs are assigned in Firmware. Guest receives ID, 
corresponding to the device from the device-tree. 
dt_node as an example: 
&avb {  
        scmi_devid = <0>;
        clocks = <&scmi_clock 0>;
        power-domains = <&scmi_power 0>;
        resets = <&scmi_reset 0>;
};

>   2) What are the expected memory attribute for the regions?

xen uses iommu_permit_access to pass agent page to the guest. So guest can 
access the page directly.

>   3) What's the threat model for the firmware? Will it verify every request?

Firmware reads data from agent page, then makes check if clockid/resetid/powerid
etc is assigned to agent.
During building guest, Xen sends permission request to the firmware for each 
device,
which is passed-through to the guest. So for avb from previous example the
device_id 0 permission request will be sent. Based on the device_id firmware 
will
set permission for clockid 0, resetid 0 and powerid 0. 

Best regards,
Oleksii.