Re: [Xen-devel] [PATCH v3 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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