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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.11.16 at 16:30, <boris.ostrovsky@xxxxxxxxxx> wrote:
> 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)

And nothing else is? And there's no other implementation
required for the lock bit?

>> 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).

Well, maybe, namely considering that the GPE and PM1a parts
would otherwise end up different, further complicating the code.

Jan


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