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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.12.16 at 00:18, <boris.ostrovsky@xxxxxxxxxx> wrote:
> +static int acpi_cpumap_access_common(struct domain *d,
> +                                     int dir, unsigned int port,
> +                                     unsigned int bytes, uint32_t *val)
> +{
> +    unsigned int first_byte = port - XEN_ACPI_CPU_MAP;
> +
> +    BUILD_BUG_ON(XEN_ACPI_CPU_MAP + XEN_ACPI_CPU_MAP_LEN
> +                 >= ACPI_GPE0_BLK_ADDRESS_V1);

Just > afaict.

> +
> +    if ( dir == XEN_DOMCTL_ACPI_READ )
> +    {
> +        uint32_t mask = (bytes < 4) ? ~0U << (bytes * 8) : 0;
> +
> +        /*
> +         * Clear bits that we are about to read to in case we
> +         * copy fewer than @bytes.
> +         */
> +        *val &= mask;
> +
> +        if ( ((d->max_vcpus + 7) / 8) > first_byte )
> +            memcpy(val, (uint8_t *)d->avail_vcpus + first_byte,
> +                   min(bytes, ((d->max_vcpus + 7) / 8) - first_byte));
> +    }
> +    else
> +        /* Guests do not write CPU map */
> +        return X86EMUL_UNHANDLEABLE;

Perhaps make this an early bail, reducing overall indentation?

> +static int acpi_access_common(struct domain *d,
> +                              int dir, unsigned int port,
> +                              unsigned int bytes, uint32_t *val)
> +{
> +    uint16_t *sts = NULL, *en = NULL;
> +    const uint16_t *mask_sts = NULL, *mask_en = NULL;
> +    static const uint16_t pm1a_sts_mask = ACPI_BITMASK_GLOBAL_LOCK_STATUS;
> +    static const uint16_t pm1a_en_mask = ACPI_BITMASK_GLOBAL_LOCK_ENABLE;
> +    static const uint16_t gpe0_sts_mask = 1U << XEN_ACPI_GPE0_CPUHP_BIT;
> +    static const uint16_t gpe0_en_mask = 1U << XEN_ACPI_GPE0_CPUHP_BIT;
> +
> +    ASSERT(!has_acpi_dm_ff(d));
> +
> +    switch ( port )
> +    {
> +    case ACPI_PM1A_EVT_BLK_ADDRESS_V1 ...
> +        ACPI_PM1A_EVT_BLK_ADDRESS_V1 +
> +        sizeof(d->arch.hvm_domain.acpi.pm1a_sts) +
> +        sizeof(d->arch.hvm_domain.acpi.pm1a_en):
> +
> +        sts = &d->arch.hvm_domain.acpi.pm1a_sts;
> +        en = &d->arch.hvm_domain.acpi.pm1a_en;
> +        mask_sts = &pm1a_sts_mask;
> +        mask_en = &pm1a_en_mask;
> +        break;
> +
> +    case ACPI_GPE0_BLK_ADDRESS_V1 ...
> +        ACPI_GPE0_BLK_ADDRESS_V1 +
> +        sizeof(d->arch.hvm_domain.acpi.gpe0_sts) +
> +        sizeof(d->arch.hvm_domain.acpi.gpe0_en):
> +
> +        sts = &d->arch.hvm_domain.acpi.gpe0_sts;
> +        en = &d->arch.hvm_domain.acpi.gpe0_en;
> +        mask_sts = &gpe0_sts_mask;
> +        mask_en = &gpe0_en_mask;
> +        break;
> +
> +    default:
> +        return X86EMUL_UNHANDLEABLE;
> +    }
> +
> +    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.

> +            if ( !--bytes )
> +                break;
> +            v >>= 8;
> +            /* fallthrough */
> +        case 1:
> +            *sts &= ~((v & 0xff) << 8);
> +            *sts &= *mask_sts;
> +            if ( !--bytes )
> +                break;
> +            v >>= 8;
> +            /* fallthrough */
> +        case 2:
> +            *en = ((*en & 0xff00) | (v & 0xff)) & *mask_en;
> +            if ( !--bytes )
> +                break;
> +            v >>= 8;
> +            /* fallthrough */
> +        case 3:
> +            *en = (((v & 0xff) << 8) | (*en & 0xff)) & *mask_en;
> +            break;
> +        }
> +    }
> +
> +    return X86EMUL_OKAY;
> +}
> +
> +
>  int hvm_acpi_domctl_access(struct domain *d, uint8_t rw,

No double blank lines please.

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

Jan

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