Re: [Xen-devel] [PATCH V3] x86/vm_event: Added support for VM_EVENT_REASON_INTERRUPT

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/11/2016 12:02 PM, Jan Beulich wrote:
>>>> On 11.11.16 at 09:06, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2220,6 +2220,20 @@ static void svm_invlpg(struct vcpu *v, unsigned long 
>> vaddr)
>>      svm_asid_g_invlpg(v, vaddr);
>>  }
>>  
>> +static bool svm_get_pending_event(struct vcpu *v, struct hvm_trap *info)
>> +{
>> +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> 
> const please.

Will do.

>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -134,6 +134,11 @@ void vm_event_set_registers(struct vcpu *v, 
>> vm_event_response_t *rsp)
>>      v->arch.user_regs.eip = rsp->data.regs.x86.rip;
>>  }
>>  
>> +void vm_event_monitor_next_interrupt(struct vcpu *v)
>> +{
>> +    v->arch.monitor.next_interrupt_enabled = 1;
> 
> true?

This is no longer bool, so I can't assign true here.

>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -576,6 +576,10 @@ struct arch_vcpu
>>      XEN_GUEST_HANDLE(vcpu_time_info_t) time_info_guest;
>>  
>>      struct arch_vm_event *vm_event;
>> +
>> +    struct {
>> +        unsigned int next_interrupt_enabled : 1;
> 
> bool? Stray spaces. And then (sorry for thinking of this only now) - is
> this really usefully an arch-specific flag? I guess there's nothing
> precluding this from also being implemented on ARM eventually?

Stray spaces? Do you mean the newline between "struct arch_vm_event
*vm_event;" and "struct {"?

I'd prefer to leave this as a bitfield for consistency. That is how it
also works in struct arch_domain from xen/include/asm-x86/domain.h:

399     /* Arch-specific monitor options */
400     struct {
401         unsigned int write_ctrlreg_enabled       : 4;
402         unsigned int write_ctrlreg_sync          : 4;
403         unsigned int write_ctrlreg_onchangeonly  : 4;
404         unsigned int singlestep_enabled          : 1;
405         unsigned int software_breakpoint_enabled : 1;
406         unsigned int debug_exception_enabled     : 1;
407         unsigned int debug_exception_sync        : 1;
408         unsigned int cpuid_enabled               : 1;
409         struct monitor_msr_bitmap *msr_bitmap;
410     } monitor;

Which leads to your next question: nothing precludes this from also
being implemented on ARM at some point, however the convention so far
has been to have a "monitor" for x86 with all the supported options, and
one for ARM:

130     /* Monitor options */
131     struct {
132         uint8_t privileged_call_enabled : 1;
133     } monitor;

So if and when this does get implemented for ARM, we would expect a
struct monitor to pop up in struct arch_vcpu in
xen/include/asm-arm/domain.h as well.

I'll submit a V4 with the const-ified VMX structure and the newline
removed from domain.h.


Thanks,
Razvan


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