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

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

On 11/22/2016 10:01 AM, Jan Beulich wrote:

>
>> +    const static uint8_t pm1a_mask[4] = {ACPI_BITMASK_GLOBAL_LOCK_STATUS, 0,
>> +                                         ACPI_BITMASK_GLOBAL_LOCK_ENABLE, 0};
>> +    const static uint8_t gpe0_mask[4] = {1U << XEN_GPE0_CPUHP_BIT, 0,
>> +                                         1U << XEN_GPE0_CPUHP_BIT, 0};
>
> Hmm, funny, in someone else's patch I've recently seen the same.
> Can we please stick to the more standard "storage type first"
> ordering of declaration elements. After all const modifies the type,
> and hence better stays together with it.
>
> And then I'd like to have an explanation (in the commit message)
> about the choice of the values for pm1a_mask.

Sure (Lock status/enable is required)


> Plus you using
> uint8_t here is at least odd, considering that this is about registers
> consisting of two 16-bit halves. I'm not even certain the spec
> permits these to be accessed with other than the specified
> granularity.


GPE registers can be 1-byte long. And, in fact, that's how ACPICA 
accesses it.

PM1 is indeed 2-byte long. I can make a check in the switch statement 
but I think I should leave the IOREQ_WRITE handling (at the bottom of 
this message) as it is for simplicity.


>
> Or wait - the literal 4-s here look bad too. Perhaps the two should
> be combined into a variable of type
> typeof(currd->arch.hvm_domain.acpi_io), so values and masks
> really match up. Which would still seem to make it desirable for the
> parts to be of type uint16_t, if permitted by the spec.

But I then assign these masks to uint8_t mask. Wouldn't it be better to 
explicitly keep those as byte-size values? Especially given how they are 
used in IOREQ_WRITE case (below).


>> +    else
>> +    {
>> +        unsigned int idx = port & 3;
>> +        unsigned int i;
>> +        uint8_t *ptr;
>
> const
>
>> +        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;
>> +
>> +        ptr = (uint8_t *)val;
>> +        for ( i = 0; i < bytes; i++, idx++ )
>> +        {
>> +            if ( idx < 2 ) /* status, write 1 to clear. */
>> +                reg[idx] &= ~(mask[i] & ptr[i]);
>> +            else           /* enable */
>> +                reg[idx] |= (mask[i] & ptr[i]);
>
> Don't you mean mask[idx] in both cases?

Oh, right, of course.

-boris

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