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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 07/06/2017 06:42 PM, Stefano Stabellini wrote:
> On Thu, 6 Jul 2017, Jan Beulich wrote:
> > > > > On 05.07.17 at 22:39, <julien.grall@xxxxxxx> wrote:
> > > On 05/07/2017 19:35, Stefano Stabellini wrote:
> > > > On Tue, 4 Jul 2017, Jan Beulich wrote:
> > > > > 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.
> > There's nothing memaccess-specific in the handler of the operation.
> > Where faults go merely depends on whether a monitor has been
> > registered afaict. Hence ...
> >
> > > 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.
> > ... no matter which way we allow setting the permissions for the
> > purpose here, we'll have to deal with what you describe, except
> > that as per above the question of setting up a ring looks orthogonal
> > to the apparent conflicts here.
> >
> > Considering the intended purpose here (as far as I recall it), was it
> > already taken into consideration to request suitable attributes right
> > at the time the page gets installed into the physmap? Iirc there's no
> > need to actually "play" with the attributes at random times.
> This operation would be done before the guest starts.
>
>
> Let's give a look at the list the changes that would be required to make
> these hypercalls suitable for this task:
>
> 1) remove the dependency on CONFIG_HAS_MEM_ACCESS
> 2) remove the p2m_mem_access_sanity_check check for these two hypercalls
> 3) remove the (!d->vm_event->monitor.ring_page) check for these two hypercalls
> 4) prevent p2m->mem_access_enabled from being set for these two hypercalls
>
> Am I missing anything? After we do this, would they still be useful for
> their original mem_access related purpose?
But how would you handle mem_access on those regions in that case? This 
looks completely incompatible.
The memaccess code has to store the previous permission in order to look 
for the fault. Here you want to modify for good.
Furthermore, memaccess is only here to modify permission. It does not 
handle cacheability... So it looks to me you are trying to re-purposing 
an hypercall that will not fit all our needs in the future.
I think the way forward is to introduce an hypercall which populate/map 
memory with a given set of attributes and permissions.
This would simplify quite a lot the logic (one hypercall instead of 
multiple one) and avoid to worry about attributes changed multiple time 
even before the guest is booting.
Cheers,

--
Julien Grall

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