Re: [V1 PATCH 3/3] pvh: disallow PHYSDEVOP_pirq_eoi_gmfn_v2/v1

["Jan Beulich" <JBeulich@suse.com>]

>>> On 03.03.14 at 13:30, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 02/28/2014 08:09 AM, Jan Beulich wrote:
>>>>> On 28.02.14 at 04:06, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
>>> So, sigh, in conclusion, what would you prefer:
>>>
>>>    a. add this new check to hvm_physdev_op.
>>>    b. leave the new check here.
>>>    c. add this check to hvm_physdev_op, and move the existing pvh checks
>>>       from do_physdev_op to hvm_physdev_op also.
>>
> 
>>   From a formal pov, all PVH special casing
>> should go away from hvm_physdev_op _and_ do_physdev_op,
>> with the possibly exception of things that are really unsuitable
>> for PVH (in which case it would be more clean to have a distinct
>> pvh_physdev_op). Temporary workarounds like the one here
>> should obviously(!?) go where other temporary workarounds
>> are - with what we have, that's hvm_physdev_op(). As I think
>> Tim said elsewhere - sprinkling around arbitrary PVH checks is
>> just not acceptable. I begin to regret that I gave my okay for
>> all these fixme-s and stuff to go in (with it yet to be seen when
>> their elimination would start), because I'm getting the feeling
>> that you take this as an excuse to add further such stuff.
>>
>> So to answer your question above: The way to go depends on
>> the long term intended behavior: If there is anything that will
>> need special casing no matter what, option d) is likely going to
>> be the cleanest one - introduce pvh_physdev_op(). But each
>> exception there either needs a proper rationale, or a proper
>> fixme annotation. And such special casing should be done with
>> future code changes in mind, i.e. considering the cost of
>> maintaining all of {do,hvm,pvh}_physdev_op() when new
>> (sub-)ops get added.
> 
> [Moving back to the main list]
> 
> So is the main reason that the checks in do_physdev_op() exist just that 
> some of the commands have not yet been implemented for PVH?
> 
> If so, I'd much rather have the checks in do_physdev_op().  I think 
> long-term, I'd prefer to have them in do_physdev_op() (as Mukesh has 
> done here) even if they're going to stay, rather than having an extra 
> function in another file with a whitelist / blacklist.  Yes, having the 
> checks in do_physdev_op() is a bit ugly, but it should make it clear in 
> one place which paths are valid for which kinds of guests.

I perhaps wouldn't mind that approach, if there wasn't already a
hvm_physdev_op() wrapper. What I first of all want is that things
get done consistently, so that one doesn't have to go guess why
similar things where done one way once, and then another way the
second time.

But there's restriction to this: Long term special cases I think would
be fine to get placed in the generic routine. Temporary workarounds
otoh seem better placed in a (temporary) wrapper, centralizing the
resulting ugliness.

Jan