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.
