Re: [PATCH] xen/vm_event: Clean up control-register-write vm_events

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 05/12/2015 08:54 PM, Andrew Cooper wrote:
> On 12/05/15 17:52, Razvan Cojocaru wrote:
>> On 05/12/2015 06:53 PM, Andrew Cooper wrote:
>>> On 12/05/15 16:50, Andrew Cooper wrote:
>>>> On 12/05/15 16:49, Razvan Cojocaru wrote:
>>>>> On 05/12/2015 06:35 PM, Jan Beulich wrote:
>>>>>>>>> On 12.05.15 at 16:58, <rcojocaru@bitdefender.com> wrote:
>>>>>>> +/* Supported values for the vm_event_write_ctrlreg index. */
>>>>>>> +#define X86_CR0     (1 << 0)
>>>>>>> +#define X86_CR3     (1 << 1)
>>>>>>> +#define X86_CR4     (1 << 2)
>>>>>>> +#define X86_XCR0    (1 << 3)
>>>>>> These names, being put in the public interface, are way too generic.
>>>>> I've copied them from Andrew Cooper's suggestion in the previous thread,
>>>>> hopefully he'll chime in. I'm happy to rename / move them if so desired.
>>>> I intended something like VM_EVENT_ARCH_X86_CR0 etc.
>>> or perhaps slightly more specific to ctrl_reg read/write.
>> VM_EVENT_ARCH_X86_MOV_TO_CR0? Or maybe VM_EVENT_WRITE_ARCH_X86_CR0?
> 
> Are there any other events which could plausibly use the same indicies? 
> Monitoring ctrl_reg reads perhaps ?

I see what you mean, those could be reused for reads and result in less
verbosity in the vm_event.h header.

I'll stick to VM_EVENT_ARCH_X86_CR0 and so on (unless requested
otherwise), I don't have a better naming convention (I guess if we need
to emphasize that something is being done to said control register we
could call them VM_EVENT_ACCESS_ARCH_X86_CR0, but that's more verbose
and IMHO not much better).

>> There's something else that should also be clarified: I've renamed the
>> vm_event.h struct to vm_event_write_ctrlreg, and changed
>> monitor.mov_to_cr* to monitor.write_ctrlreg*, but left
>> xc_monitor_mov_to_cr() in libxc, and mov_to_cr in the domctl part (I
>> thought it fit with mov_to_msr(), etc.). Should I change that to
>> write_ctrlreg as well? On the one hand, mov_to_cr seems to be consistent
>> with the previous interface, and on the other it's not consistent with
>> the name changes the patch has made.
> 
> Any API/ABI changes in this area are fine until 4.6 is released, and a
> consistent interface is much preferred.

Understood. Then I'll rename xc_monitor_mov_to_cr() to
xc_monitor_write_ctrlreg(), since vm_event_write_ctrlreg has been
previously requested. I'll also rename
XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR to
XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG, and VM_EVENT_REASON_MOV_TO_CR to
VM_EVENT_REASON_WRITE_CTRLREG.

I'm reading your reply to mean that I should also rename
xc_monitor_mov_to_msr() to xc_monitor_write_msr(), and do similar things
to the respective vm_event struct, VM_EVENT_REASON_MOV_TO_MSR and so on.
Is that correct?


Thanks,
Razvan