Re: [PATCH 07/10] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 11/07/2016 04:39 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>> Sent: 06 November 2016 21:43
>> To: xen-devel@lists.xen.org
>> Cc: jbeulich@suse.com; Andrew Cooper <Andrew.Cooper3@citrix.com>;
>> Wei Liu <wei.liu2@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger
>> Pau Monne <roger.pau@citrix.com>; Boris Ostrovsky
>> <boris.ostrovsky@oracle.com>; Paul Durrant <Paul.Durrant@citrix.com>
>> Subject: [PATCH 07/10] pvh/ioreq: Install handlers for ACPI-related PVH IO
>> accesses
>>
>> No IOREQ server installed for an HVM guest (as indicated
>> by HVM_PARAM_NR_IOREQ_SERVER_PAGES being set to zero) implies
>> a PVH guest. These guests need to handle ACPI-related IO
>> accesses.
>>
>> Logic for the handler will be provided by a later patch.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>> CC: Paul Durrant <paul.durrant@citrix.com>
>> ---
>>  tools/libxc/xc_dom_x86.c        |  3 +++
>>  xen/arch/x86/hvm/hvm.c          | 13 +++++++++----
>>  xen/arch/x86/hvm/ioreq.c        | 17 +++++++++++++++++
>>  xen/include/asm-x86/hvm/ioreq.h |  1 +
>>  4 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 7fcdee1..0017694 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -649,6 +649,9 @@ static int alloc_magic_pages_hvm(struct
>> xc_dom_image *dom)
>>          /* Limited to one module. */
>>          if ( dom->ramdisk_blob )
>>              start_info_size += sizeof(struct hvm_modlist_entry);
>> +
>> +        /* No IOREQ server for PVH guests. */
>> +        xc_hvm_param_set(xch, domid,
>> HVM_PARAM_NR_IOREQ_SERVER_PAGES, 0);
> I thought params defaulted to zero...
>
>>      }
>>      else
>>      {
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 704fd64..6f8439d 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5206,14 +5206,19 @@ static int hvmop_set_param(
>>      {
>>          unsigned int i;
>>
>> -        if ( a.value == 0 ||
>> -             a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
> ...and if they do then you should not need this change.
>
>> +        if ( a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
>>          {
>>              rc = -EINVAL;
>>              break;
>>          }
>> -        for ( i = 0; i < a.value; i++ )
>> -            set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
>> +
>> +        if ( a.value == 0 ) /* PVH guest */
>> +            acpi_ioreq_init(d);
>> +        else
>> +        {
>> +            for ( i = 0; i < a.value; i++ )
>> +                set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
>> +        }
> This looks quite wrong. Initializing the acpi io hander should be done directly in hvm_domain_initialise() IMO and not as an obscure side effect of setting a parameter to its default value.

Right, this call indeed is used to tell the hypervisor that it should
handle IO accesses itself. It was either this or adding another
emulation flag (which is what Andrew prefers).

-boris


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