[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC


  • To: Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • From: Jane Malalane <Jane.Malalane@xxxxxxxxxx>
  • Date: Wed, 2 Feb 2022 15:19:13 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=e99LSN4D6DagAsXWqsoyVLBFkSl0wPe3DNkHcfIwdgE=; b=QNNDhG1PJha1sGCd0UZrJaVFrWR1XDMkXwOUN+rjS4V/fhkC1CAhf0DeLGaRNH3cNrqpBfX0Yk4UK+h/qbo7fOwKbNDbWPwLYFbTnaUf40b/6LW2Kw5SR3es0LwYhaBnMlbiLh2vqQZKPh7WogvGlT/v9Cgjq45LnX1VEhHnPW/Z84Yptb85cV7np1jG4iWj2nzicYh9Ijy7cSFQm2YZsvwxsotPa1icvftYeSGkMAcKsaJ+S+BDeZG+l+3wQOiUZmkuSkbmpQbuJ005H/nCgOUWLZiW7sFosfJIQaE9YwwmDZLBKOYUbei5rFLYgJ93riBAQ5+A5GSGHloXNn4Nug==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=n4dzm2eYUaTWq/nPmQLbRn1/K/qVmYngpyx+23M9m8dpUgDTXqSKSHuCKGZaaKUkkuDWrhN/fTUNi/+MTrLqMyl5tVGepLE5rQ7hADjWwENgo7AsrGORVfBUFpHpfQeSiYPD3sPyfTR3LEw9FpRgLY0jS766f5I4wmh1fWC8wK1ytgtLyxoSCJKTpOu9GG5QLGcBb0QeZIRmQvyI2yXqUHEISspcROY3NI3TnVPr+z0Lqp9I46qfSow6mOXrC0tU+LV5fJAXOjKknwU9y3UgYcX8ZT4l1/d3CO+pLt0z5idMuZ+GlBDKpmPJEAnJ4xSaCHMRtv1/9aFJnkLosA5Utg==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, "Anthony Perard" <anthony.perard@xxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, "Andrew Cooper" <Andrew.Cooper3@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Julien Grall <julien@xxxxxxx>, "Stefano Stabellini" <sstabellini@xxxxxxxxxx>, Christian Lindig <christian.lindig@xxxxxxxxxx>, David Scott <dave@xxxxxxxxxx>, "Volodymyr Babchuk" <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Wed, 02 Feb 2022 15:19:31 +0000
  • Ironport-data: A9a23:dOMFiq3/hxRU+sPZv/bD5Zd3kn2cJEfYwER7XKvMYLTBsI5bp2MBz TMXXW+DMq6DYDb9L99yOovkoBkDsMPdy9ExTVdvpC1hF35El5HIVI+TRqvS04J+DSFhoGZPt Zh2hgzodZhsJpPkS5PE3oHJ9RGQ74nRLlbHILOCanAZqTNMEn9700o5w7Rh2OaEvPDia++zk YKqyyHgEAfNNw5cagr4PIra9XuDFNyr0N8plgRWicJj5TcypFFMZH4rHomjLmOQf2VhNrXSq 9Avbl2O1jixEx8FUrtJm1tgG6EAaua60QOm0hK6V0U+6/TrS+NbPqsTbZIhhUlrZzqhxc9yz dJnnLaJYjgXfafsxv5CaDxRKnQrVUFG0OevzXmXtMWSywvNcmf2wuUoB0YzVWEa0r8pWycUr 6VecW1TKEDY7w616OvTpu1EpM0lIY/ONYcWvnhmwBnSDOo8QICFSKLPjTNd9Glq2ZEVQaeAD yYfQWFqMyTRcQZpA1lUUagYjdqLvybwNAQN/Tp5ooJoujOOnWSdyoPFK8HJc9aHQcFUmEewp W/c+Wn9RBYAO7S3yyeB83+qrv/Cm2X8Qo16PIO/8vlmkViC3Fs5ARcdVUa4ifShg0v4UNVaQ 2QY8zQjhbI//0uqSp/6RRLQiGGAlg4RXZxXCeJSwBGAzO/Y7hiUAkAATyVdc5o2uckuXzso2 1SV2dTzClRHsreYVHac/be8ti6pNG4eKmpqTS0LVwwe+PH4vZo+yBnIS75e/LWd14OvX2uqm nbT8XZ41+57YdM3O7uTp0/EhjWGj7LwZTELpQ/3AHj61SZwXdvwD2C30mTz4fFFJYefa1COu nkYhsSThNwz4YGxeD+lG7tUQuzwjxqRGHiF2AM0QcF9n9i40yP7JehtDCdCyFCF2yruURvge wfttAxY//e/11P6PPYsM+pd5ynHpJUM9OgJtNiIP7KigbArLWdrGR2CgmbLhwjQfLAEy/1XB HtiWZ/E4YwmIapm1iGqYOwWzKUmwCszrUuKG8ygkEj+gOvDPSDFIVvgDLdpRrthhJ5oXS2Pq 4oPXyd040k3vBLCjtn/rtdIcAFiwYkTDpHqsc1HHtNv0SI9cFzN/8T5mOt7E6Q8xvw9vr6Ro hmVBxEEoHKi2yyvAVjaOxhLNeK0Nb4i/C1TAMDZFQvys5TVSdzxvP53mlpeVeRPydGPOtYvH qRcJpXdXq8SItkFkhxEBaTAQEVZXE3DrSqFPja/YSh5eJhlRgfT/cTjcBep/y4LZhdbf+Nly 1F5/g+EE5cFWSp4C8PaNKCmw1+r5CBPk+NuRUrYZNJUfRy0ooRtLiXwiN4xIt0NdkqflmfLi V7ODEdKv/TJrq807MLN2fKOobC2HrYsBUFdBWTas+q7bHGI4mq5zIZce++UZjSBBnjs8aCva LwNnfHxOfEKhnhQtI94H+o5xK4y/YK39bRb0h5lDDPAaFHyUuFsJXyP3M9usKxRx+AG5VvqC xzXotQDYOeHIsLoFlIVNTEJVOXb2KFGgCTW4NQ0PF7+uH198o2YXBgAJBKLkiFccud4adt33 ec7tccKwAWjkR52YM2ehyVZ+mnQfHwNV6Ir6sMTDIPx01d5z1hDZdrXCzPs4YHJYNJJaxF4L jiRjavEprJd2kudLCZjSSmThbJQ1cYUpRRH7F4ePFDYyNPKi8g+0ABV7TlqHB9eyQ9K0r4rN 2VmX6GvyX5iI9u8aBB/Yl2R
  • Ironport-hdrordr: A9a23:jwBlvKiAjM6VMb5wC11uQychD3BQX3d13DAbv31ZSRFFG/FwyP rAoB1L73PJYWgqNU3IwerwRZVpQRvnhPtICRF4B8bsYOCUghrVEGgE1/qt/9SAIVyzygc578 ldmsdFeaTN5DRB/KXHCUyDYqwdKbq8geGVbIXlvg9QpGhRAskKhWYYNu/YKDwMeOAvP+tjKH P23Lsim9PUQwVwUi3NPAhjYwGsnayoqLvWJTo9QzI34giHij2lrJTgFQKD4xsYWzRThZ8/7G nsiWXCl+WemsD+7iWZ+37Y7pxQltek4MBEHtawhs8cLSipohq0Zb5mR6aJsFkO0aKSARcR4Z vxSiUbToBOAkDqDyaISNzWqk/dOQMVmjrfIJmj8CLeSILCNWoH4oF69P1km1PimjQdVZdHof h2NiuixutqJAKFkyLn69fSURZ20kKyvHo5iOYWy2dSSI0EddZq3MciFW5uYd499RjBmcga+S hVfbThzecTdUnfY2HSv2FpztDpVnMvHg2eSkxHvsCOyTBZkH1w0kNdnaUk7zo93YN4T4MB6/ XPM6xumr0LRsgKbbhlDONERcesEGTCTR/FLWrXK1X6E6MMPW7LtvfMkf8IzfDvfIZNwIo5mZ zHXl8dvWkue1j2AcnLx5FP+gClehTKYd0s8LAo23FUgMyOeFPbC1z2dLl1qbrRnxw2OLyoZ8 qO
  • Ironport-sdr: +6F5xza2VnTPhpC6RHfoWCvtjj+ALrKImDga2vTO9Z/wNhCHEpJ0GQYbN9i6BWE62JqVU/shu5 jEKNkIgXphXEcBI68zWok/O4GVgGQJ/zbkJaqNT0JYVMI3Qt87tUC2FRSyJic3h+TFC3djFkBT 2phB8sjHlQqWZfeDc1dyGgPNouVGNH5lLCmvqVjN8vE0hOt9Y8jlSiL02/ma0l14bU5co9QARS CHMmsIfEGlUSu8XScdu+CaT2pK80Ehfek3k2Bjg/OZicGFPFyYZ9UZt9EleIoPpWcuOqtlNmGI dj+D6v+o160La1FdPOaydKsy
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYE5eVWWLwfSz8JEi35sISHqV9YKx+fmKAgAHq4YA=
  • Thread-topic: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC

On 01/02/2022 10:02, Roger Pau Monné wrote:
> On Thu, Jan 27, 2022 at 04:01:33PM +0000, Jane Malalane wrote:
>> Introduce a new per-domain creation x86 specific flag to
>> select whether hardware assisted virtualization should be used for
>> x{2}APIC.
>>
>> A per-domain option is added to xl in order to select the usage of
>> x{2}APIC hardware assisted vitualization, as well as a global
>> configuration option.
>>
>> Having all APIC interaction exit to Xen for emulation is slow and can
>> induce much overhead. Hardware can speed up x{2}APIC by running APIC
>> read/write accesses without taking a VM exit.
>>
>> Signed-off-by: Jane Malalane <jane.malalane@xxxxxxxxxx>
>> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>> CC: Juergen Gross <jgross@xxxxxxxx>
>> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> CC: George Dunlap <george.dunlap@xxxxxxxxxx>
>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>> CC: Julien Grall <julien@xxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Christian Lindig <christian.lindig@xxxxxxxxxx>
>> CC: David Scott <dave@xxxxxxxxxx>
>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
>> CC: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
>> ---
>>   docs/man/xl.cfg.5.pod.in              | 10 ++++++++
>>   docs/man/xl.conf.5.pod.in             | 12 ++++++++++
>>   tools/golang/xenlight/helpers.gen.go  | 12 ++++++++++
>>   tools/libs/light/libxl_arch.h         |  5 ++--
>>   tools/libs/light/libxl_arm.c          |  5 ++--
>>   tools/libs/light/libxl_create.c       | 21 ++++++++++-------
>>   tools/libs/light/libxl_types.idl      |  2 ++
>>   tools/libs/light/libxl_x86.c          | 43 
>> +++++++++++++++++++++++++++++++++--
>>   tools/ocaml/libs/xc/xenctrl.ml        |  2 ++
>>   tools/ocaml/libs/xc/xenctrl.mli       |  2 ++
>>   tools/xl/xl.c                         |  8 +++++++
>>   tools/xl/xl.h                         |  2 ++
>>   tools/xl/xl_parse.c                   | 14 ++++++++++++
>>   xen/arch/x86/domain.c                 | 27 +++++++++++++++++++++-
>>   xen/arch/x86/hvm/vmx/vmcs.c           |  4 ++++
>>   xen/arch/x86/hvm/vmx/vmx.c            | 13 +++++++----
>>   xen/arch/x86/include/asm/hvm/domain.h |  6 +++++
>>   xen/arch/x86/traps.c                  |  6 +++--
>>   xen/include/public/arch-x86/xen.h     |  2 ++
>>   19 files changed, 174 insertions(+), 22 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
>> index b98d161398..974fe7d2d8 100644
>> --- a/docs/man/xl.cfg.5.pod.in
>> +++ b/docs/man/xl.cfg.5.pod.in
>> @@ -1862,6 +1862,16 @@ firmware tables when using certain older guest 
>> Operating
>>   Systems. These tables have been superseded by newer constructs within
>>   the ACPI tables.
>>   
>> +=item B<assisted_xapic=BOOLEAN>
>> +B<(x86 only)> Enables or disables hardware assisted virtualization for 
>> xapic.
>> +This allows accessing APIC registers without a VM-exit.
>> +The default is settable via L<xl.conf(5)>.
>> +
>> +=item B<assisted_x2apic=BOOLEAN>
>> +B<(x86 only)> Enables or disables hardware assisted virtualization for 
>> x2apic.
>> +This allows accessing APIC registers without a VM-exit.
>> +The default is settable via L<xl.conf(5)>.
> 
> Like you do below I would capitalize xAPIC and x2APIC in the option
> text.
> 
>> +
>>   =item B<nx=BOOLEAN>
>>   
>>   B<(x86 only)> Hides or exposes the No-eXecute capability. This allows a 
>> guest
>> diff --git a/docs/man/xl.conf.5.pod.in b/docs/man/xl.conf.5.pod.in
>> index df20c08137..2d0a59d019 100644
>> --- a/docs/man/xl.conf.5.pod.in
>> +++ b/docs/man/xl.conf.5.pod.in
>> @@ -107,6 +107,18 @@ Sets the default value for the C<max_grant_version> 
>> domain config value.
>>   
>>   Default: maximum grant version supported by the hypervisor.
>>   
>> +=item B<assisted_xapic=BOOLEAN>
>> +
>> +If enabled, domains will use xAPIC hardware assisted emulation by default.
>> +
>> +Default: enabled.
>> +
>> +=item B<assisted_x2apic=BOOLEAN>
>> +
>> +If enabled, domains will use x2APIC hardware assisted emulation by default.
>> +
>> +Default: enabled.
> 
> I think for both options this should be:
> 
> Default: enabled if supported.
> 
>> +
>>   =item B<vif.default.script="PATH">
>>   
>>   Configures the default hotplug script used by virtual network devices.
>> diff --git a/tools/golang/xenlight/helpers.gen.go 
>> b/tools/golang/xenlight/helpers.gen.go
>> index dd4e6c9f14..90e7b9b205 100644
>> --- a/tools/golang/xenlight/helpers.gen.go
>> +++ b/tools/golang/xenlight/helpers.gen.go
>> @@ -636,6 +636,12 @@ x.Passthrough = Passthrough(xc.passthrough)
>>   if err := 
>> x.XendSuspendEvtchnCompat.fromC(&xc.xend_suspend_evtchn_compat);err != nil {
>>   return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
>>   }
>> +if err := x.ArchX86.AssistedXapic.fromC(&xc.arch_x86.assisted_xapic);err != 
>> nil {
>> +return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
>> +}
>> +if err := x.ArchX86.AssistedX2Apic.fromC(&xc.arch_x86.assisted_x2apic);err 
>> != nil {
>> +return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
>> +}
>>   
>>    return nil}
>>   
>> @@ -679,6 +685,12 @@ xc.passthrough = C.libxl_passthrough(x.Passthrough)
>>   if err := x.XendSuspendEvtchnCompat.toC(&xc.xend_suspend_evtchn_compat); 
>> err != nil {
>>   return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
>>   }
>> +if err := x.ArchX86.AssistedXapic.toC(&xc.arch_x86.assisted_xapic); err != 
>> nil {
>> +return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
>> +}
>> +if err := x.ArchX86.AssistedX2Apic.toC(&xc.arch_x86.assisted_x2apic); err 
>> != nil {
>> +return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
>> +}
>>   
>>    return nil
>>    }
>> diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
>> index 00cc50394d..2eaff45526 100644
>> --- a/tools/libs/light/libxl_arch.h
>> +++ b/tools/libs/light/libxl_arch.h
>> @@ -71,8 +71,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc 
>> *gc,
>>                                                  libxl_domain_create_info 
>> *c_info);
>>   
>>   _hidden
>> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> -                                              libxl_domain_build_info 
>> *b_info);
>> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> +                                             libxl_domain_build_info 
>> *b_info,
>> +                                             const libxl_physinfo 
>> *physinfo);
>>   
>>   _hidden
>>   int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 52f2545498..4d422bef96 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -1384,8 +1384,9 @@ void 
>> libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
>>       }
>>   }
>>   
>> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> -                                              libxl_domain_build_info 
>> *b_info)
>> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> +                                             libxl_domain_build_info 
>> *b_info,
>> +                                             const libxl_physinfo *physinfo)
>>   {
>>       /* ACPI is disabled by default */
>>       libxl_defbool_setdefault(&b_info->acpi, false);
>> diff --git a/tools/libs/light/libxl_create.c 
>> b/tools/libs/light/libxl_create.c
>> index d7a40d7550..2bae6fef62 100644
>> --- a/tools/libs/light/libxl_create.c
>> +++ b/tools/libs/light/libxl_create.c
>> @@ -264,7 +264,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>       if (!b_info->event_channels)
>>           b_info->event_channels = 1023;
>>   
>> -    libxl__arch_domain_build_info_setdefault(gc, b_info);
>>       libxl_defbool_setdefault(&b_info->dm_restrict, false);
>>   
>>       if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT)
>> @@ -456,15 +455,21 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>           libxl_defbool_setdefault(&b_info->nested_hvm,               false);
>>       }
>>   
>> -    if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
>> -        libxl_physinfo info;
>> +    libxl_physinfo info;
>>   
>> -        rc = libxl_get_physinfo(CTX, &info);
>> -        if (rc) {
>> -            LOG(ERROR, "failed to get hypervisor info");
>> -            return rc;
>> -        }
>> +    rc = libxl_get_physinfo(CTX, &info);
>> +    if (rc) {
>> +        LOG(ERROR, "failed to get hypervisor info");
>> +        return rc;
>> +    }
>>   
>> +    rc = libxl__arch_domain_build_info_setdefault(gc, b_info, &info);
>> +    if (rc) {
>> +        LOG(ERROR, "unable to set domain arch build info defaults");
>> +        return rc;
>> +    }
>> +
>> +    if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
>>           if (info.cap_gnttab_v2)
>>               b_info->max_grant_version = 2;
>>           else if (info.cap_gnttab_v1)
>> diff --git a/tools/libs/light/libxl_types.idl 
>> b/tools/libs/light/libxl_types.idl
>> index 42ac6c357b..db5eb0a0b3 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -648,6 +648,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>                                  ("vuart", libxl_vuart_type),
>>                                 ])),
>>       ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>> +                               ("assisted_xapic", libxl_defbool),
>> +                               ("assisted_x2apic", libxl_defbool),
>>                                 ])),
>>       # Alternate p2m is not bound to any architecture or guest type, as it 
>> is
>>       # supported by x86 HVM and ARM support is planned.
>> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
>> index 33da51fe89..b257fca756 100644
>> --- a/tools/libs/light/libxl_x86.c
>> +++ b/tools/libs/light/libxl_x86.c
>> @@ -23,6 +23,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>       if (libxl_defbool_val(d_config->b_info.arch_x86.msr_relaxed))
>>           config->arch.misc_flags |= XEN_X86_MSR_RELAXED;
>>   
>> +    if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_xapic))
>> +        config->arch.misc_flags |= XEN_X86_ASSISTED_XAPIC;
>> +
>> +    if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_x2apic))
>> +        config->arch.misc_flags |= XEN_X86_ASSISTED_X2APIC;
>> +
>>       return 0;
>>   }
>>   
>> @@ -819,11 +825,44 @@ void 
>> libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
>>   {
>>   }
>>   
>> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> -                                              libxl_domain_build_info 
>> *b_info)
>> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> +                                             libxl_domain_build_info 
>> *b_info,
>> +                                             const libxl_physinfo *physinfo)
>>   {
>> +    int rc;
>> +    bool assisted_xapic;
>> +    bool assisted_x2apic;
>> +
>>       libxl_defbool_setdefault(&b_info->acpi, true);
>>       libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
>> +
>> +    libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, false);
>> +    libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, false);
>> +
>> +    assisted_xapic = libxl_defbool_val(b_info->arch_x86.assisted_xapic);
>> +    assisted_x2apic = libxl_defbool_val(b_info->arch_x86.assisted_x2apic);
>> +
>> +    if ((assisted_xapic || assisted_x2apic) &&
>> +        b_info->type == LIBXL_DOMAIN_TYPE_PV)
>> +    {
>> +        LOG(ERROR, "Interrupt Controller Virtualization not supported for 
>> PV");
>> +        rc = ERROR_INVAL;
>> +        goto out;
>> +    }
>> +
>> +    if ((assisted_xapic && !physinfo->cap_assisted_xapic) ||
>> +         (assisted_x2apic && !physinfo->cap_assisted_x2apic))
>> +    {
>> +        LOG(ERROR, "x%sAPIC hardware supported emulation not available",
>> +            assisted_xapic && !physinfo->cap_assisted_xapic ? "" : "2");
>> +        rc =  ERROR_INVAL;
>> +        goto out;
>> +    }
> 
> I think the logic here is slightly wrong, as you are setting the
> default value of assisted_x{2}apic to false, and we would instead like
> to set it to the current value supported by the hardware in order to
> keep current behavior.
> 
> Also the options are HVM/PVH only, so having them set for PV should
> result in an error regardless of the set value, ie:
> 
> if (b_info->type == LIBXL_DOMAIN_TYPE_PV &&
>      (!libxl_defbool_is_default(&b_info->arch_x86.assisted_xapic) ||
>       !libxl_defbool_is_default(&b_info->arch_x86.assisted_x2apic)))
>       ERROR
> 
> libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic,
>                           physinfo->cap_assisted_xapic);
> libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic,
>                           physinfo->cap_assisted_x2apic);
> 
> I don't think you need the local assisted_x{2}apic variables.

