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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 11/07/2016 10:55 AM, Konrad Rzeszutek Wilk wrote:
> On Sun, Nov 06, 2016 at 04:42:41PM -0500, Boris Ostrovsky wrote:
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>> ---
>> CC: Paul Durrant <paul.durrant@xxxxxxxxxx>
>> ---
>>  xen/arch/x86/hvm/ioreq.c | 66 
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>>
>> 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;
>> +    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):
> That may need some documentation or a #define perhaps?

This will need to get into public header
(xen/include/public/hvm/ioreq.h) since mk_dsdt.c is the other user.


>
> Also just in case somebody decided it was funny and compile Xen with
> HVM_MAX_VCPUS set to say 4, won't this go in 0xfffffff region?
>
> You may want to add a BUILD_BUG_ON for the HVM_MAX_VCPUS, like:
>
> BUILD_BUGON(HVM_MAX_VCPUS > 8)?

OK (although I'd think compiler would flag the case statment like that)


>
>> +        is_cpu_map = 1;
>> +        break;
>> +    default:
>> +        return X86EMUL_UNHANDLEABLE;
>> +    }
>> +
>> +    if ( bytes == 0 )
>> +        return X86EMUL_OKAY;
> Should you also check for other odd sizes? Say 3?

I think 3-byte read are OK, unless they are not allowed by higher-level
ioreq code.

(And writes are already not allowed)

>
>> +        if ( is_cpu_map )
>> +            /* CPU map should not be written. */
> It shouldn't? Then who updates this? Oh we do it via the the hypercall.
> You may want to mention in this function how this all is suppose to work?

This is updated by the hypervisor's ACPI access handler (or qemu for HVM
guests) and read by AML. I'll add a comment to this effect.

-boris


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