| Message ID | 1480433602-13290-2-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 29.11.16 at 16:33, <boris.ostrovsky@oracle.com> wrote: > @@ -108,12 +111,12 @@ static void pmt_update_time(PMTState *s) > s->last_gtime = curr_gtime; > > /* Update timer value atomically wrt lock-free reads in handle_pmt_io(). */ > - *(volatile uint32_t *)&s->pm.tmr_val = tmr_val; > + *(volatile uint32_t *)&acpi->tmr_val = tmr_val; Please use write_atomic() instead of retaining this casting. > @@ -197,7 +202,7 @@ static int handle_evt_io( > } > else /* p->dir == IOREQ_READ */ > { > - data = s->pm.pm1a_sts | (((uint32_t) s->pm.pm1a_en) << 16); > + data = acpi->pm1a_sts | (((uint32_t) acpi->pm1a_en) << 16); Please drop the stray blank and parentheses. > @@ -237,16 +243,17 @@ static int handle_pmt_io( > * updated value with a lock-free atomic read. > */ > spin_barrier(&s->lock); > - *val = read_atomic(&s->pm.tmr_val); > + *val = read_atomic(&(acpi->tmr_val)); Please don't add unnecessary parentheses. > @@ -261,21 +268,21 @@ static int pmtimer_save(struct domain *d, hvm_domain_context_t *h) > x = (((s->vcpu->arch.hvm_vcpu.guest_time ?: hvm_get_guest_time(s->vcpu)) - > s->last_gtime) * s->scale) >> 32; > if ( x < 1UL<<31 ) > - s->pm.tmr_val += x; > - if ( (s->pm.tmr_val & TMR_VAL_MSB) != msb ) > - s->pm.pm1a_sts |= TMR_STS; > + acpi->tmr_val += x; Hard tab. > --- a/xen/include/public/arch-x86/hvm/save.h > +++ b/xen/include/public/arch-x86/hvm/save.h > @@ -525,16 +525,16 @@ DECLARE_HVM_SAVE_TYPE(HPET, 12, struct hvm_hw_hpet); > > > /* > - * PM timer > + * ACPI registers > */ > > -struct hvm_hw_pmtimer { > +struct hvm_hw_acpi { > uint32_t tmr_val; /* PM_TMR_BLK.TMR_VAL: 32bit free-running counter */ > uint16_t pm1a_sts; /* PM1a_EVT_BLK.PM1a_STS: status register */ > uint16_t pm1a_en; /* PM1a_EVT_BLK.PM1a_EN: enable register */ > }; > > -DECLARE_HVM_SAVE_TYPE(PMTIMER, 13, struct hvm_hw_pmtimer); > +DECLARE_HVM_SAVE_TYPE(ACPI, 13, struct hvm_hw_acpi); However much I appreciate this switch to a better name, I'm not convinced we can actually do this as easily: There's no __XEN_TOOLS__ guard anywhere in this file, and hence everything here is part of the stable ABI. I'm afraid you minimally will have to add interface version guards, retaining the old naming for old consumers. Jan
On 12/01/2016 10:52 AM, Jan Beulich wrote: >> --- a/xen/include/public/arch-x86/hvm/save.h >> +++ b/xen/include/public/arch-x86/hvm/save.h >> @@ -525,16 +525,16 @@ DECLARE_HVM_SAVE_TYPE(HPET, 12, struct hvm_hw_hpet); >> >> >> /* >> - * PM timer >> + * ACPI registers >> */ >> >> -struct hvm_hw_pmtimer { >> +struct hvm_hw_acpi { >> uint32_t tmr_val; /* PM_TMR_BLK.TMR_VAL: 32bit free-running counter */ >> uint16_t pm1a_sts; /* PM1a_EVT_BLK.PM1a_STS: status register */ >> uint16_t pm1a_en; /* PM1a_EVT_BLK.PM1a_EN: enable register */ >> }; >> >> -DECLARE_HVM_SAVE_TYPE(PMTIMER, 13, struct hvm_hw_pmtimer); >> +DECLARE_HVM_SAVE_TYPE(ACPI, 13, struct hvm_hw_acpi); > However much I appreciate this switch to a better name, I'm not > convinced we can actually do this as easily: There's no > __XEN_TOOLS__ guard anywhere in this file, and hence everything > here is part of the stable ABI. I'm afraid you minimally will have to > add interface version guards, retaining the old naming for old > consumers. Right, I haven't though about out-of-tree users. Should new fields (added in patch 7) also be guarded? -boris
On 01/12/16 16:28, Boris Ostrovsky wrote: > On 12/01/2016 10:52 AM, Jan Beulich wrote: >>> --- a/xen/include/public/arch-x86/hvm/save.h >>> +++ b/xen/include/public/arch-x86/hvm/save.h >>> @@ -525,16 +525,16 @@ DECLARE_HVM_SAVE_TYPE(HPET, 12, struct hvm_hw_hpet); >>> >>> >>> /* >>> - * PM timer >>> + * ACPI registers >>> */ >>> >>> -struct hvm_hw_pmtimer { >>> +struct hvm_hw_acpi { >>> uint32_t tmr_val; /* PM_TMR_BLK.TMR_VAL: 32bit free-running counter */ >>> uint16_t pm1a_sts; /* PM1a_EVT_BLK.PM1a_STS: status register */ >>> uint16_t pm1a_en; /* PM1a_EVT_BLK.PM1a_EN: enable register */ >>> }; >>> >>> -DECLARE_HVM_SAVE_TYPE(PMTIMER, 13, struct hvm_hw_pmtimer); >>> +DECLARE_HVM_SAVE_TYPE(ACPI, 13, struct hvm_hw_acpi); >> However much I appreciate this switch to a better name, I'm not >> convinced we can actually do this as easily: There's no >> __XEN_TOOLS__ guard anywhere in this file, and hence everything >> here is part of the stable ABI. I'm afraid you minimally will have to >> add interface version guards, retaining the old naming for old >> consumers. > > Right, I haven't though about out-of-tree users. Should new fields > (added in patch 7) also be guarded? Be aware that my Hypervisor migration v2 plans (which follow the CPUID plans) will remove all of this (as it should never have gotten into the ABI to start with), and replace it with something looking suspiciously like the other migration v2 stream formats. If you can get away without changing names for now, probably best to just leave comment. ~Andrew
On 12/01/2016 11:29 AM, Andrew Cooper wrote: > On 01/12/16 16:28, Boris Ostrovsky wrote: >> On 12/01/2016 10:52 AM, Jan Beulich wrote: >>>> --- a/xen/include/public/arch-x86/hvm/save.h >>>> +++ b/xen/include/public/arch-x86/hvm/save.h >>>> @@ -525,16 +525,16 @@ DECLARE_HVM_SAVE_TYPE(HPET, 12, struct hvm_hw_hpet); >>>> >>>> >>>> /* >>>> - * PM timer >>>> + * ACPI registers >>>> */ >>>> >>>> -struct hvm_hw_pmtimer { >>>> +struct hvm_hw_acpi { >>>> uint32_t tmr_val; /* PM_TMR_BLK.TMR_VAL: 32bit free-running counter */ >>>> uint16_t pm1a_sts; /* PM1a_EVT_BLK.PM1a_STS: status register */ >>>> uint16_t pm1a_en; /* PM1a_EVT_BLK.PM1a_EN: enable register */ >>>> }; >>>> >>>> -DECLARE_HVM_SAVE_TYPE(PMTIMER, 13, struct hvm_hw_pmtimer); >>>> +DECLARE_HVM_SAVE_TYPE(ACPI, 13, struct hvm_hw_acpi); >>> However much I appreciate this switch to a better name, I'm not >>> convinced we can actually do this as easily: There's no >>> __XEN_TOOLS__ guard anywhere in this file, and hence everything >>> here is part of the stable ABI. I'm afraid you minimally will have to >>> add interface version guards, retaining the old naming for old >>> consumers. >> Right, I haven't though about out-of-tree users. Should new fields >> (added in patch 7) also be guarded? > Be aware that my Hypervisor migration v2 plans (which follow the CPUID > plans) will remove all of this (as it should never have gotten into the > ABI to start with), and replace it with something looking suspiciously > like the other migration v2 stream formats. > > If you can get away without changing names for now, probably best to > just leave comment. OK, I can leave it as pmtimer then. -boris
On Tue, Nov 29, 2016 at 10:33:08AM -0500, Boris Ostrovsky wrote: > These registers (pm1a specifically) are not all specific to pm timer > and are accessed by non-pmtimer code (for example, sleep/power button > emulation). > > In addition to moving those regsters to struct hvm_domain rename > HVM save state structures and routines as well. > > No functional changes are introduced. > > (While this file is being modified, also add emacs mode style rune) > > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > --- > Changes in v4: > * New patch > > tools/misc/xen-hvmctx.c | 4 +- Acked-by: Wei Liu <wei.liu2@citrix.com>
diff --git a/tools/misc/xen-hvmctx.c b/tools/misc/xen-hvmctx.c index 32be120..8e8a245 100644 --- a/tools/misc/xen-hvmctx.c +++ b/tools/misc/xen-hvmctx.c @@ -342,7 +342,7 @@ static void dump_hpet(void) static void dump_pmtimer(void) { - HVM_SAVE_TYPE(PMTIMER) p; + HVM_SAVE_TYPE(ACPI) p; READ(p); printf(" ACPI PM: TMR_VAL 0x%x, PM1a_STS 0x%x, PM1a_EN 0x%x\n", p.tmr_val, (unsigned) p.pm1a_sts, (unsigned) p.pm1a_en); @@ -462,7 +462,7 @@ int main(int argc, char **argv) case HVM_SAVE_CODE(PIT): dump_pit(); break; case HVM_SAVE_CODE(RTC): dump_rtc(); break; case HVM_SAVE_CODE(HPET): dump_hpet(); break; - case HVM_SAVE_CODE(PMTIMER): dump_pmtimer(); break; + case HVM_SAVE_CODE(ACPI): dump_pmtimer(); break; case HVM_SAVE_CODE(MTRR): dump_mtrr(); break; case HVM_SAVE_CODE(VIRIDIAN_DOMAIN): dump_viridian_domain(); break; case HVM_SAVE_CODE(VIRIDIAN_VCPU): dump_viridian_vcpu(); break; diff --git a/xen/arch/x86/hvm/pmtimer.c b/xen/arch/x86/hvm/pmtimer.c index 99d1e86..5144928 100644 --- a/xen/arch/x86/hvm/pmtimer.c +++ b/xen/arch/x86/hvm/pmtimer.c @@ -56,9 +56,11 @@ /* Dispatch SCIs based on the PM1a_STS and PM1a_EN registers */ static void pmt_update_sci(PMTState *s) { + struct hvm_hw_acpi *acpi = &s->vcpu->domain->arch.hvm_domain.acpi; + ASSERT(spin_is_locked(&s->lock)); - if ( s->pm.pm1a_en & s->pm.pm1a_sts & SCI_MASK ) + if ( acpi->pm1a_en & acpi->pm1a_sts & SCI_MASK ) hvm_isa_irq_assert(s->vcpu->domain, SCI_IRQ); else hvm_isa_irq_deassert(s->vcpu->domain, SCI_IRQ); @@ -72,7 +74,7 @@ void hvm_acpi_power_button(struct domain *d) return; spin_lock(&s->lock); - s->pm.pm1a_sts |= PWRBTN_STS; + d->arch.hvm_domain.acpi.pm1a_sts |= PWRBTN_STS; pmt_update_sci(s); spin_unlock(&s->lock); } @@ -85,7 +87,7 @@ void hvm_acpi_sleep_button(struct domain *d) return; spin_lock(&s->lock); - s->pm.pm1a_sts |= SLPBTN_STS; + d->arch.hvm_domain.acpi.pm1a_sts |= PWRBTN_STS; pmt_update_sci(s); spin_unlock(&s->lock); } @@ -95,7 +97,8 @@ void hvm_acpi_sleep_button(struct domain *d) static void pmt_update_time(PMTState *s) { uint64_t curr_gtime, tmp; - uint32_t tmr_val = s->pm.tmr_val, msb = tmr_val & TMR_VAL_MSB; + struct hvm_hw_acpi *acpi = &s->vcpu->domain->arch.hvm_domain.acpi; + uint32_t tmr_val = acpi->tmr_val, msb = tmr_val & TMR_VAL_MSB; ASSERT(spin_is_locked(&s->lock)); @@ -108,12 +111,12 @@ static void pmt_update_time(PMTState *s) s->last_gtime = curr_gtime; /* Update timer value atomically wrt lock-free reads in handle_pmt_io(). */ - *(volatile uint32_t *)&s->pm.tmr_val = tmr_val; + *(volatile uint32_t *)&acpi->tmr_val = tmr_val; /* If the counter's MSB has changed, set the status bit */ if ( (tmr_val & TMR_VAL_MSB) != msb ) { - s->pm.pm1a_sts |= TMR_STS; + acpi->pm1a_sts |= TMR_STS; pmt_update_sci(s); } } @@ -133,7 +136,8 @@ static void pmt_timer_callback(void *opaque) pmt_update_time(s); /* How close are we to the next MSB flip? */ - pmt_cycles_until_flip = TMR_VAL_MSB - (s->pm.tmr_val & (TMR_VAL_MSB - 1)); + pmt_cycles_until_flip = TMR_VAL_MSB - + (s->vcpu->domain->arch.hvm_domain.acpi.tmr_val & (TMR_VAL_MSB - 1)); /* Overall time between MSB flips */ time_until_flip = (1000000000ULL << 23) / FREQUENCE_PMTIMER; @@ -152,6 +156,7 @@ static int handle_evt_io( int dir, unsigned int port, unsigned int bytes, uint32_t *val) { struct vcpu *v = current; + struct hvm_hw_acpi *acpi = &v->domain->arch.hvm_domain.acpi; PMTState *s = &v->domain->arch.hvm_domain.pl_time->vpmt; uint32_t addr, data, byte; int i; @@ -175,16 +180,16 @@ static int handle_evt_io( { /* PM1a_STS register bits are write-to-clear */ case 0 /* PM1a_STS_ADDR */: - s->pm.pm1a_sts &= ~byte; + acpi->pm1a_sts &= ~byte; break; case 1 /* PM1a_STS_ADDR + 1 */: - s->pm.pm1a_sts &= ~(byte << 8); + acpi->pm1a_sts &= ~(byte << 8); break; case 2 /* PM1a_EN_ADDR */: - s->pm.pm1a_en = (s->pm.pm1a_en & 0xff00) | byte; + acpi->pm1a_en = (acpi->pm1a_en & 0xff00) | byte; break; case 3 /* PM1a_EN_ADDR + 1 */: - s->pm.pm1a_en = (s->pm.pm1a_en & 0xff) | (byte << 8); + acpi->pm1a_en = (acpi->pm1a_en & 0xff) | (byte << 8); break; default: gdprintk(XENLOG_WARNING, @@ -197,7 +202,7 @@ static int handle_evt_io( } else /* p->dir == IOREQ_READ */ { - data = s->pm.pm1a_sts | (((uint32_t) s->pm.pm1a_en) << 16); + data = acpi->pm1a_sts | (((uint32_t) acpi->pm1a_en) << 16); data >>= 8 * addr; if ( bytes == 1 ) data &= 0xff; else if ( bytes == 2 ) data &= 0xffff; @@ -215,6 +220,7 @@ static int handle_pmt_io( int dir, unsigned int port, unsigned int bytes, uint32_t *val) { struct vcpu *v = current; + struct hvm_hw_acpi *acpi = &v->domain->arch.hvm_domain.acpi; PMTState *s = &v->domain->arch.hvm_domain.pl_time->vpmt; if ( bytes != 4 || dir != IOREQ_READ ) @@ -226,7 +232,7 @@ static int handle_pmt_io( { /* We hold the lock: update timer value and return it. */ pmt_update_time(s); - *val = s->pm.tmr_val; + *val = acpi->tmr_val; spin_unlock(&s->lock); } else @@ -237,16 +243,17 @@ static int handle_pmt_io( * updated value with a lock-free atomic read. */ spin_barrier(&s->lock); - *val = read_atomic(&s->pm.tmr_val); + *val = read_atomic(&(acpi->tmr_val)); } return X86EMUL_OKAY; } -static int pmtimer_save(struct domain *d, hvm_domain_context_t *h) +static int acpi_save(struct domain *d, hvm_domain_context_t *h) { + struct hvm_hw_acpi *acpi = &d->arch.hvm_domain.acpi; PMTState *s = &d->arch.hvm_domain.pl_time->vpmt; - uint32_t x, msb = s->pm.tmr_val & TMR_VAL_MSB; + uint32_t x, msb = acpi->tmr_val & TMR_VAL_MSB; int rc; if ( !has_vpm(d) ) @@ -261,21 +268,21 @@ static int pmtimer_save(struct domain *d, hvm_domain_context_t *h) x = (((s->vcpu->arch.hvm_vcpu.guest_time ?: hvm_get_guest_time(s->vcpu)) - s->last_gtime) * s->scale) >> 32; if ( x < 1UL<<31 ) - s->pm.tmr_val += x; - if ( (s->pm.tmr_val & TMR_VAL_MSB) != msb ) - s->pm.pm1a_sts |= TMR_STS; + acpi->tmr_val += x; + if ( (acpi->tmr_val & TMR_VAL_MSB) != msb ) + acpi->pm1a_sts |= TMR_STS; /* No point in setting the SCI here because we'll already have saved the * IRQ and *PIC state; we'll fix it up when we restore the domain */ - - rc = hvm_save_entry(PMTIMER, 0, h, &s->pm); + rc = hvm_save_entry(ACPI, 0, h, acpi); spin_unlock(&s->lock); return rc; } -static int pmtimer_load(struct domain *d, hvm_domain_context_t *h) +static int acpi_load(struct domain *d, hvm_domain_context_t *h) { + struct hvm_hw_acpi *acpi = &d->arch.hvm_domain.acpi; PMTState *s = &d->arch.hvm_domain.pl_time->vpmt; if ( !has_vpm(d) ) @@ -284,7 +291,7 @@ static int pmtimer_load(struct domain *d, hvm_domain_context_t *h) spin_lock(&s->lock); /* Reload the registers */ - if ( hvm_load_entry(PMTIMER, h, &s->pm) ) + if ( hvm_load_entry(ACPI, h, acpi) ) { spin_unlock(&s->lock); return -EINVAL; @@ -302,7 +309,7 @@ static int pmtimer_load(struct domain *d, hvm_domain_context_t *h) return 0; } -HVM_REGISTER_SAVE_RESTORE(PMTIMER, pmtimer_save, pmtimer_load, +HVM_REGISTER_SAVE_RESTORE(ACPI, acpi_save, acpi_load, 1, HVMSR_PER_DOM); int pmtimer_change_ioport(struct domain *d, unsigned int version) @@ -377,5 +384,15 @@ void pmtimer_reset(struct domain *d) return; /* Reset the counter. */ - d->arch.hvm_domain.pl_time->vpmt.pm.tmr_val = 0; + d->arch.hvm_domain.acpi.tmr_val = 0; } + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h index f34d784..d55b432 100644 --- a/xen/include/asm-x86/hvm/domain.h +++ b/xen/include/asm-x86/hvm/domain.h @@ -102,6 +102,8 @@ struct hvm_domain { struct hvm_vioapic *vioapic; struct hvm_hw_stdvga stdvga; + struct hvm_hw_acpi acpi; + /* VCPU which is current target for 8259 interrupts. */ struct vcpu *i8259_target; diff --git a/xen/include/asm-x86/hvm/vpt.h b/xen/include/asm-x86/hvm/vpt.h index a27bea4..1b7213d 100644 --- a/xen/include/asm-x86/hvm/vpt.h +++ b/xen/include/asm-x86/hvm/vpt.h @@ -121,7 +121,6 @@ typedef struct RTCState { #define FREQUENCE_PMTIMER 3579545 /* Timer should run at 3.579545 MHz */ typedef struct PMTState { - struct hvm_hw_pmtimer pm; /* 32bit timer value */ struct vcpu *vcpu; /* Keeps sync with this vcpu's guest-time */ uint64_t last_gtime; /* Last (guest) time we updated the timer */ uint32_t not_accounted; /* time not accounted at last update */ diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h index 8d73b51..3997487 100644 --- a/xen/include/public/arch-x86/hvm/save.h +++ b/xen/include/public/arch-x86/hvm/save.h @@ -525,16 +525,16 @@ DECLARE_HVM_SAVE_TYPE(HPET, 12, struct hvm_hw_hpet); /* - * PM timer + * ACPI registers */ -struct hvm_hw_pmtimer { +struct hvm_hw_acpi { uint32_t tmr_val; /* PM_TMR_BLK.TMR_VAL: 32bit free-running counter */ uint16_t pm1a_sts; /* PM1a_EVT_BLK.PM1a_STS: status register */ uint16_t pm1a_en; /* PM1a_EVT_BLK.PM1a_EN: enable register */ }; -DECLARE_HVM_SAVE_TYPE(PMTIMER, 13, struct hvm_hw_pmtimer); +DECLARE_HVM_SAVE_TYPE(ACPI, 13, struct hvm_hw_acpi); /* * MTRR MSRs
These registers (pm1a specifically) are not all specific to pm timer and are accessed by non-pmtimer code (for example, sleep/power button emulation). In addition to moving those regsters to struct hvm_domain rename HVM save state structures and routines as well. No functional changes are introduced. (While this file is being modified, also add emacs mode style rune) Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- Changes in v4: * New patch tools/misc/xen-hvmctx.c | 4 +- xen/arch/x86/hvm/pmtimer.c | 67 +++++++++++++++++++++------------- xen/include/asm-x86/hvm/domain.h | 2 + xen/include/asm-x86/hvm/vpt.h | 1 - xen/include/public/arch-x86/hvm/save.h | 6 +-- 5 files changed, 49 insertions(+), 31 deletions(-)