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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/11/2016 01:09 PM, Jan Beulich wrote:
>>>> On 11.11.16 at 11:32, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 11/11/2016 12:26 PM, Jan Beulich wrote:
>>>>>> On 11.11.16 at 11:15, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>> On 11/11/2016 12:02 PM, Jan Beulich wrote:
>>>>>>>>>>>>> On 11.11.16 at 09:06, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>>>>>> --- 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 {"?
>>> No. I mean the ones around the colon.
>>
>> I'm sorry, I don't follow. The examples I've pasted in the previous
>> reply make similar use of the colon:
>>
>> 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;
>>
>> and
>>
>> 130     /* Monitor options */
>> 131     struct {
>> 132         uint8_t privileged_call_enabled : 1;
>> 133     } monitor;
>>
>> I take that you would prefer this?
>>
>> unsigned int next_interrupt_enabled:1;
>>
>> I have nothing against the change, I'm just confused about what the
>> proper and consistent way of writing that is.
> 
> grep-ing the include/ subtree I see that there are (apart from the
> quoted ones) examples of all kinds, so I guess it can as well stay as
> is, even if I personally consider the blanks stray here.

Alright, thanks! So since Tamas has given his ack, I guess all that's
required now is to const-ify struct vmcb_struct *vmcb in
svm_get_pending_event() (and also I now see in the examples above that a
uint8_t is probably better suited than an unsigned int for
next_interrupt_enabled, so that it will take less space in struct arch_vcpu.

I'll submit V4 shortly with these two changes.


Thanks,
Razvan

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