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

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

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.

  -George