| Message ID | 1481930319-4796-6-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: > +static int acpi_cpumap_access_common(struct domain *d, > + int dir, unsigned int port, > + unsigned int bytes, uint32_t *val) > +{ > + unsigned int first_byte = port - XEN_ACPI_CPU_MAP; > + > + BUILD_BUG_ON(XEN_ACPI_CPU_MAP + XEN_ACPI_CPU_MAP_LEN > + >= ACPI_GPE0_BLK_ADDRESS_V1); Just > afaict. > + > + if ( dir == XEN_DOMCTL_ACPI_READ ) > + { > + uint32_t mask = (bytes < 4) ? ~0U << (bytes * 8) : 0; > + > + /* > + * Clear bits that we are about to read to in case we > + * copy fewer than @bytes. > + */ > + *val &= mask; > + > + if ( ((d->max_vcpus + 7) / 8) > first_byte ) > + memcpy(val, (uint8_t *)d->avail_vcpus + first_byte, > + min(bytes, ((d->max_vcpus + 7) / 8) - first_byte)); > + } > + else > + /* Guests do not write CPU map */ > + return X86EMUL_UNHANDLEABLE; Perhaps make this an early bail, reducing overall indentation? > +static int acpi_access_common(struct domain *d, > + int dir, unsigned int port, > + unsigned int bytes, uint32_t *val) > +{ > + uint16_t *sts = NULL, *en = NULL; > + const uint16_t *mask_sts = NULL, *mask_en = NULL; > + static const uint16_t pm1a_sts_mask = ACPI_BITMASK_GLOBAL_LOCK_STATUS; > + static const uint16_t pm1a_en_mask = ACPI_BITMASK_GLOBAL_LOCK_ENABLE; > + static const uint16_t gpe0_sts_mask = 1U << XEN_ACPI_GPE0_CPUHP_BIT; > + static const uint16_t gpe0_en_mask = 1U << XEN_ACPI_GPE0_CPUHP_BIT; > + > + ASSERT(!has_acpi_dm_ff(d)); > + > + switch ( port ) > + { > + case ACPI_PM1A_EVT_BLK_ADDRESS_V1 ... > + ACPI_PM1A_EVT_BLK_ADDRESS_V1 + > + sizeof(d->arch.hvm_domain.acpi.pm1a_sts) + > + sizeof(d->arch.hvm_domain.acpi.pm1a_en): > + > + sts = &d->arch.hvm_domain.acpi.pm1a_sts; > + en = &d->arch.hvm_domain.acpi.pm1a_en; > + mask_sts = &pm1a_sts_mask; > + mask_en = &pm1a_en_mask; > + break; > + > + case ACPI_GPE0_BLK_ADDRESS_V1 ... > + ACPI_GPE0_BLK_ADDRESS_V1 + > + sizeof(d->arch.hvm_domain.acpi.gpe0_sts) + > + sizeof(d->arch.hvm_domain.acpi.gpe0_en): > + > + sts = &d->arch.hvm_domain.acpi.gpe0_sts; > + en = &d->arch.hvm_domain.acpi.gpe0_en; > + mask_sts = &gpe0_sts_mask; > + mask_en = &gpe0_en_mask; > + break; > + > + default: > + return X86EMUL_UNHANDLEABLE; > + } > + > + if ( dir == XEN_DOMCTL_ACPI_READ ) > + { > + uint32_t mask = (bytes < 4) ? ~0U << (bytes * 8) : 0; > + uint32_t data = (((uint32_t)*en) << 16) | *sts; There's one pair of pointless parentheses around the cast expression here. > + > + data >>= 8 * (port & 3); > + *val = (*val & mask) | (data & ~mask); > + } > + else > + { > + uint32_t v = *val; > + > + /* Status register is write-1-to-clear by guests */ > + switch ( port & 3 ) > + { > + case 0: > + *sts &= ~(v & 0xff); > + *sts &= *mask_sts; I can understand the first &=, but why would a read have this second (side) effect? I could see some sort of need for such only when you were setting any flags. > + if ( !--bytes ) > + break; > + v >>= 8; > + /* fallthrough */ > + case 1: > + *sts &= ~((v & 0xff) << 8); > + *sts &= *mask_sts; > + if ( !--bytes ) > + break; > + v >>= 8; > + /* fallthrough */ > + case 2: > + *en = ((*en & 0xff00) | (v & 0xff)) & *mask_en; > + if ( !--bytes ) > + break; > + v >>= 8; > + /* fallthrough */ > + case 3: > + *en = (((v & 0xff) << 8) | (*en & 0xff)) & *mask_en; > + break; > + } > + } > + > + return X86EMUL_OKAY; > +} > + > + > int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, No double blank lines please. > @@ -18,13 +135,19 @@ int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, > static int acpi_guest_access(int dir, unsigned int port, > unsigned int bytes, uint32_t *val) > { > - return X86EMUL_UNHANDLEABLE; > + return acpi_access_common(current->domain, > + (dir == IOREQ_READ) ? > + XEN_DOMCTL_ACPI_READ: XEN_DOMCTL_ACPI_WRITE, > + port, bytes, val); > } I don't think this one is meant to be used by the domctl, so I don't see why you need a helper here. That would also eliminate the seemingly unnecessary use of XEN_DOMCTL_* here (I think you already had said this was an oversight in an earlier reply). Jan
On 12/20/2016 06:50 AM, Jan Beulich wrote: > >> + >> + if ( dir == XEN_DOMCTL_ACPI_READ ) >> + { >> + uint32_t mask = (bytes < 4) ? ~0U << (bytes * 8) : 0; >> + uint32_t data = (((uint32_t)*en) << 16) | *sts; > There's one pair of pointless parentheses around the cast > expression here. > >> + >> + data >>= 8 * (port & 3); >> + *val = (*val & mask) | (data & ~mask); >> + } >> + else >> + { >> + uint32_t v = *val; >> + >> + /* Status register is write-1-to-clear by guests */ >> + switch ( port & 3 ) >> + { >> + case 0: >> + *sts &= ~(v & 0xff); >> + *sts &= *mask_sts; > I can understand the first &=, but why would a read have this second > (side) effect? I could see some sort of need for such only when you > were setting any flags. This is a write, not a read. > >> @@ -18,13 +135,19 @@ int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, >> static int acpi_guest_access(int dir, unsigned int port, >> unsigned int bytes, uint32_t *val) >> { >> - return X86EMUL_UNHANDLEABLE; >> + return acpi_access_common(current->domain, >> + (dir == IOREQ_READ) ? >> + XEN_DOMCTL_ACPI_READ: XEN_DOMCTL_ACPI_WRITE, >> + port, bytes, val); >> } > I don't think this one is meant to be used by the domctl, so I don't see > why you need a helper here. That would also eliminate the seemingly > unnecessary use of XEN_DOMCTL_* here (I think you already had said > this was an oversight in an earlier reply). Yes, XEN_DOMCTL_*is out of place here but I'd prefer to keep the helper to isolate the sense of 'dir' as is used by portio handling infrastructure (i.e. IOREQ_READ/WRITE) from how it is defined in the acpi code here. Especially if, as mentioned in another thread, I use bool is_write in common routines. -boris
>>> On 20.12.16 at 15:35, <boris.ostrovsky@oracle.com> wrote: > On 12/20/2016 06:50 AM, Jan Beulich wrote: >>> + else >>> + { >>> + uint32_t v = *val; >>> + >>> + /* Status register is write-1-to-clear by guests */ >>> + switch ( port & 3 ) >>> + { >>> + case 0: >>> + *sts &= ~(v & 0xff); >>> + *sts &= *mask_sts; >> I can understand the first &=, but why would a read have this second >> (side) effect? I could see some sort of need for such only when you >> were setting any flags. > > This is a write, not a read. Oh, right. But the question remains about that unexpected side effect. >>> @@ -18,13 +135,19 @@ int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, >>> static int acpi_guest_access(int dir, unsigned int port, >>> unsigned int bytes, uint32_t *val) >>> { >>> - return X86EMUL_UNHANDLEABLE; >>> + return acpi_access_common(current->domain, >>> + (dir == IOREQ_READ) ? >>> + XEN_DOMCTL_ACPI_READ: XEN_DOMCTL_ACPI_WRITE, >>> + port, bytes, val); >>> } >> I don't think this one is meant to be used by the domctl, so I don't see >> why you need a helper here. That would also eliminate the seemingly >> unnecessary use of XEN_DOMCTL_* here (I think you already had said >> this was an oversight in an earlier reply). > > Yes, XEN_DOMCTL_*is out of place here but I'd prefer to keep the helper > to isolate the sense of 'dir' as is used by portio handling > infrastructure (i.e. IOREQ_READ/WRITE) from how it is defined in the > acpi code here. Especially if, as mentioned in another thread, I use > bool is_write in common routines. Well, if the helper is needed for the domctl, then this is at least acceptable (albeit personally I'd prefer the domctl handler to do the translation, using the IOREQ_* values internally. If the helper is not needed for domctl, then XEN_DOMCTL_* shouldn't be passed here. Jan
On 12/20/2016 09:47 AM, Jan Beulich wrote: >>>> On 20.12.16 at 15:35, <boris.ostrovsky@oracle.com> wrote: >> On 12/20/2016 06:50 AM, Jan Beulich wrote: >>>> + else >>>> + { >>>> + uint32_t v = *val; >>>> + >>>> + /* Status register is write-1-to-clear by guests */ >>>> + switch ( port & 3 ) >>>> + { >>>> + case 0: >>>> + *sts &= ~(v & 0xff); >>>> + *sts &= *mask_sts; >>> I can understand the first &=, but why would a read have this second >>> (side) effect? I could see some sort of need for such only when you >>> were setting any flags. >> This is a write, not a read. > Oh, right. But the question remains about that unexpected side > effect. It indeed doesn't do anything for the case of guest access. It is guarding against setting unauthorized bits by domctl (introduced by the next patch). And I can move it into that case. However, as discussed in the thread about 06/13 patch, we may currently not need to provide domctl access to anything but VCPU map. So the question is whether domctl interface to GPE0/PM1a is needed at all. I think it's a useful interface with very little extra code required and the toolstack can use it, for example, to inject events into guests. But I don't have a specific use case. -boris
>>> On 20.12.16 at 16:29, <boris.ostrovsky@oracle.com> wrote: > On 12/20/2016 09:47 AM, Jan Beulich wrote: >>>>> On 20.12.16 at 15:35, <boris.ostrovsky@oracle.com> wrote: >>> On 12/20/2016 06:50 AM, Jan Beulich wrote: >>>>> + else >>>>> + { >>>>> + uint32_t v = *val; >>>>> + >>>>> + /* Status register is write-1-to-clear by guests */ >>>>> + switch ( port & 3 ) >>>>> + { >>>>> + case 0: >>>>> + *sts &= ~(v & 0xff); >>>>> + *sts &= *mask_sts; >>>> I can understand the first &=, but why would a read have this second >>>> (side) effect? I could see some sort of need for such only when you >>>> were setting any flags. >>> This is a write, not a read. >> Oh, right. But the question remains about that unexpected side >> effect. > > It indeed doesn't do anything for the case of guest access. It is > guarding against setting unauthorized bits by domctl (introduced by the > next patch). And I can move it into that case. > > However, as discussed in the thread about 06/13 patch, we may currently > not need to provide domctl access to anything but VCPU map. So the > question is whether domctl interface to GPE0/PM1a is needed at all. I > think it's a useful interface with very little extra code required and > the toolstack can use it, for example, to inject events into guests. But > I don't have a specific use case. To be honest, I'd keep out anything we don't really need. Jan
On 20/12/2016 15:41, Jan Beulich wrote: >>>> On 20.12.16 at 16:29, <boris.ostrovsky@oracle.com> wrote: >> On 12/20/2016 09:47 AM, Jan Beulich wrote: >>>>>> On 20.12.16 at 15:35, <boris.ostrovsky@oracle.com> wrote: >>>> On 12/20/2016 06:50 AM, Jan Beulich wrote: >>>>>> + else >>>>>> + { >>>>>> + uint32_t v = *val; >>>>>> + >>>>>> + /* Status register is write-1-to-clear by guests */ >>>>>> + switch ( port & 3 ) >>>>>> + { >>>>>> + case 0: >>>>>> + *sts &= ~(v & 0xff); >>>>>> + *sts &= *mask_sts; >>>>> I can understand the first &=, but why would a read have this second >>>>> (side) effect? I could see some sort of need for such only when you >>>>> were setting any flags. >>>> This is a write, not a read. >>> Oh, right. But the question remains about that unexpected side >>> effect. >> It indeed doesn't do anything for the case of guest access. It is >> guarding against setting unauthorized bits by domctl (introduced by the >> next patch). And I can move it into that case. >> >> However, as discussed in the thread about 06/13 patch, we may currently >> not need to provide domctl access to anything but VCPU map. So the >> question is whether domctl interface to GPE0/PM1a is needed at all. I >> think it's a useful interface with very little extra code required and >> the toolstack can use it, for example, to inject events into guests. But >> I don't have a specific use case. > To be honest, I'd keep out anything we don't really need. I have two specific use-cases. 1) DIMM Hotplug, and 2) Windows tablet/desktop mode switch, both of which I think will just require a toolstack entity to be able to raise an SCI. However, I am happy for you to ignore these case for now (as they haven't yet been fully designed and thought through), on the assumption that if we do need to add anything, it will happily sit alongside the VCPU code. ~Andrew
On 12/20/2016 11:46 AM, Andrew Cooper wrote: > On 20/12/2016 15:41, Jan Beulich wrote: >>>>> On 20.12.16 at 16:29, <boris.ostrovsky@oracle.com> wrote: >>> On 12/20/2016 09:47 AM, Jan Beulich wrote: >>>>>>> On 20.12.16 at 15:35, <boris.ostrovsky@oracle.com> wrote: >>>>> On 12/20/2016 06:50 AM, Jan Beulich wrote: >>>>>>> + else >>>>>>> + { >>>>>>> + uint32_t v = *val; >>>>>>> + >>>>>>> + /* Status register is write-1-to-clear by guests */ >>>>>>> + switch ( port & 3 ) >>>>>>> + { >>>>>>> + case 0: >>>>>>> + *sts &= ~(v & 0xff); >>>>>>> + *sts &= *mask_sts; >>>>>> I can understand the first &=, but why would a read have this second >>>>>> (side) effect? I could see some sort of need for such only when you >>>>>> were setting any flags. >>>>> This is a write, not a read. >>>> Oh, right. But the question remains about that unexpected side >>>> effect. >>> It indeed doesn't do anything for the case of guest access. It is >>> guarding against setting unauthorized bits by domctl (introduced by the >>> next patch). And I can move it into that case. >>> >>> However, as discussed in the thread about 06/13 patch, we may currently >>> not need to provide domctl access to anything but VCPU map. So the >>> question is whether domctl interface to GPE0/PM1a is needed at all. I >>> think it's a useful interface with very little extra code required and >>> the toolstack can use it, for example, to inject events into guests. But >>> I don't have a specific use case. >> To be honest, I'd keep out anything we don't really need. > I have two specific use-cases. 1) DIMM Hotplug, and 2) Windows > tablet/desktop mode switch, both of which I think will just require a > toolstack entity to be able to raise an SCI. And I just found these two: XEN_DOMCTL_SENDTRIGGER_POWER and XEN_DOMCTL_SENDTRIGGER_POWER. When we switch HVM to new interface these can be re-implemented on top of ACPI access. -boris > > However, I am happy for you to ignore these case for now (as they > haven't yet been fully designed and thought through), on the assumption > that if we do need to add anything, it will happily sit alongside the > VCPU code. > > ~Andrew
diff --git a/xen/arch/x86/hvm/acpi.c b/xen/arch/x86/hvm/acpi.c index 9b2885e..b2299a4 100644 --- a/xen/arch/x86/hvm/acpi.c +++ b/xen/arch/x86/hvm/acpi.c @@ -2,12 +2,129 @@ * * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. */ +#include <xen/acpi.h> #include <xen/errno.h> #include <xen/lib.h> #include <xen/sched.h> #include <public/arch-x86/xen.h> +static int acpi_cpumap_access_common(struct domain *d, + int dir, unsigned int port, + unsigned int bytes, uint32_t *val) +{ + unsigned int first_byte = port - XEN_ACPI_CPU_MAP; + + BUILD_BUG_ON(XEN_ACPI_CPU_MAP + XEN_ACPI_CPU_MAP_LEN + >= ACPI_GPE0_BLK_ADDRESS_V1); + + if ( dir == XEN_DOMCTL_ACPI_READ ) + { + uint32_t mask = (bytes < 4) ? ~0U << (bytes * 8) : 0; + + /* + * Clear bits that we are about to read to in case we + * copy fewer than @bytes. + */ + *val &= mask; + + if ( ((d->max_vcpus + 7) / 8) > first_byte ) + memcpy(val, (uint8_t *)d->avail_vcpus + first_byte, + min(bytes, ((d->max_vcpus + 7) / 8) - first_byte)); + } + else + /* Guests do not write CPU map */ + return X86EMUL_UNHANDLEABLE; + + return X86EMUL_OKAY; +} + +static int acpi_access_common(struct domain *d, + int dir, unsigned int port, + unsigned int bytes, uint32_t *val) +{ + uint16_t *sts = NULL, *en = NULL; + const uint16_t *mask_sts = NULL, *mask_en = NULL; + static const uint16_t pm1a_sts_mask = ACPI_BITMASK_GLOBAL_LOCK_STATUS; + static const uint16_t pm1a_en_mask = ACPI_BITMASK_GLOBAL_LOCK_ENABLE; + static const uint16_t gpe0_sts_mask = 1U << XEN_ACPI_GPE0_CPUHP_BIT; + static const uint16_t gpe0_en_mask = 1U << XEN_ACPI_GPE0_CPUHP_BIT; + + ASSERT(!has_acpi_dm_ff(d)); + + switch ( port ) + { + case ACPI_PM1A_EVT_BLK_ADDRESS_V1 ... + ACPI_PM1A_EVT_BLK_ADDRESS_V1 + + sizeof(d->arch.hvm_domain.acpi.pm1a_sts) + + sizeof(d->arch.hvm_domain.acpi.pm1a_en): + + sts = &d->arch.hvm_domain.acpi.pm1a_sts; + en = &d->arch.hvm_domain.acpi.pm1a_en; + mask_sts = &pm1a_sts_mask; + mask_en = &pm1a_en_mask; + break; + + case ACPI_GPE0_BLK_ADDRESS_V1 ... + ACPI_GPE0_BLK_ADDRESS_V1 + + sizeof(d->arch.hvm_domain.acpi.gpe0_sts) + + sizeof(d->arch.hvm_domain.acpi.gpe0_en): + + sts = &d->arch.hvm_domain.acpi.gpe0_sts; + en = &d->arch.hvm_domain.acpi.gpe0_en; + mask_sts = &gpe0_sts_mask; + mask_en = &gpe0_en_mask; + break; + + default: + return X86EMUL_UNHANDLEABLE; + } + + if ( dir == XEN_DOMCTL_ACPI_READ ) + { + uint32_t mask = (bytes < 4) ? ~0U << (bytes * 8) : 0; + uint32_t data = (((uint32_t)*en) << 16) | *sts; + + data >>= 8 * (port & 3); + *val = (*val & mask) | (data & ~mask); + } + else + { + uint32_t v = *val; + + /* Status register is write-1-to-clear by guests */ + switch ( port & 3 ) + { + case 0: + *sts &= ~(v & 0xff); + *sts &= *mask_sts; + if ( !--bytes ) + break; + v >>= 8; + /* fallthrough */ + case 1: + *sts &= ~((v & 0xff) << 8); + *sts &= *mask_sts; + if ( !--bytes ) + break; + v >>= 8; + /* fallthrough */ + case 2: + *en = ((*en & 0xff00) | (v & 0xff)) & *mask_en; + if ( !--bytes ) + break; + v >>= 8; + /* fallthrough */ + case 3: + *en = (((v & 0xff) << 8) | (*en & 0xff)) & *mask_en; + break; + } + } + + return X86EMUL_OKAY; +} + + int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, const xen_acpi_access_t *access, XEN_GUEST_HANDLE_PARAM(void) arg) @@ -18,13 +135,19 @@ int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, static int acpi_guest_access(int dir, unsigned int port, unsigned int bytes, uint32_t *val) { - return X86EMUL_UNHANDLEABLE; + return acpi_access_common(current->domain, + (dir == IOREQ_READ) ? + XEN_DOMCTL_ACPI_READ: XEN_DOMCTL_ACPI_WRITE, + port, bytes, val); } static int acpi_cpumap_guest_access(int dir, unsigned int port, unsigned int bytes, uint32_t *val) { - return X86EMUL_UNHANDLEABLE; + return acpi_cpumap_access_common(current->domain, + (dir == IOREQ_READ) ? + XEN_DOMCTL_ACPI_READ: XEN_DOMCTL_ACPI_WRITE, + port, bytes, val); } void hvm_acpi_init(struct domain *d) diff --git a/xen/common/domain.c b/xen/common/domain.c index 3abaca9..cb8df09 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -847,6 +847,7 @@ static void complete_domain_destroy(struct rcu_head *head) xsm_free_security_domain(d); free_cpumask_var(d->domain_dirty_cpumask); xfree(d->vcpu); + xfree(d->avail_vcpus); free_domain_struct(d); send_global_virq(VIRQ_DOM_EXC); diff --git a/xen/common/domctl.c b/xen/common/domctl.c index b0ee961..0a08b83 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -651,6 +651,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) goto maxvcpu_out; } + d->avail_vcpus = xzalloc_array(unsigned long, + BITS_TO_LONGS(d->max_vcpus)); + if ( !d->avail_vcpus ) + goto maxvcpu_out; + ret = 0; maxvcpu_out: diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 063efe6..bee190f 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -315,6 +315,9 @@ struct domain unsigned int max_vcpus; struct vcpu **vcpu; + /* Bitmap of available VCPUs. */ + unsigned long *avail_vcpus; + shared_info_t *shared_info; /* shared data area */ spinlock_t domain_lock;
Subsequent domctl access to ACPI registers and VCPU map will use the same code. We create acpi_[cpumap_]access_common() routines in anticipation of these changes. Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- Changes in v5: * Code movement due to changes in patch 4 * Added fallthough switch statemnt comments * Free d->avail_vcpus xen/arch/x86/hvm/acpi.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++- xen/common/domain.c | 1 + xen/common/domctl.c | 5 ++ xen/include/xen/sched.h | 3 ++ 4 files changed, 134 insertions(+), 2 deletions(-)