Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 08/04/2022 23:18, Stefano Stabellini wrote:
> On Tue, 29 Mar 2022, Penny Zheng wrote:
> > Right now, the memory attribute of static shared memory is RW as default,
> > What if we add memory attribute setting in device tree configuration, sometimes,
> > Users want to specify that borrower domain only has RO right, hmm, then the
> > Initialization for p2mt could be either p2m_ram_rw or p2m_ram_ro?
> > In such case, we could add another parameter in guest_physmap_add_shm to
> > show the p2m type...
> > Hope I understand what you suggested here.
> Yes, I think I understand. I think your suggestion is OK too. However,
> your suggestion is much more work than mine: I was only suggesting a
> small improvement limited to guest_physmap_add_shm and
> get_pages_from_gfn. Your suggestion involves a device tree change that
> would have a larger impact on the patch series. So if you are up for it,
> I am happy to review it. I am also fine just to have a small improvement
> on guest_physmap_add_shm/get_pages_from_gfn.
AFAIU, your proposal would mean that the behavior for 
get_pages_from_gfn() and get_page_from_gfn() will differ. This is not great.
I don't think we can easily change the behavior of get_page_from_gfn() 
because some callers can accept multiple types.
Furthermore, while today we only check p2m_ram_rw. It might be possible 
that we would want to check different type. For instance, if we want to 
use read-only mapping, it would be fine to accept p2m_ram_ro and p2m_ram_rw.
So overall, I am not in favor of initializing p2mt and let 
get_pages_from_gfn() to check the type.
Cheers,

--
Julien Grall