| Message ID | 1480433602-13290-7-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: > --- /dev/null > +++ b/xen/arch/x86/hvm/acpi.c > @@ -0,0 +1,24 @@ > +/* acpi.c: ACPI access handling > + * > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. > + */ > +#include <xen/errno.h> > +#include <xen/lib.h> > +#include <xen/sched.h> > + > + > +int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, gas_t *gas, I guess the gas pointer can be const? > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -1144,6 +1144,29 @@ struct xen_domctl_psr_cat_op { > typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); > > +/* ACPI Generic Address Structure */ > +typedef struct gas { xen_acpi_gas > +#define XEN_ACPI_SYSTEM_MEMORY 0 > +#define XEN_ACPI_SYSTEM_IO 1 > + uint8_t space_id; /* Address space */ > + uint8_t bit_width; /* Size in bits of given register */ > + uint8_t bit_offset; /* Bit offset within the register */ > + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */ > + uint64_t address; /* 64-bit address of register */ uint64_aligned_t with explicit padding added ahead of it. And then there's the question of what uses of this will look like: I'm not convinced we need to stick to the exact ACPI layout here, unless you expect (or could imagine) for the tool stack to hold GAS structures coming from elsewhere in its hands. If we don't follow the layout as strictly, we could namely widen bit_width (and maybe bit_offset) to allow for larger transfers in one go. And in such a relaxed model I don't think we'd need access_width at all as a field. > +} gas_t; xen_acpi_gas_t > + > +struct xen_domctl_acpi_access { > + gas_t gas; /* IN: Register being accessed */ > + > +#define XEN_DOMCTL_ACPI_READ 0 > +#define XEN_DOMCTL_ACPI_WRITE 1 > + uint8_t rw; /* IN: Read or write */ > + > + XEN_GUEST_HANDLE_64(uint8) val; /* IN/OUT: data */ I'd prefer if we made this void, as there's no type associated with the data really. And please add explicit padding again. Jan
On 12/01/2016 11:06 AM, Jan Beulich wrote: > >> +++ b/xen/include/public/domctl.h >> @@ -1144,6 +1144,29 @@ struct xen_domctl_psr_cat_op { >> typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; >> DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); >> >> +/* ACPI Generic Address Structure */ >> +typedef struct gas { > xen_acpi_gas > >> +#define XEN_ACPI_SYSTEM_MEMORY 0 >> +#define XEN_ACPI_SYSTEM_IO 1 >> + uint8_t space_id; /* Address space */ >> + uint8_t bit_width; /* Size in bits of given register */ >> + uint8_t bit_offset; /* Bit offset within the register */ >> + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */ >> + uint64_t address; /* 64-bit address of register */ > uint64_aligned_t with explicit padding added ahead of it. > > And then there's the question of what uses of this will look like: > I'm not convinced we need to stick to the exact ACPI layout > here, unless you expect (or could imagine) for the tool stack to > hold GAS structures coming from elsewhere in its hands. If we > don't follow the layout as strictly, we could namely widen > bit_width (and maybe bit_offset) to allow for larger transfers > in one go. And in such a relaxed model I don't think we'd need > access_width at all as a field. There is indeed no current need to use actual ACPI GAS layout but then it's not GAS, really, and should be named something else. -boris
>>> On 01.12.16 at 17:43, <boris.ostrovsky@oracle.com> wrote: > On 12/01/2016 11:06 AM, Jan Beulich wrote: >> >>> +++ b/xen/include/public/domctl.h >>> @@ -1144,6 +1144,29 @@ struct xen_domctl_psr_cat_op { >>> typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; >>> DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); >>> >>> +/* ACPI Generic Address Structure */ >>> +typedef struct gas { >> xen_acpi_gas >> >>> +#define XEN_ACPI_SYSTEM_MEMORY 0 >>> +#define XEN_ACPI_SYSTEM_IO 1 >>> + uint8_t space_id; /* Address space */ >>> + uint8_t bit_width; /* Size in bits of given register */ >>> + uint8_t bit_offset; /* Bit offset within the register */ >>> + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */ >>> + uint64_t address; /* 64-bit address of register */ >> uint64_aligned_t with explicit padding added ahead of it. >> >> And then there's the question of what uses of this will look like: >> I'm not convinced we need to stick to the exact ACPI layout >> here, unless you expect (or could imagine) for the tool stack to >> hold GAS structures coming from elsewhere in its hands. If we >> don't follow the layout as strictly, we could namely widen >> bit_width (and maybe bit_offset) to allow for larger transfers >> in one go. And in such a relaxed model I don't think we'd need >> access_width at all as a field. > > There is indeed no current need to use actual ACPI GAS layout but then > it's not GAS, really, and should be named something else. Which of course is fine by me; I had referred to that structure only for the underlying principle of specifying how to access the data. Jan
On 12/02/2016 02:48 AM, Jan Beulich wrote: >>>> On 01.12.16 at 17:43, <boris.ostrovsky@oracle.com> wrote: >> On 12/01/2016 11:06 AM, Jan Beulich wrote: >>> >>>> +++ b/xen/include/public/domctl.h >>>> @@ -1144,6 +1144,29 @@ struct xen_domctl_psr_cat_op { >>>> typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; >>>> DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); >>>> >>>> +/* ACPI Generic Address Structure */ >>>> +typedef struct gas { >>> xen_acpi_gas >>> >>>> +#define XEN_ACPI_SYSTEM_MEMORY 0 >>>> +#define XEN_ACPI_SYSTEM_IO 1 >>>> + uint8_t space_id; /* Address space */ >>>> + uint8_t bit_width; /* Size in bits of given register */ >>>> + uint8_t bit_offset; /* Bit offset within the register */ >>>> + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */ >>>> + uint64_t address; /* 64-bit address of register */ >>> uint64_aligned_t with explicit padding added ahead of it. >>> >>> And then there's the question of what uses of this will look like: >>> I'm not convinced we need to stick to the exact ACPI layout >>> here, unless you expect (or could imagine) for the tool stack to >>> hold GAS structures coming from elsewhere in its hands. If we >>> don't follow the layout as strictly, we could namely widen >>> bit_width (and maybe bit_offset) to allow for larger transfers >>> in one go. And in such a relaxed model I don't think we'd need >>> access_width at all as a field. >> >> There is indeed no current need to use actual ACPI GAS layout but then >> it's not GAS, really, and should be named something else. > > Which of course is fine by me; I had referred to that structure only > for the underlying principle of specifying how to access the data. Are there any registers that are not byte-aligned or not whole number of bytes? I am thinking about dropping bit_offset (along with access_width) and making bit_width (byte_)width. And keeping the latter as uint8_t will also implicitly limit register size to 256 bytes which I think is a reasonable size limit. -boris
Hi Boris, On 29/11/16 15:33, Boris Ostrovsky wrote: > This domctl will allow toolstack to read and write some > ACPI registers. It will be available to both x86 and ARM > but will be implemented first only for x86 Can you explain why we would need this on ARM? Cheers, > > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > --- > Changes in v4: > * New patch > > xen/arch/x86/domctl.c | 9 +++++++++ > xen/arch/x86/hvm/Makefile | 1 + > xen/arch/x86/hvm/acpi.c | 24 ++++++++++++++++++++++++ > xen/include/asm-x86/hvm/domain.h | 3 +++ > xen/include/public/domctl.h | 25 +++++++++++++++++++++++++ > 5 files changed, 62 insertions(+) > create mode 100644 xen/arch/x86/hvm/acpi.c > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index 2a2fe04..111bcbb 100644 > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -1430,6 +1430,15 @@ long arch_do_domctl( > } > break; > > + case XEN_DOMCTL_acpi_access: > + if ( !is_hvm_domain(d) ) > + ret = -EINVAL; > + else > + ret = hvm_acpi_domctl_access(d, domctl->u.acpi_access.rw, > + &domctl->u.acpi_access.gas, > + domctl->u.acpi_access.val); > + break; > + > default: > ret = iommu_do_domctl(domctl, d, u_domctl); > break; > diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile > index f750d13..bae3244 100644 > --- a/xen/arch/x86/hvm/Makefile > +++ b/xen/arch/x86/hvm/Makefile > @@ -1,6 +1,7 @@ > subdir-y += svm > subdir-y += vmx > > +obj-y += acpi.o > obj-y += asid.o > obj-y += emulate.o > obj-y += hpet.o > diff --git a/xen/arch/x86/hvm/acpi.c b/xen/arch/x86/hvm/acpi.c > new file mode 100644 > index 0000000..7d42eaf > --- /dev/null > +++ b/xen/arch/x86/hvm/acpi.c > @@ -0,0 +1,24 @@ > +/* acpi.c: ACPI access handling > + * > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. > + */ > +#include <xen/errno.h> > +#include <xen/lib.h> > +#include <xen/sched.h> > + > + > +int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, gas_t *gas, > + XEN_GUEST_HANDLE_PARAM(uint8) arg) > +{ > + return -ENOSYS; > +} > + > +/* > + * 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 d55b432..c5cd86c 100644 > --- a/xen/include/asm-x86/hvm/domain.h > +++ b/xen/include/asm-x86/hvm/domain.h > @@ -158,6 +158,9 @@ struct hvm_domain { > > #define hap_enabled(d) ((d)->arch.hvm_domain.hap_enabled) > > +int hvm_acpi_domctl_access(struct domain *currd, uint8_t rw, gas_t *gas, > + XEN_GUEST_HANDLE_PARAM(uint8) arg); > + > #endif /* __ASM_X86_HVM_DOMAIN_H__ */ > > /* > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 177319d..26fe009 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -1144,6 +1144,29 @@ struct xen_domctl_psr_cat_op { > typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); > > +/* ACPI Generic Address Structure */ > +typedef struct gas { > +#define XEN_ACPI_SYSTEM_MEMORY 0 > +#define XEN_ACPI_SYSTEM_IO 1 > + uint8_t space_id; /* Address space */ > + uint8_t bit_width; /* Size in bits of given register */ > + uint8_t bit_offset; /* Bit offset within the register */ > + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */ > + uint64_t address; /* 64-bit address of register */ > +} gas_t; > + > +struct xen_domctl_acpi_access { > + gas_t gas; /* IN: Register being accessed */ > + > +#define XEN_DOMCTL_ACPI_READ 0 > +#define XEN_DOMCTL_ACPI_WRITE 1 > + uint8_t rw; /* IN: Read or write */ > + > + XEN_GUEST_HANDLE_64(uint8) val; /* IN/OUT: data */ > +}; > +typedef struct xen_domctl_acpi_access xen_domctl_acpi_access_t; > +DEFINE_XEN_GUEST_HANDLE(xen_domctl_acpi_access_t); > + > struct xen_domctl { > uint32_t cmd; > #define XEN_DOMCTL_createdomain 1 > @@ -1221,6 +1244,7 @@ struct xen_domctl { > #define XEN_DOMCTL_monitor_op 77 > #define XEN_DOMCTL_psr_cat_op 78 > #define XEN_DOMCTL_soft_reset 79 > +#define XEN_DOMCTL_acpi_access 80 > #define XEN_DOMCTL_gdbsx_guestmemio 1000 > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > @@ -1283,6 +1307,7 @@ struct xen_domctl { > struct xen_domctl_psr_cmt_op psr_cmt_op; > struct xen_domctl_monitor_op monitor_op; > struct xen_domctl_psr_cat_op psr_cat_op; > + struct xen_domctl_acpi_access acpi_access; > uint8_t pad[128]; > } u; > }; >
>>> On 12.12.16 at 14:08, <boris.ostrovsky@oracle.com> wrote: > > On 12/02/2016 02:48 AM, Jan Beulich wrote: >>>>> On 01.12.16 at 17:43, <boris.ostrovsky@oracle.com> wrote: >>> On 12/01/2016 11:06 AM, Jan Beulich wrote: >>>> >>>>> +++ b/xen/include/public/domctl.h >>>>> @@ -1144,6 +1144,29 @@ struct xen_domctl_psr_cat_op { >>>>> typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; >>>>> DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); >>>>> >>>>> +/* ACPI Generic Address Structure */ >>>>> +typedef struct gas { >>>> xen_acpi_gas >>>> >>>>> +#define XEN_ACPI_SYSTEM_MEMORY 0 >>>>> +#define XEN_ACPI_SYSTEM_IO 1 >>>>> + uint8_t space_id; /* Address space */ >>>>> + uint8_t bit_width; /* Size in bits of given register */ >>>>> + uint8_t bit_offset; /* Bit offset within the register */ >>>>> + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */ >>>>> + uint64_t address; /* 64-bit address of register */ >>>> uint64_aligned_t with explicit padding added ahead of it. >>>> >>>> And then there's the question of what uses of this will look like: >>>> I'm not convinced we need to stick to the exact ACPI layout >>>> here, unless you expect (or could imagine) for the tool stack to >>>> hold GAS structures coming from elsewhere in its hands. If we >>>> don't follow the layout as strictly, we could namely widen >>>> bit_width (and maybe bit_offset) to allow for larger transfers >>>> in one go. And in such a relaxed model I don't think we'd need >>>> access_width at all as a field. >>> >>> There is indeed no current need to use actual ACPI GAS layout but then >>> it's not GAS, really, and should be named something else. >> >> Which of course is fine by me; I had referred to that structure only >> for the underlying principle of specifying how to access the data. > > Are there any registers that are not byte-aligned or not whole number of > bytes? > > I am thinking about dropping bit_offset (along with access_width) and > making bit_width (byte_)width. And keeping the latter as uint8_t will > also implicitly limit register size to 256 bytes which I think is a > reasonable size limit. Since we're doing the emulation (and hence defining the registers) we could require no such unusual registers. This would be something we can't simplify only if we foresee ever needing to hand through a hardware register without interposing any emulation. Whether limiting to 256 bytes is reasonable I'm not so sure, otoh. Jan
On 12/12/2016 08:28 AM, Julien Grall wrote: > Hi Boris, > > On 29/11/16 15:33, Boris Ostrovsky wrote: >> This domctl will allow toolstack to read and write some >> ACPI registers. It will be available to both x86 and ARM >> but will be implemented first only for x86 > > Can you explain why we would need this on ARM? What this is meant to say is that the new domctl is not specific to x86 and therefore is available as a common (as opposed to arch-specific) interface. If ARM ever needs to access ACPI space from toostack it should be able to use it (provided that an implementation for that is available in arch/arm). -boris > > Cheers, > >> >> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> >> --- >> Changes in v4: >> * New patch >> >> xen/arch/x86/domctl.c | 9 +++++++++ >> xen/arch/x86/hvm/Makefile | 1 + >> xen/arch/x86/hvm/acpi.c | 24 ++++++++++++++++++++++++ >> xen/include/asm-x86/hvm/domain.h | 3 +++ >> xen/include/public/domctl.h | 25 +++++++++++++++++++++++++ >> 5 files changed, 62 insertions(+) >> create mode 100644 xen/arch/x86/hvm/acpi.c >> >> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c >> index 2a2fe04..111bcbb 100644 >> --- a/xen/arch/x86/domctl.c >> +++ b/xen/arch/x86/domctl.c >> @@ -1430,6 +1430,15 @@ long arch_do_domctl( >> } >> break; >> >> + case XEN_DOMCTL_acpi_access: >> + if ( !is_hvm_domain(d) ) >> + ret = -EINVAL; >> + else >> + ret = hvm_acpi_domctl_access(d, domctl->u.acpi_access.rw, >> + &domctl->u.acpi_access.gas, >> + domctl->u.acpi_access.val); >> + break; >> + >> default: >> ret = iommu_do_domctl(domctl, d, u_domctl); >> break; >> diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile >> index f750d13..bae3244 100644 >> --- a/xen/arch/x86/hvm/Makefile >> +++ b/xen/arch/x86/hvm/Makefile >> @@ -1,6 +1,7 @@ >> subdir-y += svm >> subdir-y += vmx >> >> +obj-y += acpi.o >> obj-y += asid.o >> obj-y += emulate.o >> obj-y += hpet.o >> diff --git a/xen/arch/x86/hvm/acpi.c b/xen/arch/x86/hvm/acpi.c >> new file mode 100644 >> index 0000000..7d42eaf >> --- /dev/null >> +++ b/xen/arch/x86/hvm/acpi.c >> @@ -0,0 +1,24 @@ >> +/* acpi.c: ACPI access handling >> + * >> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights >> reserved. >> + */ >> +#include <xen/errno.h> >> +#include <xen/lib.h> >> +#include <xen/sched.h> >> + >> + >> +int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, gas_t *gas, >> + XEN_GUEST_HANDLE_PARAM(uint8) arg) >> +{ >> + return -ENOSYS; >> +} >> + >> +/* >> + * 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 d55b432..c5cd86c 100644 >> --- a/xen/include/asm-x86/hvm/domain.h >> +++ b/xen/include/asm-x86/hvm/domain.h >> @@ -158,6 +158,9 @@ struct hvm_domain { >> >> #define hap_enabled(d) ((d)->arch.hvm_domain.hap_enabled) >> >> +int hvm_acpi_domctl_access(struct domain *currd, uint8_t rw, gas_t >> *gas, >> + XEN_GUEST_HANDLE_PARAM(uint8) arg); >> + >> #endif /* __ASM_X86_HVM_DOMAIN_H__ */ >> >> /* >> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h >> index 177319d..26fe009 100644 >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -1144,6 +1144,29 @@ struct xen_domctl_psr_cat_op { >> typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; >> DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); >> >> +/* ACPI Generic Address Structure */ >> +typedef struct gas { >> +#define XEN_ACPI_SYSTEM_MEMORY 0 >> +#define XEN_ACPI_SYSTEM_IO 1 >> + uint8_t space_id; /* Address space */ >> + uint8_t bit_width; /* Size in bits of given register */ >> + uint8_t bit_offset; /* Bit offset within the register */ >> + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */ >> + uint64_t address; /* 64-bit address of register */ >> +} gas_t; >> + >> +struct xen_domctl_acpi_access { >> + gas_t gas; /* IN: Register being >> accessed */ >> + >> +#define XEN_DOMCTL_ACPI_READ 0 >> +#define XEN_DOMCTL_ACPI_WRITE 1 >> + uint8_t rw; /* IN: Read or write */ >> + >> + XEN_GUEST_HANDLE_64(uint8) val; /* IN/OUT: data */ >> +}; >> +typedef struct xen_domctl_acpi_access xen_domctl_acpi_access_t; >> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_acpi_access_t); >> + >> struct xen_domctl { >> uint32_t cmd; >> #define XEN_DOMCTL_createdomain 1 >> @@ -1221,6 +1244,7 @@ struct xen_domctl { >> #define XEN_DOMCTL_monitor_op 77 >> #define XEN_DOMCTL_psr_cat_op 78 >> #define XEN_DOMCTL_soft_reset 79 >> +#define XEN_DOMCTL_acpi_access 80 >> #define XEN_DOMCTL_gdbsx_guestmemio 1000 >> #define XEN_DOMCTL_gdbsx_pausevcpu 1001 >> #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 >> @@ -1283,6 +1307,7 @@ struct xen_domctl { >> struct xen_domctl_psr_cmt_op psr_cmt_op; >> struct xen_domctl_monitor_op monitor_op; >> struct xen_domctl_psr_cat_op psr_cat_op; >> + struct xen_domctl_acpi_access acpi_access; >> uint8_t pad[128]; >> } u; >> }; >> >
On 12/12/2016 09:02 AM, Jan Beulich wrote: >>>> On 12.12.16 at 14:08, <boris.ostrovsky@oracle.com> wrote: >> On 12/02/2016 02:48 AM, Jan Beulich wrote: >>>>>> On 01.12.16 at 17:43, <boris.ostrovsky@oracle.com> wrote: >>>> On 12/01/2016 11:06 AM, Jan Beulich wrote: >>>>>> +++ b/xen/include/public/domctl.h >>>>>> @@ -1144,6 +1144,29 @@ struct xen_domctl_psr_cat_op { >>>>>> typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; >>>>>> DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); >>>>>> >>>>>> +/* ACPI Generic Address Structure */ >>>>>> +typedef struct gas { >>>>> xen_acpi_gas >>>>> >>>>>> +#define XEN_ACPI_SYSTEM_MEMORY 0 >>>>>> +#define XEN_ACPI_SYSTEM_IO 1 >>>>>> + uint8_t space_id; /* Address space */ >>>>>> + uint8_t bit_width; /* Size in bits of given register */ >>>>>> + uint8_t bit_offset; /* Bit offset within the register */ >>>>>> + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */ >>>>>> + uint64_t address; /* 64-bit address of register */ >>>>> uint64_aligned_t with explicit padding added ahead of it. >>>>> >>>>> And then there's the question of what uses of this will look like: >>>>> I'm not convinced we need to stick to the exact ACPI layout >>>>> here, unless you expect (or could imagine) for the tool stack to >>>>> hold GAS structures coming from elsewhere in its hands. If we >>>>> don't follow the layout as strictly, we could namely widen >>>>> bit_width (and maybe bit_offset) to allow for larger transfers >>>>> in one go. And in such a relaxed model I don't think we'd need >>>>> access_width at all as a field. >>>> There is indeed no current need to use actual ACPI GAS layout but then >>>> it's not GAS, really, and should be named something else. >>> Which of course is fine by me; I had referred to that structure only >>> for the underlying principle of specifying how to access the data. >> Are there any registers that are not byte-aligned or not whole number of >> bytes? >> >> I am thinking about dropping bit_offset (along with access_width) and >> making bit_width (byte_)width. And keeping the latter as uint8_t will >> also implicitly limit register size to 256 bytes which I think is a >> reasonable size limit. > Since we're doing the emulation (and hence defining the registers) > we could require no such unusual registers. This would be something > we can't simplify only if we foresee ever needing to hand through a > hardware register without interposing any emulation. > > Whether limiting to 256 bytes is reasonable I'm not so sure, otoh. When would we ever need to access anything larger? I'd think that the common case is a few (1-4) bytes. The one instance when this is not true is the VCPU map and 256 bytes allow for 16K VCPUs, which I suspect we won't reach in a while. But I can increase the length to uint16_t if you feel it's would be better. -boris
>>> On 12.12.16 at 17:19, <boris.ostrovsky@oracle.com> wrote: > On 12/12/2016 09:02 AM, Jan Beulich wrote: >>>>> On 12.12.16 at 14:08, <boris.ostrovsky@oracle.com> wrote: >>> On 12/02/2016 02:48 AM, Jan Beulich wrote: >>>>>>> On 01.12.16 at 17:43, <boris.ostrovsky@oracle.com> wrote: >>>>> On 12/01/2016 11:06 AM, Jan Beulich wrote: >>>>>>> +++ b/xen/include/public/domctl.h >>>>>>> @@ -1144,6 +1144,29 @@ struct xen_domctl_psr_cat_op { >>>>>>> typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; >>>>>>> DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); >>>>>>> >>>>>>> +/* ACPI Generic Address Structure */ >>>>>>> +typedef struct gas { >>>>>> xen_acpi_gas >>>>>> >>>>>>> +#define XEN_ACPI_SYSTEM_MEMORY 0 >>>>>>> +#define XEN_ACPI_SYSTEM_IO 1 >>>>>>> + uint8_t space_id; /* Address space */ >>>>>>> + uint8_t bit_width; /* Size in bits of given register */ >>>>>>> + uint8_t bit_offset; /* Bit offset within the register */ >>>>>>> + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */ >>>>>>> + uint64_t address; /* 64-bit address of register */ >>>>>> uint64_aligned_t with explicit padding added ahead of it. >>>>>> >>>>>> And then there's the question of what uses of this will look like: >>>>>> I'm not convinced we need to stick to the exact ACPI layout >>>>>> here, unless you expect (or could imagine) for the tool stack to >>>>>> hold GAS structures coming from elsewhere in its hands. If we >>>>>> don't follow the layout as strictly, we could namely widen >>>>>> bit_width (and maybe bit_offset) to allow for larger transfers >>>>>> in one go. And in such a relaxed model I don't think we'd need >>>>>> access_width at all as a field. >>>>> There is indeed no current need to use actual ACPI GAS layout but then >>>>> it's not GAS, really, and should be named something else. >>>> Which of course is fine by me; I had referred to that structure only >>>> for the underlying principle of specifying how to access the data. >>> Are there any registers that are not byte-aligned or not whole number of >>> bytes? >>> >>> I am thinking about dropping bit_offset (along with access_width) and >>> making bit_width (byte_)width. And keeping the latter as uint8_t will >>> also implicitly limit register size to 256 bytes which I think is a >>> reasonable size limit. >> Since we're doing the emulation (and hence defining the registers) >> we could require no such unusual registers. This would be something >> we can't simplify only if we foresee ever needing to hand through a >> hardware register without interposing any emulation. >> >> Whether limiting to 256 bytes is reasonable I'm not so sure, otoh. > > When would we ever need to access anything larger? I'd think that the > common case is a few (1-4) bytes. The one instance when this is not true > is the VCPU map and 256 bytes allow for 16K VCPUs, which I suspect we > won't reach in a while. > > But I can increase the length to uint16_t if you feel it's would be better. It's domctl, so we can change it later anyway. As said - I'm not really sure here. Jan
Hi Boris, On 12/12/16 16:11, Boris Ostrovsky wrote: > On 12/12/2016 08:28 AM, Julien Grall wrote: >> Hi Boris, >> >> On 29/11/16 15:33, Boris Ostrovsky wrote: >>> This domctl will allow toolstack to read and write some >>> ACPI registers. It will be available to both x86 and ARM >>> but will be implemented first only for x86 >> >> Can you explain why we would need this on ARM? > > What this is meant to say is that the new domctl is not specific to x86 > and therefore is available as a common (as opposed to arch-specific) > interface. I thought you were planning to make this implemented on ARM and was trying to understanding why :). > > If ARM ever needs to access ACPI space from toostack it should be able > to use it (provided that an implementation for that is available in > arch/arm). Ok. I can't tell whether we would need it in the future. Currently, it is not necessary. Cheers,
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 2a2fe04..111bcbb 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1430,6 +1430,15 @@ long arch_do_domctl( } break; + case XEN_DOMCTL_acpi_access: + if ( !is_hvm_domain(d) ) + ret = -EINVAL; + else + ret = hvm_acpi_domctl_access(d, domctl->u.acpi_access.rw, + &domctl->u.acpi_access.gas, + domctl->u.acpi_access.val); + break; + default: ret = iommu_do_domctl(domctl, d, u_domctl); break; diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile index f750d13..bae3244 100644 --- a/xen/arch/x86/hvm/Makefile +++ b/xen/arch/x86/hvm/Makefile @@ -1,6 +1,7 @@ subdir-y += svm subdir-y += vmx +obj-y += acpi.o obj-y += asid.o obj-y += emulate.o obj-y += hpet.o diff --git a/xen/arch/x86/hvm/acpi.c b/xen/arch/x86/hvm/acpi.c new file mode 100644 index 0000000..7d42eaf --- /dev/null +++ b/xen/arch/x86/hvm/acpi.c @@ -0,0 +1,24 @@ +/* acpi.c: ACPI access handling + * + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + */ +#include <xen/errno.h> +#include <xen/lib.h> +#include <xen/sched.h> + + +int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, gas_t *gas, + XEN_GUEST_HANDLE_PARAM(uint8) arg) +{ + return -ENOSYS; +} + +/* + * 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 d55b432..c5cd86c 100644 --- a/xen/include/asm-x86/hvm/domain.h +++ b/xen/include/asm-x86/hvm/domain.h @@ -158,6 +158,9 @@ struct hvm_domain { #define hap_enabled(d) ((d)->arch.hvm_domain.hap_enabled) +int hvm_acpi_domctl_access(struct domain *currd, uint8_t rw, gas_t *gas, + XEN_GUEST_HANDLE_PARAM(uint8) arg); + #endif /* __ASM_X86_HVM_DOMAIN_H__ */ /* diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 177319d..26fe009 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -1144,6 +1144,29 @@ struct xen_domctl_psr_cat_op { typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); +/* ACPI Generic Address Structure */ +typedef struct gas { +#define XEN_ACPI_SYSTEM_MEMORY 0 +#define XEN_ACPI_SYSTEM_IO 1 + uint8_t space_id; /* Address space */ + uint8_t bit_width; /* Size in bits of given register */ + uint8_t bit_offset; /* Bit offset within the register */ + uint8_t access_width; /* Minimum Access size (ACPI 3.0) */ + uint64_t address; /* 64-bit address of register */ +} gas_t; + +struct xen_domctl_acpi_access { + gas_t gas; /* IN: Register being accessed */ + +#define XEN_DOMCTL_ACPI_READ 0 +#define XEN_DOMCTL_ACPI_WRITE 1 + uint8_t rw; /* IN: Read or write */ + + XEN_GUEST_HANDLE_64(uint8) val; /* IN/OUT: data */ +}; +typedef struct xen_domctl_acpi_access xen_domctl_acpi_access_t; +DEFINE_XEN_GUEST_HANDLE(xen_domctl_acpi_access_t); + struct xen_domctl { uint32_t cmd; #define XEN_DOMCTL_createdomain 1 @@ -1221,6 +1244,7 @@ struct xen_domctl { #define XEN_DOMCTL_monitor_op 77 #define XEN_DOMCTL_psr_cat_op 78 #define XEN_DOMCTL_soft_reset 79 +#define XEN_DOMCTL_acpi_access 80 #define XEN_DOMCTL_gdbsx_guestmemio 1000 #define XEN_DOMCTL_gdbsx_pausevcpu 1001 #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 @@ -1283,6 +1307,7 @@ struct xen_domctl { struct xen_domctl_psr_cmt_op psr_cmt_op; struct xen_domctl_monitor_op monitor_op; struct xen_domctl_psr_cat_op psr_cat_op; + struct xen_domctl_acpi_access acpi_access; uint8_t pad[128]; } u; };
This domctl will allow toolstack to read and write some ACPI registers. It will be available to both x86 and ARM but will be implemented first only for x86 Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- Changes in v4: * New patch xen/arch/x86/domctl.c | 9 +++++++++ xen/arch/x86/hvm/Makefile | 1 + xen/arch/x86/hvm/acpi.c | 24 ++++++++++++++++++++++++ xen/include/asm-x86/hvm/domain.h | 3 +++ xen/include/public/domctl.h | 25 +++++++++++++++++++++++++ 5 files changed, 62 insertions(+) create mode 100644 xen/arch/x86/hvm/acpi.c