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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.12.16 at 15:35, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 12/20/2016 06:50 AM, Jan Beulich wrote:
>>> +    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.

Oh, right. But the question remains about that unexpected side
effect.

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

Well, if the helper is needed for the domctl, then this is at least
acceptable (albeit personally I'd prefer the domctl handler to
do the translation, using the IOREQ_* values internally. If the
helper is not needed for domctl, then XEN_DOMCTL_* shouldn't
be passed here.

Jan


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