Re: [Xen-devel] [PATCH v5 06/13] x86/domctl: Handle ACPI access from domctl

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.12.16 at 00:18, <boris.ostrovsky@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel