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

[Julien Grall <julien@xxxxxxx>]

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?
Cheers,

--
Julien Grall