Re: [Xen-devel] [RFC] DOMCTL_memattrs_op : a new DOMCTL to play with stage-2 page attributes

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 05/07/2017 19:35, Stefano Stabellini wrote:
> On Tue, 4 Jul 2017, Jan Beulich wrote:
> > > > > On 03.07.17 at 19:58, <sstabellini@xxxxxxxxxx> wrote:
> > > On Mon, 3 Jul 2017, Zhongze Liu wrote:
> > > > 2017-07-03 22:25 GMT+08:00 Jan Beulich <JBeulich@xxxxxxxx>:
> > > > > > > > On 30.06.17 at 22:15, <blackskygg@xxxxxxxxx> wrote:
> > > > > > /* flags for XEN_DOMCTL_MEMATTRS_OP_SET_PERMISSIONS */
> > > > > > #define XEN_DOMCTL_MEMATTRS_ACCESS_N         0x00U
> > > > > > #define XEN_DOMCTL_MEMATTRS_ACCESS_R         (0x01U<<0)
> > > > > > #define XEN_DOMCTL_MEMATTRS_ACCESS_W         (0x01U<<1)
> > > > > > #define XEN_DOMCTL_MEMATTRS_ACCESS_X         (0x01U<<2)
> > > > > > #define XEN_DOMCTL_MEMATTRS_ACCESS_RW        \
> > > > > > (XEN_DOMCTL_MEMATTRS_ACCESS_R|XEN_DOMCTL_MEMATTRS_ACCESS_W)
> > > > > > #define XEN_DOMCTL_MEMATTRS_ACCESS_RX        \
> > > > > > (XEN_DOMCTL_MEMATTRS_ACCESS_R|XEN_DOMCTL_MEMATTRS_ACCESS_X)
> > > > > > #define XEN_DOMCTL_MEMATTRS_ACCESS_WX        \
> > > > > > (XEN_DOMCTL_MEMATTRS_ACCESS_W|XEN_DOMCTL_MEMATTRS_ACCESS_X)
> > > > > > #define XEN_DOMCTL_MEMATTRS_ACCESS_RWX        \
> > > > > > (XEN_DOMCTL_MEMATTRS_ACCESS_RW|XEN_DOMCTL_MEMATTRS_ACCESS_X)
> > > > > ... with this basically duplicating
> > > > > XENMEM_access_op_{set,get}_access I now wonder whether
> > > > > we don't already have all you need (apart from an ARM variant of
> > > > > DOMCTL_pin_mem_cacheattr).
> > > > In fact, there isn't much description on the usage of this
> > > > interface, so I turned to the implementation in
> > > > xen/common/mem_access.c, where I see this
> > > > interface invoking  p2m_set_mem_acess, which further invokes
> > > > set_mem_acess and finally
> > > > p2m->set_entry(), so I guess this might be the right interface to use.
> > > > To confirm the guess, I turned to Stabellini for help, and he told me
> > > > that XENMEM_access_op
> > > > is "for getting very detail info on what the guest is accessing", and
> > > > might not be suitable
> > > > for this scenario, so I just gave up using it, and that's why I have this RFC.
> > > > I'll re-confirm this with Stabellini.
> > > I thought that those two hypercalls were meant to be used for mem_access
> > > and vm_event operations, as in xen/arch/arm/mem_access.c and
> > > xen/arch/x86/mm/mem_access.c. The only caller is
> > > tools/tests/xen-access/xen-access.c. They are enabled separatly as part
> > > of the mem_access interface: their build is conditional to
> > > CONFIG_HAS_MEM_ACCESS. Unless we want to move them from XENMEM_access_*
> > > to DOMCTL_* operations, I don't think they could be used?
> > For one, we could remove the CONFIG_HAS_MEM_ACCESS around
> > them if a broader use is planned. And in general we should try to
> > avoid having two ways of doing the same thing, unless backwards
> > compatibility makes this a requirement. Hence if a new, better way
> > is to be introduced, the old one should at once go away. Finally, I'm
> > still unconvinced a new DOMCTL_* is better here than a (tool stack
> > only) XENMEM_*, but I agree the boundary between when to use
> > what is at best fuzzy.
> Do we maintain ABI compatibility for XENMEM_* hypercalls? I think we do,
> don't we? Also, XENMEM_* hypercalls are usually available to both
> guests and toolstack, right?
>
> We don't want two ways of doing the same thing, but at the same time
> XENMEM_ hypercalls are very different from DOMCTLs, which don't come
> with any ABI compatibility guarantees and are only available to the
> toolstack. And these two specific XENMEM hypercalls even depend on
> CONFIG_HAS_MEM_ACCESS.
>
> I am not completely sure about what the best way forward would be. I am
> OK with anything that is clear and maintainable. I would probably still
> go with updating DOMCTL_pin_mem_cacheattr into something that can handle
> both ARM and permissions, but I am also OK with making changes to
> XENMEM_access_op_{set,get}_access so that they become an option for this
> use case.
I am struggling to understand how you could make memaccess_op_*_access 
supporting 2 distinct use cases. They are currently used to instrospect 
memory by restricting the permission. All the faults will be forwarded 
to a monitor.
Here you suggest to extend them to restrict permission. But we want to 
be able to support introspection on that share page (I don't see why 
not) and we don't want to have to set-up a VM-event ring just for 
restrict the page.
Moreover, you would have to store the access permission for the 
time-being... whilst here you just modify the permission of the page for 
good.
Am I missing something here?

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel