>>> On 17.12.16 at 00:18, <boris.ostrov...@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
