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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

On Fri, Dec 24, 2021 at 03:42:42PM +0100, Julien Grall wrote:
> On 20/12/2021 16:41, Oleksii Moisieiev wrote:
> > Hi Julien,
>
> Hello,
>
> > 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://urldefense.com/v3/__https://developer.arm.com/documentation/den0056/latest__;!!GF_29dbcQIUBPA!m9pWoxBEjb8Sd1CoV5cpU8MbmLCjohYQxv2ci9tDvMmZ9oCEitqyydZ3rQWXCM5bxvIn$
> >  [developer[.]arm[.]com].
> > 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.
>
> So, IIUC, the hypervisor will not control what is written in the shared
> memory. It will only control the SMC parameters. Is my understanding
> correct?
>

For scmi_smc it will not. But potentially it can make some changes, or
convert to the different protocol.

> >
> > 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.
>
> I think you misunderstood my comment. Memory can be mapped with various type
> (e.g. Device, Memory) and attribute (cacheable, non-cacheable...). What will
> the firmware expect? What will the guest OS usually?
>
> The reason I am asking is the attributes have to matched to avoid any
> coherency issues. At the moment, if you use XEN_DOMCTL_memory_mapping, Xen
> will configure the stage-2 to use Device nGnRnE. As the result, the result
> memory access will be Device nGnRnE which is very strict.
>

Let me share with you the configuration example:
scmi expects memory to be configured in the device-tree:

cpu_scp_shm: scp-shmem@0xXXXXXXX {
        compatible = "arm,scmi-shmem";
        reg = <0x0 0xXXXXXX 0x0 0x1000>;
};

where XXXXXX address I allocate in alloc_magic_pages function.
Then I get paddr of the scmi channel for this domain and use
XEN_DOMCTL_memory_mapping to map scmi channel address to gfn.

Hope I wass able to answer your question.

Best regards,
Oleksii.