Re: [Xen-devel] [PATCH] arm/monitor vm-events: Implement guest-request support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.02.16 at 17:25, <czuzu@xxxxxxxxxxxxxxx> wrote:
> On 2/19/2016 4:26 PM, Jan Beulich wrote:
>>>>> On 18.02.16 at 20:35, <czuzu@xxxxxxxxxxxxxxx> wrote:
>>> ---
>>>   MAINTAINERS                     |   1 +
>>>   xen/arch/arm/hvm.c              |   8 +++
>>>   xen/arch/x86/hvm/event.c        | 116 
>>> ++++++----------------------------------
>>>   xen/arch/x86/hvm/hvm.c          |   1 +
>>>   xen/arch/x86/monitor.c          |  14 -----
>>>   xen/arch/x86/vm_event.c         |   1 +
>>>   xen/common/Makefile             |   2 +-
>>>   xen/common/hvm/Makefile         |   3 +-
>>>   xen/common/hvm/event.c          |  96 +++++++++++++++++++++++++++++++++
>> So here you _again_ try to introduce something HVM-ish for ARM.
>> Why? Why can't this code live in common/vm_event.c?
> 
> Are you referring to "xen/arch/arm/hvm.c"? If so, I did not introduce 
> that file, it was already there, all I did is add handling of 
> HVMOP_guest_request_vm_event to ARM-side's already existing do_hvm_op :).

No, I'm referring to your initial attempt to create arch/arm/hvm/...

> On the "HVM-ish" note, is there some incompatibility between ARM and the 
> concept of HVM?

ARM guests are neither PV nor HVM right now, but somewhere in
the middle (PVHv2 may come closest).

>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> @@ -376,17 +376,15 @@ struct arch_domain
>>>       unsigned long *pirq_eoi_map;
>>>       unsigned long pirq_eoi_map_mfn;
>>>   
>>> -    /* Monitor options */
>>> +    /* Arch-specific monitor options */
>>>       struct {
>>> -        unsigned int write_ctrlreg_enabled       : 4;
>>> -        unsigned int write_ctrlreg_sync          : 4;
>>> -        unsigned int write_ctrlreg_onchangeonly  : 4;
>>> -        unsigned int mov_to_msr_enabled          : 1;
>>> -        unsigned int mov_to_msr_extended         : 1;
>>> -        unsigned int singlestep_enabled          : 1;
>>> -        unsigned int software_breakpoint_enabled : 1;
>>> -        unsigned int guest_request_enabled       : 1;
>>> -        unsigned int guest_request_sync          : 1;
>>> +        uint16_t write_ctrlreg_enabled       : 4;
>>> +        uint16_t write_ctrlreg_sync          : 4;
>>> +        uint16_t write_ctrlreg_onchangeonly  : 4;
>>> +        uint16_t mov_to_msr_enabled          : 1;
>>> +        uint16_t mov_to_msr_extended         : 1;
>>> +        uint16_t singlestep_enabled          : 1;
>>> +        uint16_t software_breakpoint_enabled : 1;
>>>       } monitor;
>> What is this type change supposed to achieve in general, and in
>> particular in the context of this patch?
> 
> Some time before this patch there was a discussion I had w/ ARM 
> maintainer Ian Campbell who made the suggestion to try and move parts of 
> the monitor vm-events code to the common-side.
> (you can find that discussion here: 
> https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxx/msg54139.html).
> In short, the reason for his suggestion was that that previous attempt 
> of mine to add guest-request support for ARM highlighted code 
> duplication between X86 and ARM if I were to leave the monitor vm-events 
> code as it was (that is, completely arch-specific). In an effort to 
> avoid that, I first submitted a 7 patch-series which tried to move as 
> much as possible to the common-side.
> "As much as possible" meant guest-request, ctrl-reg write, single-step 
> and software breakpoints, all moved to the common-side. That also meant 
> introducing some Kconfigs to selectively activate some parts of that 
> now-common code, since not all of it was used on ARM at that moment. 
> Although conceptually that code could have remained on the common-side, 
> Tamas pointed out that we could get rid of the Kconfigs (which in his 
> opinion bloated the code) by only moving at the moment what is 
> implemented on both X86 and ARM. As for the rest, he noted that when I 
> add implementations for the other monitor vm-events on ARM as well, I 
> could move that to common as well. The above is a result of that - 
> guest-request is implemented on both X86 and ARM with this patch, hence 
> I also moved it to common.

I agree there are two fields being removed. But the question was
why you also change the type of the other fields.

>>> --- a/xen/include/asm-x86/hvm/event.h
>>> +++ b/xen/include/asm-x86/hvm/event.h
>>> @@ -1,5 +1,9 @@
>>>   /*
>>> - * event.h: Hardware virtual machine assist events.
>>> + * include/asm-x86/hvm/event.h
>>> + *
>>> + * Arch-specific hardware virtual machine event abstractions.
>>> + *
>>> + * Copyright (c) 2016, Bitdefender S.R.L.
>> Is this true? Namely, was _all_ of the code here written by people
>> on your company, including what may have got moved here from
>> elsewhere?
> 
> My bad. Removing that line would be satisfactory, yes?

To me, yes.

Jan

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