Re: [Xen-devel] [PATCH v5 05/13] pvh/acpi: Handle ACPI accesses for PVH guests

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel