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

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

On 11/15/2016 04:24 AM, Jan Beulich wrote:
>>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/hvm/ioreq.c
>> +++ b/xen/arch/x86/hvm/ioreq.c
>> @@ -1383,6 +1383,78 @@ static int hvm_access_cf8(static int acpi_ioaccess(
>>      int dir, unsigned int port, unsigned int bytes, uint32_t *val)
>>  {
>> +    unsigned int i;
>> +    unsigned int bits = bytes * 8;
>> +    unsigned int idx = port & 3;
>> +    uint8_t *reg = NULL;
>> +    bool is_cpu_map = false;
>> +    struct domain *currd = current->domain;
>> +
>> +    BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
>> +                 (ACPI_GPE0_BLK_LEN_V1 != 4));
>> +
>> +    if ( has_ioreq_cpuhp(currd) )
>> +        return X86EMUL_UNHANDLEABLE;
> Hmm, so it seems you indeed mean the flag to have the inverse sense
> of what I would have expected, presumably in order for HVM guests
> to continue to have all emulation flags set. I think that's a little
> unfortunate, or at least the name of flag and predicate are somewhat
> misleading (as there's no specific CPU hotplug related ioreq).

The other option was XEN_X86_EMU_ACPI. Would it be better?

>
>> +        if ( is_cpu_map )
>> +        {
>> +            unsigned int first_bit, last_bit;
>> +
>> +            first_bit = (port - ACPI_CPU_MAP) * 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 is only read by DSDT's PRSC method and should never
>> +             * be written by a guest.
>> +             */
>> +            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);
>> +    }
> Overall - how does this implementation match up with the following
> requirements from the spec:
>
> ● Reserved or unimplemented bits always return zero (control or enable).
> ● Writes to reserved or unimplemented bits have no affect.
>
> To me it looks as it at this point all bits are reserved/unimplemented.

We do have one bit that we need --- bit 2 of GPE --- but the rest indeed
looks like it is unused. I'll check whether there are any required buts
to be supported and if not then only checking for bit 2 will make things
slightly simpler here.

-boris


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