On 12/20/2016 06:50 AM, Jan Beulich wrote:
>
>> +
>> + if ( dir == XEN_DOMCTL_ACPI_READ )
>> + {
>> + uint32_t mask = (bytes < 4) ? ~0U << (bytes * 8) : 0;
>> + uint32_t data = (((uint32_t)*en) << 16) | *sts;
> There's one pair of pointless parentheses around the cast
> expression here.
>
>> +
>> + data >>= 8 * (port & 3);
>> + *val = (*val & mask) | (data & ~mask);
>> + }
>> + else
>> + {
>> + uint32_t v = *val;
>> +
>> + /* Status register is write-1-to-clear by guests */
>> + switch ( port & 3 )
>> + {
>> + case 0:
>> + *sts &= ~(v & 0xff);
>> + *sts &= *mask_sts;
> I can understand the first &=, but why would a read have this second
> (side) effect? I could see some sort of need for such only when you
> were setting any flags.
This is a write, not a read.
>
>> @@ -18,13 +135,19 @@ int hvm_acpi_domctl_access(struct domain *d, uint8_t rw,
>> static int acpi_guest_access(int dir, unsigned int port,
>> unsigned int bytes, uint32_t *val)
>> {
>> - return X86EMUL_UNHANDLEABLE;
>> + return acpi_access_common(current->domain,
>> + (dir == IOREQ_READ) ?
>> + XEN_DOMCTL_ACPI_READ: XEN_DOMCTL_ACPI_WRITE,
>> + port, bytes, val);
>> }
> I don't think this one is meant to be used by the domctl, so I don't see
> why you need a helper here. That would also eliminate the seemingly
> unnecessary use of XEN_DOMCTL_* here (I think you already had said
> this was an oversight in an earlier reply).
Yes, XEN_DOMCTL_*is out of place here but I'd prefer to keep the helper
to isolate the sense of 'dir' as is used by portio handling
infrastructure (i.e. IOREQ_READ/WRITE) from how it is defined in the
acpi code here. Especially if, as mentioned in another thread, I use
bool is_write in common routines.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
