| Message ID | 1481930319-4796-7-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: > @@ -32,14 +34,15 @@ static int acpi_cpumap_access_common(struct domain *d, > memcpy(val, (uint8_t *)d->avail_vcpus + first_byte, > min(bytes, ((d->max_vcpus + 7) / 8) - first_byte)); > } > - else > + else if ( !is_guest_access ) > /* Guests do not write CPU map */ > - return X86EMUL_UNHANDLEABLE; > + memcpy((uint8_t *)d->avail_vcpus + first_byte, val, > + min(bytes, ((d->max_vcpus + 7) / 8) - first_byte)); > > return X86EMUL_OKAY; > } So you're changing to return OKAY even in the guest-write case - I don't think that's what you want. > -static int acpi_access_common(struct domain *d, > +static int acpi_access_common(struct domain *d, bool is_guest_access, Why? I thought the domctl is needed only for updating the CPU map? Or maybe it would help if the patch had a non-empty description. > @@ -129,13 +138,50 @@ int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, > const xen_acpi_access_t *access, > XEN_GUEST_HANDLE_PARAM(void) arg) > { > - return -ENOSYS; > + unsigned int bytes, i; > + uint32_t val; > + uint8_t *ptr = (uint8_t *)&val; > + int rc; > + int (*do_acpi_access)(struct domain *d, bool is_guest_access, > + int dir, unsigned int port, > + unsigned int bytes, uint32_t *val); > + > + if ( has_acpi_dm_ff(d) ) > + return -EINVAL; Perhaps better EOPNOTSUPP or ENODEV? > + if ( access->space_id != XEN_ACPI_SYSTEM_IO ) > + return -EINVAL; > + > + if ( (access->address >= XEN_ACPI_CPU_MAP) && > + (access->address < XEN_ACPI_CPU_MAP + XEN_ACPI_CPU_MAP_LEN) ) > + do_acpi_access = acpi_cpumap_access_common; > + else > + do_acpi_access = acpi_access_common; > + > + for ( i = 0; i < access->width; i += sizeof(val) ) > + { > + bytes = (access->width - i > sizeof(val)) ? sizeof(val) : access->width - i; > + > + if ( (rw == XEN_DOMCTL_ACPI_WRITE) && > + copy_from_guest_offset(ptr, arg, i, bytes) ) > + return -EFAULT; > + > + rc = do_acpi_access(d, false, rw, access->address, bytes, &val); > + if ( rc ) > + return rc; > + > + if ( (rw == XEN_DOMCTL_ACPI_READ) && > + copy_to_guest_offset(arg, i, ptr, bytes) ) > + return -EFAULT; > + } I could imagine Coverity considering val potentially uninitialized with this logic, i.e. I think we'd be better off if it had an initializer. Jan
On 12/20/2016 08:24 AM, Jan Beulich wrote: > >> -static int acpi_access_common(struct domain *d, >> +static int acpi_access_common(struct domain *d, bool is_guest_access, > Why? I thought the domctl is needed only for updating the CPU > map? Or maybe it would help if the patch had a non-empty > description. domctl updates both the map and the status. I.e. in the toolstack it looks like /*Update VCPU map. */ rc = xc_acpi_iowrite(CTX->xch, domid, XEN_ACPI_CPU_MAP, cpumap->size, cpumap->map); if (!rc) { /* Send an SCI. */ uint16_t val = 1 << XEN_ACPI_GPE0_CPUHP_BIT; rc = xc_acpi_iowrite(CTX->xch, domid, ACPI_GPE0_BLK_ADDRESS_V1, sizeof(val), &val); } I'll make a note in the commit message of the fact that both are accessed. OTOH, maybe we should have an update to the map trigger the SCI and not require the toolstack to do so? -boris
>>> On 20.12.16 at 15:45, <boris.ostrovsky@oracle.com> wrote: > On 12/20/2016 08:24 AM, Jan Beulich wrote: >> >>> -static int acpi_access_common(struct domain *d, >>> +static int acpi_access_common(struct domain *d, bool is_guest_access, >> Why? I thought the domctl is needed only for updating the CPU >> map? Or maybe it would help if the patch had a non-empty >> description. > > domctl updates both the map and the status. I.e. in the toolstack it > looks like > > /*Update VCPU map. */ > rc = xc_acpi_iowrite(CTX->xch, domid, XEN_ACPI_CPU_MAP, > cpumap->size, cpumap->map); > if (!rc) { > /* Send an SCI. */ > uint16_t val = 1 << XEN_ACPI_GPE0_CPUHP_BIT; > rc = xc_acpi_iowrite(CTX->xch, domid, ACPI_GPE0_BLK_ADDRESS_V1, > sizeof(val), &val); > } > > > I'll make a note in the commit message of the fact that both are accessed. > > OTOH, maybe we should have an update to the map trigger the SCI and not > require the toolstack to do so? That would make sense, I think. Jan
diff --git a/xen/arch/x86/hvm/acpi.c b/xen/arch/x86/hvm/acpi.c index b2299a4..044699d 100644 --- a/xen/arch/x86/hvm/acpi.c +++ b/xen/arch/x86/hvm/acpi.c @@ -7,9 +7,11 @@ #include <xen/lib.h> #include <xen/sched.h> +#include <asm/guest_access.h> + #include <public/arch-x86/xen.h> -static int acpi_cpumap_access_common(struct domain *d, +static int acpi_cpumap_access_common(struct domain *d, bool is_guest_access, int dir, unsigned int port, unsigned int bytes, uint32_t *val) { @@ -32,14 +34,15 @@ static int acpi_cpumap_access_common(struct domain *d, memcpy(val, (uint8_t *)d->avail_vcpus + first_byte, min(bytes, ((d->max_vcpus + 7) / 8) - first_byte)); } - else + else if ( !is_guest_access ) /* Guests do not write CPU map */ - return X86EMUL_UNHANDLEABLE; + memcpy((uint8_t *)d->avail_vcpus + first_byte, val, + min(bytes, ((d->max_vcpus + 7) / 8) - first_byte)); return X86EMUL_OKAY; } -static int acpi_access_common(struct domain *d, +static int acpi_access_common(struct domain *d, bool is_guest_access, int dir, unsigned int port, unsigned int bytes, uint32_t *val) { @@ -96,14 +99,20 @@ static int acpi_access_common(struct domain *d, switch ( port & 3 ) { case 0: - *sts &= ~(v & 0xff); + if ( is_guest_access ) + *sts &= ~(v & 0xff); + else + *sts = (*sts & 0xff00) | (v & 0xff); *sts &= *mask_sts; if ( !--bytes ) break; v >>= 8; /* fallthrough */ case 1: - *sts &= ~((v & 0xff) << 8); + if ( is_guest_access ) + *sts &= ~((v & 0xff) << 8); + else + *sts = ((v & 0xff) << 8) | (*sts & 0xff); *sts &= *mask_sts; if ( !--bytes ) break; @@ -129,13 +138,50 @@ int hvm_acpi_domctl_access(struct domain *d, uint8_t rw, const xen_acpi_access_t *access, XEN_GUEST_HANDLE_PARAM(void) arg) { - return -ENOSYS; + unsigned int bytes, i; + uint32_t val; + uint8_t *ptr = (uint8_t *)&val; + int rc; + int (*do_acpi_access)(struct domain *d, bool is_guest_access, + int dir, unsigned int port, + unsigned int bytes, uint32_t *val); + + if ( has_acpi_dm_ff(d) ) + return -EINVAL; + + if ( access->space_id != XEN_ACPI_SYSTEM_IO ) + return -EINVAL; + + if ( (access->address >= XEN_ACPI_CPU_MAP) && + (access->address < XEN_ACPI_CPU_MAP + XEN_ACPI_CPU_MAP_LEN) ) + do_acpi_access = acpi_cpumap_access_common; + else + do_acpi_access = acpi_access_common; + + for ( i = 0; i < access->width; i += sizeof(val) ) + { + bytes = (access->width - i > sizeof(val)) ? sizeof(val) : access->width - i; + + if ( (rw == XEN_DOMCTL_ACPI_WRITE) && + copy_from_guest_offset(ptr, arg, i, bytes) ) + return -EFAULT; + + rc = do_acpi_access(d, false, rw, access->address, bytes, &val); + if ( rc ) + return rc; + + if ( (rw == XEN_DOMCTL_ACPI_READ) && + copy_to_guest_offset(arg, i, ptr, bytes) ) + return -EFAULT; + } + + return 0; } static int acpi_guest_access(int dir, unsigned int port, unsigned int bytes, uint32_t *val) { - return acpi_access_common(current->domain, + return acpi_access_common(current->domain, true, (dir == IOREQ_READ) ? XEN_DOMCTL_ACPI_READ: XEN_DOMCTL_ACPI_WRITE, port, bytes, val); @@ -144,7 +190,7 @@ static int acpi_guest_access(int dir, unsigned int port, static int acpi_cpumap_guest_access(int dir, unsigned int port, unsigned int bytes, uint32_t *val) { - return acpi_cpumap_access_common(current->domain, + return acpi_cpumap_access_common(current->domain, true, (dir == IOREQ_READ) ? XEN_DOMCTL_ACPI_READ: XEN_DOMCTL_ACPI_WRITE, port, bytes, val);
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- Changes in v5: * Code movement due to changes in patch 4 xen/arch/x86/hvm/acpi.c | 64 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 55 insertions(+), 9 deletions(-)