| Message ID | 1481930319-4796-5-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 17.12.16 at 00:18, <boris.ostrovsky@oracle.com> wrote: > --- a/xen/include/public/arch-x86/hvm/save.h > +++ b/xen/include/public/arch-x86/hvm/save.h > @@ -527,7 +527,37 @@ DECLARE_HVM_SAVE_TYPE(HPET, 12, struct hvm_hw_hpet); > /* > * PM timer > */ > +#if __XEN_INTERFACE_VERSION__ >= 0x00040800 > +struct hvm_hw_pmtimer { > + 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 */ > +#if defined(__XEN__) || defined(__XEN_TOOLS__) > + uint16_t gpe0_sts; > + uint16_t gpe0_en; > +#endif Why inside another #ifdef? There's no other example in this file which might have suggested to you that it needs doing this way. In fact there are also no pre-existing uses of __XEN_INTERFACE_VERSION__ in this header, so I also don't see why you added one (and then with a slightly off value check). If anything the _whole_ header would need to become Xen/tools only. > +static inline int _hvm_hw_fix_pmtimer(void *h, uint32_t size) > +{ > +#if defined(__XEN__) || defined(__XEN_TOOLS__) > + struct hvm_hw_pmtimer *acpi = (struct hvm_hw_pmtimer *)h; > + > + if ( size == sizeof(struct hvm_hw_pmtimer_compat) ) > + acpi->gpe0_sts = acpi->gpe0_en = 0; > +#endif Same here. Jan
On 12/20/2016 06:24 AM, Jan Beulich wrote: >>>> On 17.12.16 at 00:18, <boris.ostrovsky@oracle.com> wrote: >> --- a/xen/include/public/arch-x86/hvm/save.h >> +++ b/xen/include/public/arch-x86/hvm/save.h >> @@ -527,7 +527,37 @@ DECLARE_HVM_SAVE_TYPE(HPET, 12, struct hvm_hw_hpet); >> /* >> * PM timer >> */ >> +#if __XEN_INTERFACE_VERSION__ >= 0x00040800 >> +struct hvm_hw_pmtimer { >> + 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 */ >> +#if defined(__XEN__) || defined(__XEN_TOOLS__) >> + uint16_t gpe0_sts; >> + uint16_t gpe0_en; >> +#endif > Why inside another #ifdef? There's no other example in this file > which might have suggested to you that it needs doing this way. > In fact there are also no pre-existing uses of > __XEN_INTERFACE_VERSION__ in this header, so I also don't see > why you added one (and then with a slightly off value check). Don't we want users of old interface to continue using original definition of hvm_hw_timer? And not to expose them to the fix routine below? -boris > > If anything the _whole_ header would need to become Xen/tools > only. > >> +static inline int _hvm_hw_fix_pmtimer(void *h, uint32_t size) >> +{ >> +#if defined(__XEN__) || defined(__XEN_TOOLS__) >> + struct hvm_hw_pmtimer *acpi = (struct hvm_hw_pmtimer *)h; >> + >> + if ( size == sizeof(struct hvm_hw_pmtimer_compat) ) >> + acpi->gpe0_sts = acpi->gpe0_en = 0; >> +#endif > Same here. > > Jan >
>>> On 20.12.16 at 15:03, <boris.ostrovsky@oracle.com> wrote: > On 12/20/2016 06:24 AM, Jan Beulich wrote: >>>>> On 17.12.16 at 00:18, <boris.ostrovsky@oracle.com> wrote: >>> --- a/xen/include/public/arch-x86/hvm/save.h >>> +++ b/xen/include/public/arch-x86/hvm/save.h >>> @@ -527,7 +527,37 @@ DECLARE_HVM_SAVE_TYPE(HPET, 12, struct hvm_hw_hpet); >>> /* >>> * PM timer >>> */ >>> +#if __XEN_INTERFACE_VERSION__ >= 0x00040800 >>> +struct hvm_hw_pmtimer { >>> + 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 */ >>> +#if defined(__XEN__) || defined(__XEN_TOOLS__) >>> + uint16_t gpe0_sts; >>> + uint16_t gpe0_en; >>> +#endif >> Why inside another #ifdef? There's no other example in this file >> which might have suggested to you that it needs doing this way. >> In fact there are also no pre-existing uses of >> __XEN_INTERFACE_VERSION__ in this header, so I also don't see >> why you added one (and then with a slightly off value check). > > Don't we want users of old interface to continue using original > definition of hvm_hw_timer? And not to expose them to the fix routine below? There shouldn't be any such old users, because of ... >> If anything the _whole_ header would need to become Xen/tools >> only. ... this. Jan
On 12/20/2016 09:10 AM, Jan Beulich wrote: >>>> On 20.12.16 at 15:03, <boris.ostrovsky@oracle.com> wrote: >> On 12/20/2016 06:24 AM, Jan Beulich wrote: >>>>>> On 17.12.16 at 00:18, <boris.ostrovsky@oracle.com> wrote: >>>> --- a/xen/include/public/arch-x86/hvm/save.h >>>> +++ b/xen/include/public/arch-x86/hvm/save.h >>>> @@ -527,7 +527,37 @@ DECLARE_HVM_SAVE_TYPE(HPET, 12, struct hvm_hw_hpet); >>>> /* >>>> * PM timer >>>> */ >>>> +#if __XEN_INTERFACE_VERSION__ >= 0x00040800 >>>> +struct hvm_hw_pmtimer { >>>> + 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 */ >>>> +#if defined(__XEN__) || defined(__XEN_TOOLS__) >>>> + uint16_t gpe0_sts; >>>> + uint16_t gpe0_en; >>>> +#endif >>> Why inside another #ifdef? There's no other example in this file >>> which might have suggested to you that it needs doing this way. >>> In fact there are also no pre-existing uses of >>> __XEN_INTERFACE_VERSION__ in this header, so I also don't see >>> why you added one (and then with a slightly off value check). >> Don't we want users of old interface to continue using original >> definition of hvm_hw_timer? And not to expose them to the fix routine below? > There shouldn't be any such old users, because of ... > >>> If anything the _whole_ header would need to become Xen/tools >>> only. > ... this. Is this file is not supposed to be used by anyone outside of the Xen tree? -boris
>>> On 20.12.16 at 15:16, <boris.ostrovsky@oracle.com> wrote: > On 12/20/2016 09:10 AM, Jan Beulich wrote: >>>>> On 20.12.16 at 15:03, <boris.ostrovsky@oracle.com> wrote: >>> On 12/20/2016 06:24 AM, Jan Beulich wrote: >>>>>>> On 17.12.16 at 00:18, <boris.ostrovsky@oracle.com> wrote: >>>>> --- a/xen/include/public/arch-x86/hvm/save.h >>>>> +++ b/xen/include/public/arch-x86/hvm/save.h >>>>> @@ -527,7 +527,37 @@ DECLARE_HVM_SAVE_TYPE(HPET, 12, struct hvm_hw_hpet); >>>>> /* >>>>> * PM timer >>>>> */ >>>>> +#if __XEN_INTERFACE_VERSION__ >= 0x00040800 >>>>> +struct hvm_hw_pmtimer { >>>>> + 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 */ >>>>> +#if defined(__XEN__) || defined(__XEN_TOOLS__) >>>>> + uint16_t gpe0_sts; >>>>> + uint16_t gpe0_en; >>>>> +#endif >>>> Why inside another #ifdef? There's no other example in this file >>>> which might have suggested to you that it needs doing this way. >>>> In fact there are also no pre-existing uses of >>>> __XEN_INTERFACE_VERSION__ in this header, so I also don't see >>>> why you added one (and then with a slightly off value check). >>> Don't we want users of old interface to continue using original >>> definition of hvm_hw_timer? And not to expose them to the fix routine below? >> There shouldn't be any such old users, because of ... >> >>>> If anything the _whole_ header would need to become Xen/tools >>>> only. >> ... this. > > Is this file is not supposed to be used by anyone outside of the Xen tree? I don't think so, no. In any event - prior additions did not do any precautions to guard possible foreign consumers. Maybe Andrew has an opinion here ... Jan
On 20/12/2016 14:45, Jan Beulich wrote: >>>> On 20.12.16 at 15:16, <boris.ostrovsky@oracle.com> wrote: >> On 12/20/2016 09:10 AM, Jan Beulich wrote: >>>>>> On 20.12.16 at 15:03, <boris.ostrovsky@oracle.com> wrote: >>>> On 12/20/2016 06:24 AM, Jan Beulich wrote: >>>>>>>> On 17.12.16 at 00:18, <boris.ostrovsky@oracle.com> wrote: >>>>>> --- a/xen/include/public/arch-x86/hvm/save.h >>>>>> +++ b/xen/include/public/arch-x86/hvm/save.h >>>>>> @@ -527,7 +527,37 @@ DECLARE_HVM_SAVE_TYPE(HPET, 12, struct hvm_hw_hpet); >>>>>> /* >>>>>> * PM timer >>>>>> */ >>>>>> +#if __XEN_INTERFACE_VERSION__ >= 0x00040800 >>>>>> +struct hvm_hw_pmtimer { >>>>>> + 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 */ >>>>>> +#if defined(__XEN__) || defined(__XEN_TOOLS__) >>>>>> + uint16_t gpe0_sts; >>>>>> + uint16_t gpe0_en; >>>>>> +#endif >>>>> Why inside another #ifdef? There's no other example in this file >>>>> which might have suggested to you that it needs doing this way. >>>>> In fact there are also no pre-existing uses of >>>>> __XEN_INTERFACE_VERSION__ in this header, so I also don't see >>>>> why you added one (and then with a slightly off value check). >>>> Don't we want users of old interface to continue using original >>>> definition of hvm_hw_timer? And not to expose them to the fix routine below? >>> There shouldn't be any such old users, because of ... >>> >>>>> If anything the _whole_ header would need to become Xen/tools >>>>> only. >>> ... this. >> Is this file is not supposed to be used by anyone outside of the Xen tree? > I don't think so, no. In any event - prior additions did not do > any precautions to guard possible foreign consumers. Maybe > Andrew has an opinion here ... Our policies along these lines are a mess. We really should separate the guest ABI/API from the toolstack ABI/API, and by this, I mean having separate directory structures. In the meantime, all of the content in this file is only useful to entities which can make DOMCTLs to start with, so the entire file should be restricted to Xen/tools only. ~Andrew
On 12/20/2016 09:55 AM, Andrew Cooper wrote: >>> Is this file is not supposed to be used by anyone outside of the Xen tree? >> I don't think so, no. In any event - prior additions did not do >> any precautions to guard possible foreign consumers. Maybe >> Andrew has an opinion here ... > Our policies along these lines are a mess. We really should separate > the guest ABI/API from the toolstack ABI/API, and by this, I mean having > separate directory structures. > > In the meantime, all of the content in this file is only useful to > entities which can make DOMCTLs to start with, so the entire file should > be restricted to Xen/tools only. I can add that but it should be done as a separate patch as it has nothing to do with what the series is trying to provide. -boris
diff --git a/xen/arch/x86/hvm/acpi.c b/xen/arch/x86/hvm/acpi.c index 013b399..9b2885e 100644 --- a/xen/arch/x86/hvm/acpi.c +++ b/xen/arch/x86/hvm/acpi.c @@ -6,6 +6,7 @@ #include <xen/lib.h> #include <xen/sched.h> +#include <public/arch-x86/xen.h> int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, const xen_acpi_access_t *access, @@ -14,6 +15,36 @@ int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, return -ENOSYS; } +static int acpi_guest_access(int dir, unsigned int port, + unsigned int bytes, uint32_t *val) +{ + return X86EMUL_UNHANDLEABLE; +} + +static int acpi_cpumap_guest_access(int dir, unsigned int port, + unsigned int bytes, uint32_t *val) +{ + return X86EMUL_UNHANDLEABLE; +} + +void hvm_acpi_init(struct domain *d) +{ + if ( has_acpi_dm_ff(d) ) + return; + + register_portio_handler(d, XEN_ACPI_CPU_MAP, + XEN_ACPI_CPU_MAP_LEN, acpi_cpumap_guest_access); + + register_portio_handler(d, ACPI_GPE0_BLK_ADDRESS_V1, + sizeof(d->arch.hvm_domain.acpi.gpe0_sts) + + sizeof(d->arch.hvm_domain.acpi.gpe0_en), + acpi_guest_access); + register_portio_handler(d, ACPI_PM1A_EVT_BLK_ADDRESS_V1, + sizeof(d->arch.hvm_domain.acpi.pm1a_sts) + + sizeof(d->arch.hvm_domain.acpi.pm1a_en), + acpi_guest_access); +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 2b3977a..8d2dfb8 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -647,6 +647,8 @@ int hvm_domain_initialise(struct domain *d) hvm_ioreq_init(d); + hvm_acpi_init(d); + if ( is_pvh_domain(d) ) { register_portio_handler(d, 0, 0x10003, handle_pvh_io); diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index 39cc658..6470c53 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -426,6 +426,8 @@ struct arch_domain #define has_vvga(d) (!!((d)->arch.emulation_flags & XEN_X86_EMU_VGA)) #define has_viommu(d) (!!((d)->arch.emulation_flags & XEN_X86_EMU_IOMMU)) #define has_vpit(d) (!!((d)->arch.emulation_flags & XEN_X86_EMU_PIT)) +#define has_acpi_dm_ff(d) (!!((d)->arch.emulation_flags & \ + XEN_X86_EMU_ACPI_DM_FF)) #define has_arch_pdevs(d) (!list_empty(&(d)->arch.pdev_list)) diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h index 32880de..58fba33 100644 --- a/xen/include/asm-x86/hvm/domain.h +++ b/xen/include/asm-x86/hvm/domain.h @@ -166,6 +166,7 @@ struct hvm_domain { #define hap_enabled(d) ((d)->arch.hvm_domain.hap_enabled) +void hvm_acpi_init(struct domain *d); int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, const xen_acpi_access_t *access, XEN_GUEST_HANDLE_PARAM(void) arg); diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h index 8d73b51..b1d560f 100644 --- a/xen/include/public/arch-x86/hvm/save.h +++ b/xen/include/public/arch-x86/hvm/save.h @@ -527,7 +527,37 @@ DECLARE_HVM_SAVE_TYPE(HPET, 12, struct hvm_hw_hpet); /* * PM timer */ +#if __XEN_INTERFACE_VERSION__ >= 0x00040800 +struct hvm_hw_pmtimer { + 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 */ +#if defined(__XEN__) || defined(__XEN_TOOLS__) + uint16_t gpe0_sts; + uint16_t gpe0_en; +#endif +}; + +struct hvm_hw_pmtimer_compat { + uint32_t tmr_val; + uint16_t pm1a_sts; + uint16_t pm1a_en; +}; + +static inline int _hvm_hw_fix_pmtimer(void *h, uint32_t size) +{ +#if defined(__XEN__) || defined(__XEN_TOOLS__) + struct hvm_hw_pmtimer *acpi = (struct hvm_hw_pmtimer *)h; + + if ( size == sizeof(struct hvm_hw_pmtimer_compat) ) + acpi->gpe0_sts = acpi->gpe0_en = 0; +#endif + return 0; +} +DECLARE_HVM_SAVE_TYPE_COMPAT(PMTIMER, 13, struct hvm_hw_pmtimer, \ + struct hvm_hw_pmtimer_compat, _hvm_hw_fix_pmtimer); +#else /* __XEN_INTERFACE_VERSION__ */ struct hvm_hw_pmtimer { uint32_t tmr_val; /* PM_TMR_BLK.TMR_VAL: 32bit free-running counter */ uint16_t pm1a_sts; /* PM1a_EVT_BLK.PM1a_STS: status register */ @@ -535,6 +565,7 @@ struct hvm_hw_pmtimer { }; DECLARE_HVM_SAVE_TYPE(PMTIMER, 13, struct hvm_hw_pmtimer); +#endif /* __XEN_INTERFACE_VERSION__ */ /* * MTRR MSRs diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h index 12f719d..2565acd 100644 --- a/xen/include/public/arch-x86/xen.h +++ b/xen/include/public/arch-x86/xen.h @@ -283,12 +283,14 @@ struct xen_arch_domainconfig { #define XEN_X86_EMU_IOMMU (1U<<_XEN_X86_EMU_IOMMU) #define _XEN_X86_EMU_PIT 8 #define XEN_X86_EMU_PIT (1U<<_XEN_X86_EMU_PIT) +#define _XEN_X86_EMU_ACPI_DM_FF 9 +#define XEN_X86_EMU_ACPI_DM_FF (1U<<_XEN_X86_EMU_ACPI_DM_FF) #define XEN_X86_EMU_ALL (XEN_X86_EMU_LAPIC | XEN_X86_EMU_HPET | \ XEN_X86_EMU_PM | XEN_X86_EMU_RTC | \ XEN_X86_EMU_IOAPIC | XEN_X86_EMU_PIC | \ XEN_X86_EMU_VGA | XEN_X86_EMU_IOMMU | \ - XEN_X86_EMU_PIT) + XEN_X86_EMU_PIT | XEN_X86_EMU_ACPI_DM_FF) uint32_t emulation_flags; };
PVH guests will have ACPI accesses emulated by the hypervisor as opposed to QEMU (as is the case for HVM guests). This patch installs handler for accesses to PM1A, GPE0 (which is added to struct hvm_hw_acpi) and VCPU map. Logic for the handler will be provided by a later patch. Whether or not the handler needs to be installed is decided based on the value of XEN_X86_EMU_ACPI_FF flag which indicates whether emulation is implemented in QEMU. Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- Changes in v5: * Split VCPU map and ACPI register access routines * Added compat restore routine xen/arch/x86/hvm/acpi.c | 31 +++++++++++++++++++++++++++++++ xen/arch/x86/hvm/hvm.c | 2 ++ xen/include/asm-x86/domain.h | 2 ++ xen/include/asm-x86/hvm/domain.h | 1 + xen/include/public/arch-x86/hvm/save.h | 31 +++++++++++++++++++++++++++++++ xen/include/public/arch-x86/xen.h | 4 +++- 6 files changed, 70 insertions(+), 1 deletion(-)