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

Re: [Xen-devel] [PATCH 01/14] Nested Virtualization: tools



On Tuesday 17 August 2010 09:19:20 Dong, Eddie wrote:
> > # HG changeset patch
> > # User cegger
> > # Date 1280925492 -7200
> > tools: Add nestedhvm guest config option
> >
> > diff -r 3d0c15fe28db -r b13ace9a80d8 tools/libxc/xc_cpuid_x86.c
> > --- a/tools/libxc/xc_cpuid_x86.c
> > +++ b/tools/libxc/xc_cpuid_x86.c
> > @@ -29,7 +29,7 @@
> >  #define set_bit(idx, dst)   ((dst) |= (1u << ((idx) & 31)))
> >
> >  #define DEF_MAX_BASE 0x0000000du
> > -#define DEF_MAX_EXT  0x80000008u
> > +#define DEF_MAX_EXT  0x8000000au
>
> An real Intel HW only have max leaf of 80000008, I am not sure if renaming
> it to 8000000A will cause potential issues.

The leave 8000000A is needed for SVM. Does this change cause any problems on 
Intel CPUs ?

> >  static int hypervisor_is_64bit(xc_interface *xch)
> >  {
> > @@ -77,7 +77,7 @@ static void xc_cpuid_brand_get(char *str
> >  static void amd_xc_cpuid_policy(
> >      xc_interface *xch, domid_t domid,
> >      const unsigned int *input, unsigned int *regs,
> > -    int is_pae)
> > +    int is_pae, int is_nestedhvm)
> >  {
> >      switch ( input[0] )
> >      {
> > @@ -96,6 +96,7 @@ static void amd_xc_cpuid_policy(
> >          /* Filter all other features according to a whitelist. */
> >          regs[2] &= ((is_64bit ? bitmaskof(X86_FEATURE_LAHF_LM) : 0) |
> >                      bitmaskof(X86_FEATURE_CMP_LEGACY) |
> > +                    (is_nestedhvm ? bitmaskof(X86_FEATURE_SVME) : 0)
> >
> >                      | bitmaskof(X86_FEATURE_ALTMOVCR) |
> >
> >                      bitmaskof(X86_FEATURE_ABM) |
> >                      bitmaskof(X86_FEATURE_SSE4A) |
> > @@ -120,13 +121,43 @@ static void amd_xc_cpuid_policy(
> >           */
> >          regs[2] = ((regs[2] & 0xf000u) + 1) | ((regs[2] & 0xffu) <<
> >          1) | 1u; break;
> > +
> > +    case 0x8000000a: {
> > +        uint32_t edx;
> > +
> > +        if (!is_nestedhvm) {
> > +            regs[0] = regs[1] = regs[2] = regs[3] = 0;
> > +            break;
> > +        }
> > +
> > +#define SVM_FEATURE_NPT            0x00000001
> > +#define SVM_FEATURE_LBRV           0x00000002
> > +#define SVM_FEATURE_SVML           0x00000004
> > +#define SVM_FEATURE_NRIPS          0x00000008
> > +#define SVM_FEATURE_PAUSEFILTER    0x00000400
>
> Should those MACROs go to head file?

They are only used right below and exist for readability.
Do you see whereelse they can be used?

>
> > +
> > +        /* Only passthrough SVM features which are implemented */
> > +        edx = 0;
> > +        if (regs[3] & SVM_FEATURE_NPT)
> > +            edx |= SVM_FEATURE_NPT;
> > +        if (regs[3] & SVM_FEATURE_LBRV)
> > +            edx |= SVM_FEATURE_LBRV;
> > +        if (regs[3] & SVM_FEATURE_NRIPS)
> > +            edx |= SVM_FEATURE_NRIPS;
> > +        if (regs[3] & SVM_FEATURE_PAUSEFILTER)
> > +            edx |= SVM_FEATURE_PAUSEFILTER;
> > +
> > +        regs[3] = edx;
> > +        break;
> > +    }
> > +
> >      }
> >  }
> >
> >  static void intel_xc_cpuid_policy(
> >      xc_interface *xch, domid_t domid,
> >      const unsigned int *input, unsigned int *regs,
> > -    int is_pae)
> > +    int is_pae, int is_nestedhvm)
> >  {
> >      switch ( input[0] )
> >      {
> > @@ -160,6 +191,11 @@ static void intel_xc_cpuid_policy(
> >          /* Mask AMD Number of Cores information. */
> >          regs[2] = 0;
> >          break;
> > +
> > +    case 0x8000000a:
> > +        /* Clear AMD SVM feature bits */
> > +        regs[0] = regs[1] = regs[2] = regs[3] = 0;
> > +        break;
>
> How do you expect an L1 guest running on top of virtual Intel processor
> will try to detect AMD feature (CPUID leaf 0x8000000a) when it knows
> running on top of Intel processor? Leaf 8000000a is not existed in native
> Intel processor, or do you expect to paravirtualize the L1 guest? Note:
> Intel processor detect VMX feature in CPUID.1:ECX.VMX[bit 5], and most of
> rest capability in MSRs.

Fine. I just want to make sure that the variables are initialized and don't
contain garbage.

>
> Further more, if a future Intel processor defines CPUID 8000000A, how can
> both of them be accomodated? This is one example of difficulty to support
> SVM-on-VMX, although it is not a deadset, or do you expect to modify L1
> guest for this (kind of PV solution).

If I were implementing SVM-on-VMX then I wouldn't do that here.

> >      }
> >  }
> >
> > @@ -168,12 +204,17 @@ static void xc_cpuid_hvm_policy(
> >      const unsigned int *input, unsigned int *regs)
> >  {
> >      char brand[13];
> > +    unsigned long nestedhvm;
> >      unsigned long pae;
> >      int is_pae;
> > +    int is_nestedhvm;
> >
> >      xc_get_hvm_param(xch, domid, HVM_PARAM_PAE_ENABLED, &pae);
> >      is_pae = !!pae;
> >
> > +    xc_get_hvm_param(xch, domid, HVM_PARAM_NESTEDHVM, &nestedhvm);
>
> If you insist to support cross vendor nested virtualization, I would like
> to suggest we have multiple options for configuration: VMX, SVM, or HW. VMX
> and SVM option is for what situation that the user want to enforce the
> guest VMX/SVM features regardless of underlying hardware, while HW means to
> implements same with underlying virtualization feature in guest. In this
> way, it provides room for either cross vendor nested virtualization or
> natively virtualization.

No, I don't insist on cross vendor nested virtualization. I just 
followed "pae" here. You see the analogy in the code lines?

Christoph

>
> > +    is_nestedhvm = !!nestedhvm;
> > +
> >      switch ( input[0] )
> >      {
> >      case 0x00000000:
> > @@ -259,6 +300,7 @@ static void xc_cpuid_hvm_policy(
> >      case 0x80000004: /* ... continued         */
> >      case 0x80000005: /* AMD L1 cache/TLB info (dumped by Intel
> >      policy) */ case 0x80000006: /* AMD L2/3 cache/TLB info ; Intel
> > L2 cache features */ +    case 0x8000000a: /* AMD SVM feature bits */
> >          break;
> >
> >      default:
> > @@ -268,9 +310,9 @@ static void xc_cpuid_hvm_policy(
> >
> >      xc_cpuid_brand_get(brand);
> >      if ( strstr(brand, "AMD") )
> > -        amd_xc_cpuid_policy(xch, domid, input, regs, is_pae);
> > +        amd_xc_cpuid_policy(xch, domid, input, regs, is_pae,
> >      is_nestedhvm); else
> > -        intel_xc_cpuid_policy(xch, domid, input, regs, is_pae);
> > +        intel_xc_cpuid_policy(xch, domid, input, regs, is_pae,
> > is_nestedhvm);
> >
> >  }
> >
> > diff -r 3d0c15fe28db -r b13ace9a80d8
> > tools/python/xen/xend/XendConfig.py ---
> > a/tools/python/xen/xend/XendConfig.py +++
> > b/tools/python/xen/xend/XendConfig.py @@ -185,6 +185,7 @@
> >      XENAPI_PLATFORM_CFG_TYPES = { 'vhpt': int,
> >      'guest_os_type': str,
> >      'hap': int,
> > +    'nestedhvm' : int,
> >      'xen_extended_power_mgmt': int,
> >      'pci_msitranslate': int,
> >      'pci_power_mgmt': int,
> > diff -r 3d0c15fe28db -r b13ace9a80d8
> > tools/python/xen/xend/XendConstants.py ---
> > a/tools/python/xen/xend/XendConstants.py +++
> > b/tools/python/xen/xend/XendConstants.py @@ -52,6 +52,7 @@
> >  HVM_PARAM_TIMER_MODE   = 10 HVM_PARAM_HPET_ENABLED = 11
> >  HVM_PARAM_ACPI_S_STATE = 14
> >  HVM_PARAM_VPT_ALIGN    = 16
> > +HVM_PARAM_NESTEDHVM    = 17 # x86
> >
> >  restart_modes = [
> >      "restart",
> > diff -r 3d0c15fe28db -r b13ace9a80d8
> > tools/python/xen/xend/XendDomainInfo.py ---
> > a/tools/python/xen/xend/XendDomainInfo.py +++
> > b/tools/python/xen/xend/XendDomainInfo.py @@ -2585,10 +2585,15 @@
> >              class XendDomainInfo: xc.hvm_set_param(self.domid,
> >                               HVM_PARAM_TIMER_MODE, long(timer_mode))
> >
> > -        # Set Viridian interface configuration of domain
> > -        viridian = self.info["platform"].get("viridian")
> > -        if arch.type == "x86" and hvm and viridian is not None:
> > -            xc.hvm_set_param(self.domid, HVM_PARAM_VIRIDIAN,
> > long(viridian)) +        if arch.type == "x86" and hvm:
> > +            # Set Viridian interface configuration of domain
> > +            viridian = self.info["platform"].get("viridian")
> > +            if viridian is not None:
> > +                xc.hvm_set_param(self.domid, HVM_PARAM_VIRIDIAN,
> > long(viridian)) +            # Set nestedhvm of domain
> > +            nestedhvm = self.info["platform"].get("nestedhvm")
> > +            if nestedhvm is not None:
> > +                xc.hvm_set_param(self.domid, HVM_PARAM_NESTEDHVM,
> > long(nestedhvm))
> >
> >          # If nomigrate is set, disable migration
> >          nomigrate = self.info["platform"].get("nomigrate")
> > diff -r 3d0c15fe28db -r b13ace9a80d8 tools/python/xen/xm/create.py
> > --- a/tools/python/xen/xm/create.py
> > +++ b/tools/python/xen/xm/create.py
> > @@ -633,6 +633,11 @@ gopts.var('hap', val='HAP',
> >            use="""Hap status (0=hap is disabled;
> >            1=hap is enabled.""")
> >
> > +gopts.var('nestedhvm', val='NESTEDHVM',
> > +          fn=set_int, default=0,
> > +          use="""Nested HVM status (0=Nested HVM is disabled;
> > +          1=Nested HVM is enabled.""")
> > +
> >  gopts.var('s3_integrity', val='TBOOT_MEMORY_PROTECT',
> >            fn=set_int, default=1,
> >            use="""Should domain memory integrity be verified during
> > S3? @@ -1083,7 +1088,7 @@ def configure_hvm(config_image, vals):
> >               'isa',
> >               'keymap',
> >               'localtime',
> > -             'nographic',
> > +             'nestedhvm', 'nographic',
> >               'opengl', 'oos',
> >               'pae', 'pci', 'pci_msitranslate', 'pci_power_mgmt',
> >               'rtc_timeoffset',
> > diff -r 3d0c15fe28db -r b13ace9a80d8 xen/include/public/hvm/params.h
> > --- a/xen/include/public/hvm/params.h
> > +++ b/xen/include/public/hvm/params.h
> > @@ -109,6 +109,9 @@
> >  /* Boolean: Enable aligning all periodic vpts to reduce interrupts */
> >  #define HVM_PARAM_VPT_ALIGN    16
> >
> > -#define HVM_NR_PARAMS          17
> > +/* Boolean: Enable nestedhvm */
> > +#define HVM_PARAM_NESTEDHVM    17
> > +
> > +#define HVM_NR_PARAMS          18
> >
> >  #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
>
> Thx, Eddie



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel


 


Rackspace

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