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

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

On 26/01/2012 16:11, "Keir Fraser" <keir@xen.org> 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.

...in that the auto-unmask behaviour becomes explicitly selectable, rather
than implicitly, via two different commands for setting *something else*
which only differ in a side effect. That's kind of as gross as what we have
already, or worse.

 -- Keir

>  -- Keir
> 
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> ---
>>  xen/arch/ia64/xen/domain.c    |    1 +
>>  xen/arch/ia64/xen/hypercall.c |    5 ++++-
>>  xen/arch/x86/domain.c         |    1 +
>>  xen/arch/x86/physdev.c        |    6 +++++-
>>  xen/include/asm-ia64/domain.h |    3 +++
>>  xen/include/asm-x86/domain.h  |    3 +++
>>  xen/include/public/physdev.h  |    8 ++++++++
>>  7 files changed, 25 insertions(+), 2 deletions(-)
>> 
>> diff --git a/xen/arch/ia64/xen/domain.c b/xen/arch/ia64/xen/domain.c
>> index 1ea5a90..a31bd32 100644
>> --- a/xen/arch/ia64/xen/domain.c
>> +++ b/xen/arch/ia64/xen/domain.c
>> @@ -1731,6 +1731,7 @@ int domain_relinquish_resources(struct domain *d)
>> if (d->arch.pirq_eoi_map != NULL) {
>> put_page(virt_to_page(d->arch.pirq_eoi_map));
>> d->arch.pirq_eoi_map = NULL;
>> +   d->arch.auto_unmask = 0;
>> }
>>  
>> /* Tear down shadow mode stuff. */
>> diff --git a/xen/arch/ia64/xen/hypercall.c b/xen/arch/ia64/xen/hypercall.c
>> index 130675e..44c3407 100644
>> --- a/xen/arch/ia64/xen/hypercall.c
>> +++ b/xen/arch/ia64/xen/hypercall.c
>> @@ -65,7 +65,7 @@ static long __do_pirq_guest_eoi(struct domain *d, int pirq)
>>  {
>> if ( pirq < 0 || pirq >= NR_IRQS )
>> return -EINVAL;
>> - if ( d->arch.pirq_eoi_map ) {
>> + if ( d->arch.pirq_eoi_map  && d->arch.auto_unmask ) {
>> spin_lock(&d->event_lock);
>> evtchn_unmask(pirq_to_evtchn(d, pirq));
>> spin_unlock(&d->event_lock);
>> @@ -508,6 +508,7 @@ long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>>          break;
>>      }
>>  
>> +    case PHYSDEVOP_pirq_eoi_gmfn_new:
>>      case PHYSDEVOP_pirq_eoi_gmfn: {
>>          struct physdev_pirq_eoi_gmfn info;
>>          unsigned long mfn;
>> @@ -531,6 +532,8 @@ long do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>>          }
>>  
>>          current->domain->arch.pirq_eoi_map = mfn_to_virt(mfn);
>> +  if ( cmd == PHYSDEVOP_pirq_eoi_gmfn )
>> +   current->domain->arch.auto_unmask = 1;
>>          ret = 0;
>>          break;
>>      }
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index 61d83c8..a540af7 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -2125,6 +2125,7 @@ int domain_relinquish_resources(struct domain *d)
>>                  put_page_and_type(
>>                      mfn_to_page(d->arch.pv_domain.pirq_eoi_map_mfn));
>>                  d->arch.pv_domain.pirq_eoi_map = NULL;
>> +                d->arch.pv_domain.auto_unmask = 0;
>>              }
>>          }
>>  
>> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
>> index f280c28..efd517f 100644
>> --- a/xen/arch/x86/physdev.c
>> +++ b/xen/arch/x86/physdev.c
>> @@ -271,7 +271,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>>              break;
>>          }
>>          if ( !is_hvm_domain(v->domain) &&
>> -             v->domain->arch.pv_domain.pirq_eoi_map )
>> +             v->domain->arch.pv_domain.pirq_eoi_map &&
>> +             v->domain->arch.pv_domain.auto_unmask )
>>              evtchn_unmask(pirq->evtchn);
>>          if ( !is_hvm_domain(v->domain) ||
>>               domain_pirq_to_irq(v->domain, eoi.irq) > 0 )
>> @@ -293,6 +294,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>>          break;
>>      }
>>  
>> +    case PHYSDEVOP_pirq_eoi_gmfn_new:
>>      case PHYSDEVOP_pirq_eoi_gmfn: {
>>          struct physdev_pirq_eoi_gmfn info;
>>          unsigned long mfn;
>> @@ -329,6 +331,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
>>              ret = -ENOSPC;
>>              break;
>>          }
>> +        if ( cmd == PHYSDEVOP_pirq_eoi_gmfn )
>> +            v->domain->arch.pv_domain.auto_unmask = 1;
>>  
>>          put_gfn(current->domain, info.gmfn);
>>          ret = 0;
>> diff --git a/xen/include/asm-ia64/domain.h b/xen/include/asm-ia64/domain.h
>> index 12dc3bd..8163d67 100644
>> --- a/xen/include/asm-ia64/domain.h
>> +++ b/xen/include/asm-ia64/domain.h
>> @@ -186,6 +186,9 @@ struct arch_domain {
>>      /* Shared page for notifying that explicit PIRQ EOI is required. */
>>      unsigned long *pirq_eoi_map;
>>      unsigned long pirq_eoi_map_mfn;
>> + /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
>> +  * unmask the event channel */
>> + unsigned int auto_unmask;
>>  
>>      /* Address of efi_runtime_services_t (placed in domain memory)  */
>>      void *efi_runtime;
>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>> index 00bbaeb..b0cbe65 100644
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -231,6 +231,9 @@ struct pv_domain
>>      /* Shared page for notifying that explicit PIRQ EOI is required. */
>>      unsigned long *pirq_eoi_map;
>>      unsigned long pirq_eoi_map_mfn;
>> +    /* set auto_unmask to 1 if you want PHYSDEVOP_eoi to automatically
>> +     * unmask the event channel */
>> +    unsigned int auto_unmask;
>>  
>>      /* Pseudophysical e820 map (XENMEM_memory_map).  */
>>      spinlock_t e820_lock;
>> diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
>> index 6e23295..d555719 100644
>> --- a/xen/include/public/physdev.h
>> +++ b/xen/include/public/physdev.h
>> @@ -50,6 +50,14 @@ DEFINE_XEN_GUEST_HANDLE(physdev_eoi_t);
>>   * array indexed by Xen's PIRQ value.
>>   */
>>  #define PHYSDEVOP_pirq_eoi_gmfn         17
>> +/*
>> + * Register a shared page for the hypervisor to indicate whether the
>> + * guest must issue PHYSDEVOP_eoi. This hypercall is very similar to
>> + * PHYSDEVOP_pirq_eoi_gmfn but it doesn't change the semantics of
>> + * PHYSDEVOP_eoi. The page registered is used as a bit array indexed by
>> + * Xen's PIRQ value.
>> + */
>> +#define PHYSDEVOP_pirq_eoi_gmfn_new         28
>>  struct physdev_pirq_eoi_gmfn {
>>      /* IN */
>>      xen_pfn_t gmfn;
> 
>