Re: [PATCH V3 3/6] xen, Intruduce pmtimer_change_ioport and HVM_PARAM_ACPI_NEW_IOPORT.

[Keir Fraser <keir@xen.org>]

On 29/10/2010 13:08, "anthony.perard@citrix.com" <anthony.perard@citrix.com>
wrote:

> From: Anthony PERARD <anthony.perard@citrix.com>
> 
> 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@xen.org>

 Thanks,
 Keir

> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  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__ */