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 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).
But this is a good point, I think we should check length to be aligned
before making any permission changes.
I will add it in v2. 
Thank you for remark.

Oleksii.