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
|
Home