Re: [PATCH 08/10] pvh/acpi: Handle ACPI accesses for PVH guests

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

>> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
>> index 171ea82..ced7c92 100644
>> --- a/xen/arch/x86/hvm/ioreq.c
>> +++ b/xen/arch/x86/hvm/ioreq.c
>> @@ -1392,6 +1392,72 @@ void hvm_ioreq_init(struct domain *d)
>>  static int acpi_ioaccess(
>>      int dir, unsigned int port, unsigned int bytes, uint32_t *val)
>>  {
>> +    unsigned int i;
>> +    unsigned int bits = bytes * 8;
>> +    uint8_t *reg = NULL;
>> +    unsigned idx = port & 3;
>> +    bool is_cpu_map = 0;
>
> Shouldn't we be using false instead of 0 now that we are using proper bool types?
>
>> +    struct domain *currd = current->domain;
>> +
>> +    BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
>> +                 (ACPI_GPE0_BLK_LEN_V1 != 4));
>> +
>> +    switch (port)
>> +    {
>> +    case ACPI_PM1A_EVT_BLK_ADDRESS_V1 ...
>> +        (ACPI_PM1A_EVT_BLK_ADDRESS_V1 + ACPI_PM1A_EVT_BLK_LEN - 1):
>> +        reg = currd->arch.hvm_domain.acpi_io.pm1a;
>> +        break;
>> +    case ACPI_GPE0_BLK_ADDRESS_V1 ...
>> +        (ACPI_GPE0_BLK_ADDRESS_V1 + ACPI_GPE0_BLK_LEN_V1 - 1):
>> +        reg = currd->arch.hvm_domain.acpi_io.gpe;
>> +        break;
>> +    case 0xaf00 ... (0xaf00 + HVM_MAX_VCPUS/8 - 1):
>> +        is_cpu_map = 1;
>
> s/1/true ?
>
>> +        break;
>> +    default:
>> +        return X86EMUL_UNHANDLEABLE;
>> +    }
>> +
>> +    if ( bytes == 0 )
>> +        return X86EMUL_OKAY;
>> +
>> +    if ( dir == IOREQ_READ )
>> +    {
>> +        *val &= ~((1U << bits) - 1);
>> +
>> +        if ( is_cpu_map )
>> +        {
>> +            unsigned first_bit, last_bit;
>
> unsigned int
>
>> +
>> +            first_bit = (port - 0xaf00) * 8;
>> +            last_bit = min(currd->arch.avail_vcpus, first_bit + bits);
>> +            for (i = first_bit; i < last_bit; i++)
>> +                *val |= (1U << (i - first_bit));
>> +        }
>> +        else
>> +            memcpy(val, &reg[idx], bytes);
>> +    }
>> +    else
>> +    {
>> +        if ( is_cpu_map )
>> +            /* CPU map should not be written. */
>> +            return X86EMUL_UNHANDLEABLE;
>> +
>> +        /* Write either status or enable reegister. */
>> +        if ( (bytes > 2) || ((bytes == 2) && (port & 1)) )
>> +            return X86EMUL_UNHANDLEABLE;
>> +
>> +        if ( idx < 2 ) /* status, write 1 to clear. */
>> +        {
>> +            reg[idx] &= ~(*val & 0xff);
>> +            if ( bytes == 2 )
>> +                reg[idx + 1] &= ~((*val >> 8) & 0xff);
>> +        }
>> +        else           /* enable */
>> +            memcpy(&reg[idx], val, bytes);
>
> idx should be strictly == 2 in the else case shouldn't it (since it = port & 3) so would it not be more efficient to use direct assignment rather than resorting to a call to memcpy?


Why do you think idx can't be 3? Reading 1 byte from index 3 should be 
possible.

-boris

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