Re: [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole

From: Tamas K Lengyel <tamas@tklengyel.com>
To: Corneliu ZUZU <czuzu@bitdefender.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Xen-devel <xen-devel@lists.xen.org>,
	Paul Durrant <paul.durrant@citrix.com>,
	Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH 14/16] x86/monitor: clarify separation between monitor subsys and vm-event as a whole
Date: Wed, 13 Jul 2016 12:56:27 -0600	[thread overview]
Message-ID: <CABfawhnMZit6mQUbLT-OHcq2hws+XQAaGGV4kNnpopZPc6bTDg@mail.gmail.com> (raw)
In-Reply-To: <bc35430f-ff04-d018-0743-d08564dc93c6@bitdefender.com>

On Tue, Jul 12, 2016 at 10:26 PM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
> On 7/9/2016 9:57 PM, Corneliu ZUZU wrote:
>>
>> On 7/9/2016 9:26 PM, Tamas K Lengyel wrote:
>>>>
>>>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>>>> index ae1dcb4..7663da2 100644
>>>> --- a/xen/include/asm-x86/domain.h
>>>> +++ b/xen/include/asm-x86/domain.h
>>>> @@ -9,6 +9,7 @@
>>>>   #include <asm/e820.h>
>>>>   #include <asm/mce.h>
>>>>   #include <public/vcpu.h>
>>>> +#include <public/vm_event.h>
>>>>   #include <public/hvm/hvm_info_table.h>
>>>>
>>>>   #define has_32bit_shinfo(d) ((d)->arch.has_32bit_shinfo)
>>>> @@ -503,6 +504,20 @@ typedef enum __packed {
>>>>       SMAP_CHECK_DISABLED,        /* disable the check */
>>>>   } smap_check_policy_t;
>>>>
>>>> +/*
>>>> + * Should we emulate the next matching instruction on VCPU resume
>>>> + * after a vm_event?
>>>> + */
>>>> +struct arch_vm_event_monitor {
>>>
>>> This should be named struct arch_vcpu_monitor.
>>
>>
>> Good idea.
>>
>>>
>>>> +    uint32_t emulate_flags;
>>>> +    struct vm_event_emul_read_data emul_read_data;
>>>
>>> This should probably get renamed as well at some point to struct
>>> monitor_emul_read_data.
>>
>>
>> Ack.
>>
>>>> +    struct monitor_write_data write_data;
>>>> +};
>>>> +
>>>> +struct arch_vm_event {
>>>> +    struct arch_vm_event_monitor *monitor;
>>>> +};
>>>
>>> IMHO there is not much point in defining struct arch_vm_event this
>>> way, we could just as well store the pointer to the arch_monitor
>>> directly in arch_vcpu as we do right now.
>>>
>>
>> I stated the reason for that in the commit message (see 3rd '*'), Jan
>> insists it would be preferable to occupy just one pointer in arch_vcpu
>> (which would still hold if we do as you suggest but I was wondering how
>> probable would it be in the future for one of the paging/share vm-event
>> subsystems to need per-vCPU resources). But personally, yeah, I would have
>> preferred what you suggest as well..
>>
>> Zuzu.
>
>
> Tamas, any update on this? Do you think we should think now about per-vCPU
> resources being occupied in the future by paging/share or leave that for
> later and do what you suggested?

It's highly unlikely that either paging or sharing will have any use
of this struct in the near future provided but subsystems are pretty
much orphaned at this point. If we ever going to need it it's an easy
enough adjustment but for now it's not needed.

Tamas

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