WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH V3 3/6] xen, Intruduce pmtimer_change_ioport and

To: <anthony.perard@xxxxxxxxxx>, Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH V3 3/6] xen, Intruduce pmtimer_change_ioport and HVM_PARAM_ACPI_NEW_IOPORT.
From: Keir Fraser <keir@xxxxxxx>
Date: Fri, 29 Oct 2010 13:25:51 +0100
Cc:
Delivery-date: Fri, 29 Oct 2010 05:33:46 -0700
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:sender:user-agent:date :subject:from:to:message-id:thread-topic:thread-index:in-reply-to :mime-version:content-type:content-transfer-encoding; bh=cGOjH5o8+2aGeOivfaqVtkF4o6NtOxIRQwIso5k/6zw=; b=cKxVC9DJQEcr2o6XPd/a11o+79rlm5Jua0fGAIY48P4lAwo78XrqEt6lVfllQfefoM gsUlLicErswlUiaO3qjnt47L/3h54NXh3SaIzlKOoKMdJ8JSBPpIO6WYLvg4zGqxIHIb XENlatmxb9kzsvYRKFmiwT4pgqnRioP6KAxzE=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=sender:user-agent:date:subject:from:to:message-id:thread-topic :thread-index:in-reply-to:mime-version:content-type :content-transfer-encoding; b=ns5Bf6BqAo0nSIGmzn4lkuiRJXWO5hlfbc97pRz+RnP+8ublEs1S5XNqLFu7uybJPT 3JK/36C4A5bbhQdrkaz7GNPX6vLYwzxPavtHtEyoNgLkaqpBXjeviBn7xxioQpXPtrMG 759ekUVElMSDlfNorjighZJ5sy9u+ZvwsvVPs=
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1288354119-1916-4-git-send-email-anthony.perard@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: Act3ZGxxWwBHAX6Txk+V+N4svMsarg==
Thread-topic: [Xen-devel] [PATCH V3 3/6] xen, Intruduce pmtimer_change_ioport and HVM_PARAM_ACPI_NEW_IOPORT.
User-agent: Microsoft-Entourage/12.26.0.100708
On 29/10/2010 13:08, "anthony.perard@xxxxxxxxxx" <anthony.perard@xxxxxxxxxx>
wrote:

> From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> 
> By default, Xen will handle the old ACPI IO port. But it can switch to
> the new one by setting the HVM_PARAM_ACPI_NEW_IOPORT to 1.

Fine, but this new parameter deserves a better explanatory comment in
include/public/hvm/params.h. Its meaning is subtle and not immediately
obvious. So go into some detail -- that it is basically a version number,
current valid versions are 0 and 1, and the effect of setting each of those
valid version numbers. Note that some other parameters have maybe half a
page of accompanying explanatory comment. It's better to write a bit too
much rather than too little, and ensures our interface is well documented
and hence well used and maintained, because others will understand it.

Apart from this one point, I am happy for the entire patch series to be
checked in. So once you've made that improvement:
Acked-by: Keir Fraser <keir@xxxxxxx>

 Thanks,
 Keir

> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c          |    9 +++++++++
>  xen/arch/x86/hvm/pmtimer.c      |   26 ++++++++++++++++++++++++--
>  xen/include/asm-x86/hvm/vpt.h   |    1 +
>  xen/include/public/hvm/params.h |    5 ++++-
>  4 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 94190d3..ea296e5 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2991,6 +2991,15 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE(void)
> arg)
>                      rc = -EINVAL;
>  
>                  break;
> +            case HVM_PARAM_ACPI_NEW_IOPORT:
> +                if (a.value == 1)
> +                    pmtimer_change_ioport(d, 1);
> +                else if (a.value == 0)
> +                    pmtimer_change_ioport(d, 0);
> +                else
> +                    rc = -EINVAL;
> +
> +                break;
>              }
>  
>              if ( rc == 0 )
> diff --git a/xen/arch/x86/hvm/pmtimer.c b/xen/arch/x86/hvm/pmtimer.c
> index 1b9ab7b..f046201 100644
> --- a/xen/arch/x86/hvm/pmtimer.c
> +++ b/xen/arch/x86/hvm/pmtimer.c
> @@ -24,6 +24,9 @@
>  #include <asm/acpi.h> /* for hvm_acpi_power_button prototype */
>  
>  /* Slightly more readable port I/O addresses for the registers we intercept
> */
> +#define PM1a_STS_ADDR_OLD (ACPI_PM1A_EVT_BLK_ADDRESS_OLD)
> +#define PM1a_EN_ADDR_OLD  (ACPI_PM1A_EVT_BLK_ADDRESS_OLD + 2)
> +#define TMR_VAL_ADDR_OLD  (ACPI_PM_TMR_BLK_ADDRESS_OLD)
>  #define PM1a_STS_ADDR (ACPI_PM1A_EVT_BLK_ADDRESS)
>  #define PM1a_EN_ADDR  (ACPI_PM1A_EVT_BLK_ADDRESS + 2)
>  #define TMR_VAL_ADDR  (ACPI_PM_TMR_BLK_ADDRESS)
> @@ -155,16 +158,20 @@ static int handle_evt_io(
>              switch ( addr )
>              {
>                  /* PM1a_STS register bits are write-to-clear */
> +            case PM1a_STS_ADDR_OLD:
>              case PM1a_STS_ADDR:
>                  s->pm.pm1a_sts &= ~byte;
>                  break;
> +            case PM1a_STS_ADDR_OLD + 1:
>              case PM1a_STS_ADDR + 1:
>                  s->pm.pm1a_sts &= ~(byte << 8);
>                  break;
>                  
> +            case PM1a_EN_ADDR_OLD:
>              case PM1a_EN_ADDR:
>                  s->pm.pm1a_en = (s->pm.pm1a_en & 0xff00) | byte;
>                  break;
> +            case PM1a_EN_ADDR_OLD + 1:
>              case PM1a_EN_ADDR + 1:
>                  s->pm.pm1a_en = (s->pm.pm1a_en & 0xff) | (byte << 8);
>                  break;
> @@ -272,6 +279,21 @@ static int pmtimer_load(struct domain *d,
> hvm_domain_context_t *h)
>  HVM_REGISTER_SAVE_RESTORE(PMTIMER, pmtimer_save, pmtimer_load,
>                            1, HVMSR_PER_DOM);
>  
> +void pmtimer_change_ioport(struct domain *d, int use_new)
> +{
> +    if (use_new) {
> +        register_portio_handler(d, TMR_VAL_ADDR, 4, handle_pmt_io);
> +        register_portio_handler(d, PM1a_STS_ADDR, 4, handle_evt_io);
> +        unregister_portio_handler(d, TMR_VAL_ADDR_OLD, 4);
> +        unregister_portio_handler(d, PM1a_STS_ADDR_OLD, 4);
> +    } else {
> +        register_portio_handler(d, TMR_VAL_ADDR_OLD, 4, handle_pmt_io);
> +        register_portio_handler(d, PM1a_STS_ADDR_OLD, 4, handle_evt_io);
> +        unregister_portio_handler(d, TMR_VAL_ADDR, 4);
> +        unregister_portio_handler(d, PM1a_STS_ADDR, 4);
> +    }
> +}
> +
>  void pmtimer_init(struct vcpu *v)
>  {
>      PMTState *s = &v->domain->arch.hvm_domain.pl_time.vpmt;
> @@ -284,8 +306,8 @@ void pmtimer_init(struct vcpu *v)
>  
>      /* Intercept port I/O (need two handlers because PM1a_CNT is between
>       * PM1a_EN and TMR_VAL and is handled by qemu) */
> -    register_portio_handler(v->domain, TMR_VAL_ADDR, 4, handle_pmt_io);
> -    register_portio_handler(v->domain, PM1a_STS_ADDR, 4, handle_evt_io);
> +    register_portio_handler(v->domain, TMR_VAL_ADDR_OLD, 4, handle_pmt_io);
> +    register_portio_handler(v->domain, PM1a_STS_ADDR_OLD, 4, handle_evt_io);
>  
>      /* Set up callback to fire SCIs when the MSB of TMR_VAL changes */
>      init_timer(&s->timer, pmt_timer_callback, s, v->processor);
> diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h
> index 65b0dff..6b68888 100644
> --- a/xen/include/asm-x86/hvm/vpt.h
> +++ b/xen/include/asm-x86/hvm/vpt.h
> @@ -179,6 +179,7 @@ void rtc_update_clock(struct domain *d);
>  void pmtimer_init(struct vcpu *v);
>  void pmtimer_deinit(struct domain *d);
>  void pmtimer_reset(struct domain *d);
> +void pmtimer_change_ioport(struct domain *d, int use_new);
>  
>  void hpet_init(struct vcpu *v);
>  void hpet_deinit(struct domain *d);
> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
> index 673148b..40af8d8 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -113,6 +113,9 @@
>  #define HVM_PARAM_CONSOLE_PFN    17
>  #define HVM_PARAM_CONSOLE_EVTCHN 18
>  
> -#define HVM_NR_PARAMS          19
> +/* to specify which firmware acpi ioport is used */
> +#define HVM_PARAM_ACPI_NEW_IOPORT 19
> +
> +#define HVM_NR_PARAMS          20
>  
>  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel