Re: [PATCH 1/2] xen: Introduce PHYSDEVOP_pirq_eoi_gmfn_new

[Keir Fraser <keir.xen@gmail.com>]

On 26/01/2012 16:29, "Stefano Stabellini" <stefano.stabellini@eu.citrix.com>
wrote:

> On Thu, 26 Jan 2012, Keir Fraser wrote:
>> On 26/01/2012 15:49, "Stefano Stabellini" <Stefano.Stabellini@eu.citrix.com>
>> wrote:
>> 
>>> PHYSDEVOP_pirq_eoi_gmfn changes the semantics of PHYSDEVOP_eoi.
>>> Introduce PHYSDEVOP_pirq_eoi_gmfn_new, that is like
>>> PHYSDEVOP_pirq_eoi_gmfn but it doesn't modify the behaviour of another
>>> hypercall.
>> 
>> It's nasty that pirq_eoi_gmfn has the side effect. I suggest add a PHYSDEVOP
>> to explicitly enable/disable unmask-on-eoi (i.e., the command accepts a
>> boolean parameter). Once it is explicitly enabled/disabled in this way,
>> pirq_eoi_gmfn no longer has the side effect (regardless of whether it is
>> called before or after the explicit setting). So e.g., pv_domain.auto_unmask
>> becomes an int where 0/1 means no/yes, and -1 means default (i.e., old
>> behavour where it depends on whether PHYSDEVOP_pirq_eoi_gmfn has been
>> called).
>> 
>> This seems to me to move a bad interface in a better direction.
> 
> The problem with this approach is that by default we have an hypercall
> (PHYSDEVOP_pirq_eoi_gmfn) changing the behaviour of another one
> (PHYSDEVOP_eoi). Not only this but we have an hypercall
> (PHYSDEVOP_pirq_eoi_gmfn) violating the public interface of shared_info
> as documented in public/xen.h.
> 
> Introducing a new hypercall with the same name
> (PHYSDEVOP_pirq_eoi_gmfn_new) is the first step in admitting that the
> old hypercall was a mistake and should not be used.
> 
> I don't think we should ever change the semantics of PHYSDEVOP_eoi with
> another hypercall. If we want a PHYSDEVOP that eoi and unmask and event
> channel let's introduce PHYSDEVOP_eoi_unmask.

Okay, fine by me. See my comments on naming in the email I sent just a sec
ago.

 -- Keir