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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

> > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> > index 171ea82..ced7c92 100644
> > --- a/xen/arch/x86/hvm/ioreq.c
> > +++ b/xen/arch/x86/hvm/ioreq.c
> > @@ -1392,6 +1392,72 @@ void hvm_ioreq_init(struct domain *d)
> >  static int acpi_ioaccess(
> >      int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> >  {
> > +    unsigned int i;
> > +    unsigned int bits = bytes * 8;
> > +    uint8_t *reg = NULL;
> > +    unsigned idx = port & 3;
> > +    bool is_cpu_map = 0;
> Shouldn't we be using false instead of 0 now that we are using proper bool 
> types?
>
> > +    struct domain *currd = current->domain;
> > +
> > +    BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
> > +                 (ACPI_GPE0_BLK_LEN_V1 != 4));
> > +
> > +    switch (port)
> > +    {
> > +    case ACPI_PM1A_EVT_BLK_ADDRESS_V1 ...
> > +        (ACPI_PM1A_EVT_BLK_ADDRESS_V1 + ACPI_PM1A_EVT_BLK_LEN - 1):
> > +        reg = currd->arch.hvm_domain.acpi_io.pm1a;
> > +        break;
> > +    case ACPI_GPE0_BLK_ADDRESS_V1 ...
> > +        (ACPI_GPE0_BLK_ADDRESS_V1 + ACPI_GPE0_BLK_LEN_V1 - 1):
> > +        reg = currd->arch.hvm_domain.acpi_io.gpe;
> > +        break;
> > +    case 0xaf00 ... (0xaf00 + HVM_MAX_VCPUS/8 - 1):
> > +        is_cpu_map = 1;
> s/1/true ?
>
> > +        break;
> > +    default:
> > +        return X86EMUL_UNHANDLEABLE;
> > +    }
> > +
> > +    if ( bytes == 0 )
> > +        return X86EMUL_OKAY;
> > +
> > +    if ( dir == IOREQ_READ )
> > +    {
> > +        *val &= ~((1U << bits) - 1);
> > +
> > +        if ( is_cpu_map )
> > +        {
> > +            unsigned first_bit, last_bit;
> unsigned int
>
> > +
> > +            first_bit = (port - 0xaf00) * 8;
> > +            last_bit = min(currd->arch.avail_vcpus, first_bit + bits);
> > +            for (i = first_bit; i < last_bit; i++)
> > +                *val |= (1U << (i - first_bit));
> > +        }
> > +        else
> > +            memcpy(val, &reg[idx], bytes);
> > +    }
> > +    else
> > +    {
> > +        if ( is_cpu_map )
> > +            /* CPU map should not be written. */
> > +            return X86EMUL_UNHANDLEABLE;
> > +
> > +        /* Write either status or enable reegister. */
> > +        if ( (bytes > 2) || ((bytes == 2) && (port & 1)) )
> > +            return X86EMUL_UNHANDLEABLE;
> > +
> > +        if ( idx < 2 ) /* status, write 1 to clear. */
> > +        {
> > +            reg[idx] &= ~(*val & 0xff);
> > +            if ( bytes == 2 )
> > +                reg[idx + 1] &= ~((*val >> 8) & 0xff);
> > +        }
> > +        else           /* enable */
> > +            memcpy(&reg[idx], val, bytes);
> idx should be strictly == 2 in the else case shouldn't it (since it = port & 3) 
> so would it not be more efficient to use direct assignment rather than resorting to 
> a call to memcpy?

Why do you think idx can't be 3? Reading 1 byte from index 3 should be 
possible.
-boris

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