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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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).

Jan


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