Makes sense. In that case, could I instead just have this?

if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
{
     if (physinfo->cap_assisted_xapic)
         libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, true);
     if (physinfo->cap_assisted_x2apic)
         libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, true);
}

Or do i still need to also check that assisted_x{2}apic hasn't been set 
elsewhere for PV domains, in which case, I'm happy to add the code you 
proposed above with this code I have here too.
> 
>> +
>> +    rc = 0;
>> +out:
>> +    return rc;
> 
> The out label is not really needed here and makes the code longer.
> Just 'return ERROR_INVAL' in the error paths or 0 at the end of the
> function. You can then also drop the local rc variable.
> 
>> +
>>   }
>>   
>>   int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
>> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
>> index 7ce832d605..cce30d8731 100644
>> --- a/tools/ocaml/libs/xc/xenctrl.ml
>> +++ b/tools/ocaml/libs/xc/xenctrl.ml
>> @@ -50,6 +50,8 @@ type x86_arch_emulation_flags =
>>   
>>   type x86_arch_misc_flags =
>>      | X86_MSR_RELAXED
>> +    | X86_ASSISTED_XAPIC
>> +    | X86_ASSISTED_X2APIC
>>   
>>   type xen_x86_arch_domainconfig =
>>   {
>> diff --git a/tools/ocaml/libs/xc/xenctrl.mli 
>> b/tools/ocaml/libs/xc/xenctrl.mli
>> index a2b15130ee..67a22ec15c 100644
>> --- a/tools/ocaml/libs/xc/xenctrl.mli
>> +++ b/tools/ocaml/libs/xc/xenctrl.mli
>> @@ -44,6 +44,8 @@ type x86_arch_emulation_flags =
>>   
>>   type x86_arch_misc_flags =
>>     | X86_MSR_RELAXED
>> +  | X86_ASSISTED_XAPIC
>> +  | X86_ASSISTED_X2APIC
>>   
>>   type xen_x86_arch_domainconfig = {
>>     emulation_flags: x86_arch_emulation_flags list;
>> diff --git a/tools/xl/xl.c b/tools/xl/xl.c
>> index 2d1ec18ea3..b97e491c9c 100644
>> --- a/tools/xl/xl.c
>> +++ b/tools/xl/xl.c
>> @@ -57,6 +57,8 @@ int max_grant_frames = -1;
>>   int max_maptrack_frames = -1;
>>   int max_grant_version = LIBXL_MAX_GRANT_DEFAULT;
>>   libxl_domid domid_policy = INVALID_DOMID;
>> +int assisted_xapic = 0;
>> +int assisted_x2apic = 0;
> 
> This should be initialized to -1, in order to denote the values are
> unset...
> 
>>   
>>   xentoollog_level minmsglevel = minmsglevel_default;
>>   
>> @@ -201,6 +203,12 @@ static void parse_global_config(const char *configfile,
>>       if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
>>           claim_mode = l;
>>   
>> +    if (!xlu_cfg_get_long (config, "assisted_xapic", &l, 0))
>> +        assisted_xapic = l;
>> +
>> +    if (!xlu_cfg_get_long (config, "assisted_x2apic", &l, 0))
>> +        assisted_x2apic = l;
>> +
>>       xlu_cfg_replace_string (config, "remus.default.netbufscript",
>>           &default_remus_netbufscript, 0);
>>       xlu_cfg_replace_string (config, "colo.default.proxyscript",
>> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
>> index c5c4bedbdd..528deb3feb 100644
>> --- a/tools/xl/xl.h
>> +++ b/tools/xl/xl.h
>> @@ -286,6 +286,8 @@ extern libxl_bitmap global_vm_affinity_mask;
>>   extern libxl_bitmap global_hvm_affinity_mask;
>>   extern libxl_bitmap global_pv_affinity_mask;
>>   extern libxl_domid domid_policy;
>> +extern int assisted_xapic;
>> +extern int assisted_x2apic;
>>   
>>   enum output_format {
>>       OUTPUT_FORMAT_JSON,
>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>> index 117fcdcb2b..16ff9e76bc 100644
>> --- a/tools/xl/xl_parse.c
>> +++ b/tools/xl/xl_parse.c
>> @@ -1681,6 +1681,20 @@ void parse_config_data(const char *config_source,
>>           xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 
>> 0);
>>           xlu_cfg_get_defbool(config, "apic", &b_info->apic, 0);
>>   
>> +        e = xlu_cfg_get_defbool(config, "assisted_xapic",
>> +                                &b_info->arch_x86.assisted_xapic, 0);
>> +        if (e == ESRCH) /* not specified */
>> +            libxl_defbool_set(&b_info->arch_x86.assisted_xapic, 
>> assisted_xapic);
> 
> ...because here you only want to use the global values if they have
> actually been set by the user (assisted_x{2}apic != -1):
> 
> e = xlu_cfg_get_defbool(config, "assisted_xapic",
>                          &b_info->arch_x86.assisted_xapic, 0);
> if (e == ESRCH && assisted_xapic != -1) /* use global default if present */
>      libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic);
> else if (e)
>      exit(1);

Oh right I see.
> 
>> +        else if (e)
>> +            exit(1);
>> +
>> +        e = xlu_cfg_get_defbool(config, "assisted_x2apic",
>> +                                &b_info->arch_x86.assisted_x2apic, 0);
>> +        if (e == ESRCH) /* not specified */
>> +            libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, 
>> assisted_x2apic);
>> +        else if (e)
>> +            exit(1);
>> +
>>           switch (xlu_cfg_get_list(config, "viridian",
>>                                    &viridian, &num_viridian, 1))
>>           {
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index ef1812dc14..d08f51e28b 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -619,6 +619,8 @@ int arch_sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>       bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
>>       bool hap = config->flags & XEN_DOMCTL_CDF_hap;
>>       bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt;
>> +    bool assisted_xapic = config->arch.misc_flags & XEN_X86_ASSISTED_XAPIC;
>> +    bool assisted_x2apic = config->arch.misc_flags & 
>> XEN_X86_ASSISTED_X2APIC;
>>       unsigned int max_vcpus;
>>   
>>       if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
>> @@ -685,13 +687,30 @@ int arch_sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>           }
>>       }
>>   
>> -    if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED )
>> +    if ( config->arch.misc_flags & ~(XEN_X86_MSR_RELAXED |
>> +                                     XEN_X86_ASSISTED_XAPIC |
>> +                                     XEN_X86_ASSISTED_X2APIC) )
>>       {
>>           dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n",
>>                   config->arch.misc_flags);
>>           return -EINVAL;
>>       }
>>   
>> +    if ( (assisted_xapic || assisted_x2apic) && !hvm )
>> +    {
>> +        dprintk(XENLOG_INFO,
>> +                "Interrupt Controller Virtualization not supported for 
>> PV\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    if ( (assisted_xapic && !assisted_xapic_available) ||
>> +         (assisted_x2apic && !assisted_x2apic_available) )
>> +    {
>> +        dprintk(XENLOG_INFO, "x%sAPIC requested but not available\n",
> 
> This should be a little bit more concise, as Xen does always offer
> a fully software virtualized x{2}APIC.
> 
> "hardware assisted x%sAPIC requested but not available\n"
> 
> Thanks, Roger.

Thank you for your comments.

Jane.

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.