Re: [PATCH v2 3/7] xen/vm-events: Move monitor_domctl to common-side.

[Corneliu ZUZU <czuzu@bitdefender.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4076 bytes --]

On 2/10/2016 7:56 PM, Tamas K Lengyel wrote:
>
>
> On Wed, Feb 10, 2016 at 10:34 AM, Corneliu ZUZU <czuzu@bitdefender.com 
> <mailto:czuzu@bitdefender.com>> wrote:
>
>
>     On 2/10/2016 6:39 PM, Tamas K Lengyel wrote:
>>
>>
>>         diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
>>         index 3a90f47..e46be1b 100644
>>         --- a/xen/arch/x86/Kconfig
>>         +++ b/xen/arch/x86/Kconfig
>>         @@ -14,6 +14,10 @@ config X86
>>                 select HAS_MEM_ACCESS
>>                 select HAS_MEM_PAGING
>>                 select HAS_MEM_SHARING
>>         +       select HAS_VM_EVENT_WRITE_CTRLREG
>>
>>
>>     You mentioned in the previous revision of this series that
>>     currently you only have plans to implement write_ctrleg and
>>     guest_request events for ARM. I think singlestep and
>>     software_breakpoint should not be moved to common without a clear
>>     plan to have those implemented. Now, if you were to include the
>>     implementation of write_ctrlreg and guest_request in this series
>>     and leave the others in x86 as they are now, I don't think there
>>     would be any reason to have these Kconfig options present at all.
>
>     Moving what made sense to be moved to common was a suggestion of
>     Ian's.
>     The purpose of this patch is to --avoid-- having to go through
>     this process again
>     when an implementation of feature X for architecture A != X86
>     comes into place.
>     IMHO what is common should stay in common and I don't see any
>     issues w/
>     having them there, only advantages (future implementations of
>     these features will
>     be easier).
>     Maybe Ian can chime in on this.
>
>
> That's the upside. The downside is that in the interim, while those 
> features are not implemented, we need to add a bunch of Kconfig 
> variables to decide under what build they are available.

Technically, you don't need to add anything unless you implement that 
feature.
E.g. the ARM Kconfig currently contains no mention of these options, 
since they're not implemented there @ all.
And when implemented they're only added once and it's one line "select 
HAS_....", it's not like you have to specify a command-line
parameter when building Xen or something like that, so IMO they don't 
add considerable complexity.
And after all, these kind of situations (i.e. activating/deactivating 
code based on architecture) are why arch-specific Kconfigs exist, right? 
Why not use them? :)

> If it was moved to common only when the feature is available for all 
> architectures, we wouldn't need that many ifdefs and the code would be 
> clearer. So I do see why it would be beneficial having these moved to 
> common now, but I still rather have it happen when it's necessary and 
> without the Kconfig settings.

What if a 3rd architecture comes into place, you'd have to move them 
back from common to the arch-side and get back to the code duplication 
we just got rid of?
And if you then also implement it for the 3rd architecture, you move 
them back to common from the arch-side?
It seems uncomfortable having to keep track of all architectures when 
wanting to add such a feature implementation for just one of them.
I don't know what & if such plans exist for Xen, but why make that 
future process of adding in support for other architectures 
unnecessarily complicated,
even if it won't happen soon?

Also, IMO the code is clearer like this:
* you know what parts interest you when implementing parts of these 
features/when debugging/when simply looking @ the code
* the #ifdefs make it possible for that code to be put in common => that 
makes it *clear* that those code parts are NOT
architecture specific and their implementation can be safely used for 
all architectures.
* code duplication is avoided => avoid having to reflect a modification 
when it happens in more places than it would be necessary

There are disadvantages, everything has a downside but IMHO they are 
minor compared to the alternative.

Ian, any comments on this? :)

Thank you,
Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 7344 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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