Re: [PATCH 28/34] x86/vm_event: put vm_event_fill_regs under CONFIG_HVM

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 8/21/18 1:45 PM, Wei Liu wrote:
> On Sun, Aug 19, 2018 at 07:41:11PM +0300, Razvan Cojocaru wrote:
>> On 8/17/18 6:12 PM, Wei Liu wrote:
>>> Ideally the HVM specific part of VM event should be moved into hvm/ at
>>> some point, but this will do for now.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>>  xen/arch/x86/vm_event.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>>> index f91aade..b4f6afb 100644
>>> --- a/xen/arch/x86/vm_event.c
>>> +++ b/xen/arch/x86/vm_event.c
>>> @@ -124,6 +124,7 @@ void vm_event_monitor_next_interrupt(struct vcpu *v)
>>>  
>>>  void vm_event_fill_regs(vm_event_request_t *req)
>>>  {
>>> +#if CONFIG_HVM
>>>      const struct cpu_user_regs *regs = guest_cpu_user_regs();
>>>      struct segment_register seg;
>>>      struct hvm_hw_cpu ctxt;
>>> @@ -177,6 +178,7 @@ void vm_event_fill_regs(vm_event_request_t *req)
>>>  
>>>      hvm_get_segment_register(curr, x86_seg_cs, &seg);
>>>      req->data.regs.x86.cs_arbytes = seg.attr;
>>> +#endif
>>>  }
>>
>> Some registers can be obtained here without using HVM-specific code,
>> unless I'm misunderstanding how it works. So I wonder if it wouldn't be
>> better to put only the "struct hvm_hw_cpu ctxt;"-related code under
>> CONFIG_HVM - that way what can be queried via guest_cpu_user_regs()
>> alone won't be lost.
>>
> 
> But there is an assertion for is_hvm_vcpu at the beginning of the
> function, that's how I got the impression that function is supposed to
> be HVM only. Do you want me to remove that ASSERT too?

At the moment it only make sense for HVM, but since that assert this
comment appeared in include/asm-x86/monitor.h:

 73     /*
 74      * At the moment only Intel and AMD HVM domains are supported.
However,
 75      * event delivery could be extended to PV domains.
 76      */

and there has been an (unsuccessful) series to add vm_events to PV domains:

https://lists.xenproject.org/archives/html/xen-devel/2014-07/msg01010.html

That's what I had in mind when I said that not everything there is tied
to HVM. However, we have no current plans to extend mem_access to PV and
the function does indeed only makes sense for HVM domains at the moment,
so unless Tamas or other maintainers have something against your changes
I suppose we don't need to anticipate PV support yet.

So,

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>


Thanks,
Razvan

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