Re: [PATCH v2 07/12] x86/altp2m: add control of suppress_ve.

[George Dunlap <george.dunlap@eu.citrix.com>]

On 07/07/2015 05:24 PM, Ed White wrote:
> On 07/07/2015 03:10 AM, George Dunlap wrote:
>> On 07/06/2015 07:43 PM, Ed White wrote:
>>>> Introducing yet another layer -- particularly in a hooked interface like
>>>> this -- just seems clunky.  It's not the worst thing in the world; if I
>>>> thought this would be the difference between making it or not, I might
>>>> just say fix it later.  But I don't think it will; and these little
>>>> things add up.
>>>>
>>>
>>> I don't want to change set/get everywhere, and Tim already made it clear
>>> that coupling suppress_ve with p2m_type_t is not acceptable.
>>>
>>> How can I provide an implementation that does not do either of the above
>>> but does allow access to suppress_ve in a way that is acceptable?
>>>
>>> Tell me and I will do it.
>>
>> The only reason I can think that you don't want to change get/set is
>> that you think it's too much work.
>>
>> So here you go, I modified your patch; it took me 10 minutes, which is
>> less than what it would have taken me to continue arguing with you.
>> I've compile-tested it, but not done anything else (including porting
>> subsequent patches onto it).
>>
> 
> I'm disappointed that you think that. I respect yours, Jan's, etc. role
> as maintainers, and your absolute right to reject anything you think
> is inappropriate. It's clear that Jan, and now apparently you, don't
> respect my abilities or desire to do good work.
> 
> I won't make the changes you suggest because I don't think they
> represent good design, and I don't think they are the right way to
> solve the issue at hand.
> 
> I'm going to hand the Intel end of further discussions off to Ravi
> Sahita.

So let's clarify a few things.

First, I do respect your work; on the whole I've found this patch series
sensibly designed and quite readable, given how complex the feature is
you're trying to implement.

Secondly, I understand that you think that changing all the callers to
set_entry and get_entry is ugly, and that if you were the maintainer you
would do things differently.

However, the world is a big place and lots of people have different
ideas of what's ugly and what's not.  The way they do things in qemu is
different than the way they do things in libvirt, which is different
than the way they do things in the Linux kernel.  If the maintainer asks
for things to be done a certain way, you just do it that way, even if
you think it's ugly.  That's just how things are sometimes.

So when you said you didn't want to change get/set, after being told by
both myself and Jan that we thought that was the right approach, I
assumed that the reason was that you felt it unfair to be forced to do
the tedious task of tracking down all the changes and doing it yourself.

Well, it is a bit tedious to do that, and I can see how you might be
upset about doing it.  On the other hand, I think that's the right thing
to do.

So rather than try to force you to do it, or compromising on what I
think is the right thing to do, I decided, as a show of good faith, to
do the tedious task myself, so that you wouldn't have to.

It sounds like you took this as an insult; hopefully now you understand
that it wasn't.

It also sounds like you are refusing to make this change because you're
standing on your principles.  I don't really understand that attitude,
particularly in this case.  That's your choice, I'm not going to argue
with it, though I do hope you'll reconsider.

I will say, however, that if you are unwilling to compromise on this
sort of thing, then contributing to open-source projects is probably not
going to be in general a very fruitful activity for you.

 -George