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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/11/2016 05:33 PM, Jan Beulich wrote:
>>>> On 11.11.16 at 16:16, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> 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 still think it should be bool (and may not even need to be a bitfield
> at this point).

OK, I'll make it a plain bool, and it can be changed later to a bitfield
if need be. This would also clear the spaces around the colon debate. I
assume Tamas won't mind such a simple change.


Thanks,
Razvan

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