Re: [Xen-devel] [PATCH v6 05/12] x86/domctl: Handle ACPI access from domctl

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 07/31/2017 10:14 AM, Ross Lagerwall wrote:
> On 01/03/2017 02:04 PM, Boris Ostrovsky wrote:
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>> ---
>> Changes in v6:
>> * Adjustments to to patch 4 changes.
>> * Added a spinlock for VCPU map access
>> * Return an error on guest trying to write VCPU map
>>
> snip
>> -static int acpi_cpumap_access_common(struct domain *d, bool is_write,
>> -                                     unsigned int port,
>> +static int acpi_cpumap_access_common(struct domain *d, bool
>> is_guest_access,
>> +                                     bool is_write, unsigned int port,
>>                                        unsigned int bytes, uint32_t
>> *val)
>>   {
>>       unsigned int first_byte = port - XEN_ACPI_CPU_MAP;
>> +    int rc = X86EMUL_OKAY;
>>
>>       BUILD_BUG_ON(XEN_ACPI_CPU_MAP + XEN_ACPI_CPU_MAP_LEN
>>                    > ACPI_GPE0_BLK_ADDRESS_V1);
>>
>> +    spin_lock(&d->arch.hvm_domain.acpi_lock);
>> +
>>       if ( !is_write )
>>       {
>>           uint32_t mask = (bytes < 4) ? ~0U << (bytes * 8) : 0;
>> @@ -32,23 +37,61 @@ static int acpi_cpumap_access_common(struct
>> domain *d, bool is_write,
>>               memcpy(val, (uint8_t *)d->avail_vcpus + first_byte,
>>                      min(bytes, ((d->max_vcpus + 7) / 8) - first_byte));
>>       }
>> +    else if ( !is_guest_access )
>> +        memcpy((uint8_t *)d->avail_vcpus + first_byte, val,
>> +               min(bytes, ((d->max_vcpus + 7) / 8) - first_byte));
>>       else
>>           /* Guests do not write CPU map */
>> -        return X86EMUL_UNHANDLEABLE;
>> +        rc = X86EMUL_UNHANDLEABLE;
>>
>> -    return X86EMUL_OKAY;
>> +    spin_unlock(&d->arch.hvm_domain.acpi_lock);
>> +
>> +    return rc;
>>   }
>>
>>   int hvm_acpi_domctl_access(struct domain *d,
>>                              const struct xen_domctl_acpi_access
>> *access)
>>   {
>> -    return -ENOSYS;
>> +    unsigned int bytes, i;
>> +    uint32_t val = 0;
>> +    uint8_t *ptr = (uint8_t *)&val;
>> +    int rc;
>> +    bool is_write = (access->rw == XEN_DOMCTL_ACPI_WRITE) ? true :
>> false;
>> +
>> +    if ( has_acpi_dm_ff(d) )
>> +        return -EOPNOTSUPP;
>> +
>> +    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)) )
>> +        return -ENODEV;
>> +
>> +    for ( i = 0; i < access->width; i += sizeof(val) )
>> +    {
>> +        bytes = (access->width - i > sizeof(val)) ?
>> +            sizeof(val) : access->width - i;
>> +
>> +        if ( is_write && copy_from_guest_offset(ptr, access->val, i,
>> bytes) )
>> +            return -EFAULT;
>> +
>> +        rc = acpi_cpumap_access_common(d, false, is_write,
>> +                                       access->address, bytes, &val);
>
> While I'm looking at this code...
> This doesn't work if access->width > sizeof(val) (4 bytes). The same
> value (access->address) is always passed into
> acpi_cpumap_access_common for 'port' and this is used as an offset
> into the avail_cpus array. So the offset is unchanged and only the
> first 4 bytes of avail_cpus ever gets changed.

I'd have to go back to the series (haven't looked at it since it was
posted back in January) but I think I enforce somewhere size of the
access to fit into 4 bytes. And if not then you are right.


-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel