[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.