On Fri, Mar 25, 2022, 5:04 AM Jan Beulich <[email protected]> wrote: > On 24.03.2022 18:02, Tamas K Lengyel wrote: > > On Thu, Mar 24, 2022 at 12:44 PM Roger Pau Monné <[email protected]> > wrote: > >> > >> On Thu, Mar 24, 2022 at 12:22:49PM -0400, Tamas K Lengyel wrote: > >>> On Thu, Mar 24, 2022 at 12:04 PM Roger Pau Monné <[email protected]> > wrote: > >>>> > >>>> On Thu, Mar 24, 2022 at 11:52:38AM -0400, Tamas K Lengyel wrote: > >>>>> On Thu, Mar 24, 2022 at 11:46 AM Roger Pau Monné < > [email protected]> wrote: > >>>>>> > >>>>>> On Tue, Mar 22, 2022 at 01:41:39PM -0400, Tamas K Lengyel wrote: > >>>>>>> diff --git a/xen/include/public/memory.h > b/xen/include/public/memory.h > >>>>>>> index 208d8dcbd9..30ce23c5a7 100644 > >>>>>>> --- a/xen/include/public/memory.h > >>>>>>> +++ b/xen/include/public/memory.h > >>>>>>> @@ -541,12 +541,14 @@ struct xen_mem_sharing_op { > >>>>>>> uint32_t gref; /* IN: gref to debug */ > >>>>>>> } u; > >>>>>>> } debug; > >>>>>>> - struct mem_sharing_op_fork { /* OP_FORK */ > >>>>>>> + struct mem_sharing_op_fork { /* OP_FORK/_RESET */ > >>>>>>> domid_t parent_domain; /* IN: parent's domain > id */ > >>>>>>> /* These flags only makes sense for short-lived forks */ > >>>>>>> #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0) > >>>>>>> #define XENMEM_FORK_BLOCK_INTERRUPTS (1u << 1) > >>>>>>> #define XENMEM_FORK_SKIP_SPECIAL_PAGES (1u << 2) > >>>>>>> +#define XENMEM_FORK_RESET_STATE (1u << 3) > >>>>>>> +#define XENMEM_FORK_RESET_MEMORY (1u << 4) > >>>>>>> uint16_t flags; /* IN: optional > settings */ > >>>>>>> uint32_t pad; /* Must be set to 0 */ > >>>>>>> } fork; > >>>>>>> diff --git a/xen/include/public/vm_event.h > b/xen/include/public/vm_event.h > >>>>>>> index bb003d21d0..81c2ee28cc 100644 > >>>>>>> --- a/xen/include/public/vm_event.h > >>>>>>> +++ b/xen/include/public/vm_event.h > >>>>>>> @@ -127,6 +127,14 @@ > >>>>>>> * Reset the vmtrace buffer (if vmtrace is enabled) > >>>>>>> */ > >>>>>>> #define VM_EVENT_FLAG_RESET_VMTRACE (1 << 13) > >>>>>>> +/* > >>>>>>> + * Reset the VM state (if VM is fork) > >>>>>>> + */ > >>>>>>> +#define VM_EVENT_FLAG_RESET_FORK_STATE (1 << 14) > >>>>>>> +/* > >>>>>>> + * Remove unshared entried from physmap (if VM is fork) > >>>>>>> + */ > >>>>>>> +#define VM_EVENT_FLAG_RESET_FORK_MEMORY (1 << 15) > >>>>>> > >>>>>> I'm confused about why two different interfaces are added to do this > >>>>>> kind of selective resets, one to vm_event and one to xenmem_fork? > >>>>>> > >>>>>> I thin k the natural place for the option to live would be > >>>>>> XENMEM_FORK? > >>>>> > >>>>> Yes, that's the natural place for it. But we are adding it to both > for > >>>>> a reason. In our use-case the reset operation will happen after a > >>>>> vm_event is received to which we already must send a reply. Setting > >>>>> the flag on the vm_event reply saves us having to issue an extra > memop > >>>>> hypercall afterwards. > >>>> > >>>> Can you do a multicall and batch both operations in a single > >>>> hypercall? > >>>> > >>>> That would seem more natural than adding duplicated interfaces. > >>> > >>> Not in a straight forward way, no. There is no exposed API in libxc to > >>> do a multicall. Even if that was an option it is still easier for me > >>> to just flip a bit in the response field than having to construct a > >>> whole standalone hypercall structure to be sent as part of a > >>> multicall. > >> > >> Right, I can see it being easier, but it seems like a bad choice from > >> an interface PoV. You are the maintainer of both subsystems, but it > >> would seem to me it's in your best interest to try to keep the > >> interfaces separated and clean. > >> > >> Would it be possible for the reset XENMEM_FORK op to have the side > >> effect of performing what you would instead do with the vm_event > >> hypercall? > > > > Yes, the event response is really just an event channel signal to Xen, > > so the memop hypercall could similarly encode the "now check the > > vm_event response" as an optional field. But why is that any better > > than the current event channel route processing the vm_response > > encoding the "now do these ops on the fork"? > > Well, as Roger said: Less duplication in the interface. >
No, you would just duplicate something else instead, ie. the event channel hypercall. > > We already have a bunch of different operations you can encode in the > > vm_event response field, so it reduces the complexity on the toolstack > > side since I don't have to switch around which hypercall I need to > > issue depending on what extra ops I want to put into a single > > hypercall. > > The two goals need to be weighed against one another; for the moment > I think I'm with Roger aiming at a clean interface. > It may look like that from the Xen side but from the toolstack side this is actually the cleanest way to achieve what we need. The vm_event interfaces are already strongly integrated with both the mem_sharing and mem_paging subsystems so nothing is gained by now for no reason trying to keep them separate. So I strongly disagree with this suggestion and I'm going to keep it as-is. I appreciate the feedback nevertheless. Tamas >
