[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |