Re: [PATCH v4 12/15] x86/altp2m: Add altp2mhvm HVM domain parameter.

[George Dunlap <george.dunlap@eu.citrix.com>]

On 07/10/2015 11:12 PM, Sahita, Ravi wrote:
>> From: dunlapg@gmail.com [mailto:dunlapg@gmail.com] On Behalf Of George
>> Dunlap
>> Sent: Friday, July 10, 2015 10:32 AM
>>
>> On Fri, Jul 10, 2015 at 1:52 AM, Ed White <edmund.h.white@intel.com>
>> wrote:
>>> The altp2mhvm and nestedhvm parameters are mutually exclusive and
>>> cannot be set together.
>>>
>>> Signed-off-by: Ed White <edmund.h.white@intel.com>
>>>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> for the
>> hypervisor bits.
>>> ---
>>>  docs/man/xl.cfg.pod.5           | 12 ++++++++++++
>>>  tools/libxl/libxl.h             |  6 ++++++
>>>  tools/libxl/libxl_create.c      |  1 +
>>>  tools/libxl/libxl_dom.c         |  2 ++
>>>  tools/libxl/libxl_types.idl     |  1 +
>>>  tools/libxl/xl_cmdimpl.c        | 10 ++++++++++
>>>  xen/arch/x86/hvm/hvm.c          | 23 +++++++++++++++++++++--
>>>  xen/include/public/hvm/params.h |  5 ++++-
>>>  8 files changed, 57 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 index
>>> a3e0e2e..18afd46 100644
>>> --- a/docs/man/xl.cfg.pod.5
>>> +++ b/docs/man/xl.cfg.pod.5
>>> @@ -1035,6 +1035,18 @@ enabled by default and you should usually omit
>>> it. It may be necessary  to disable the HPET in order to improve
>>> compatibility with guest  Operating Systems (X86 only)
>>>
>>> +=item B<altp2mhvm=BOOLEAN>
>>> +
>>> +Enables or disables hvm guest access to alternate-p2m capability.
>>> +Alternate-p2m allows a guest to manage multiple p2m guest physical
>>> +"memory views" (as opposed to a single p2m). This option is disabled
>>> +by default and is available only to hvm domains.
>>> +You may want this option if you want to access-control/isolate access
>>> +to specific guest physical memory pages accessed by the guest, e.g.
>>> +for HVM domain memory introspection or for isolation/access-control
>>> +of memory between components within a single guest hvm domain.
>>> +
>>>  =item B<nestedhvm=BOOLEAN>
>>>
>>>  Enable or disables guest access to hardware virtualisation features,
>>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index
>>> a1c5d15..17222e7 100644
>>> --- a/tools/libxl/libxl.h
>>> +++ b/tools/libxl/libxl.h
>>> @@ -745,6 +745,12 @@ typedef struct libxl__ctx libxl_ctx;  #define
>>> LIBXL_HAVE_BUILDINFO_SERIAL_LIST 1
>>>
>>>  /*
>>> + * LIBXL_HAVE_ALTP2M
>>> + * If this is defined, then libxl supports alternate p2m functionality.
>>> + */
>>> +#define LIBXL_HAVE_ALTP2M 1
>>> +
>>> +/*
>>>   * LIBXL_HAVE_REMUS
>>>   * If this is defined, then libxl supports remus.
>>>   */
>>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>>> index f366a09..418deee 100644
>>> --- a/tools/libxl/libxl_create.c
>>> +++ b/tools/libxl/libxl_create.c
>>> @@ -329,6 +329,7 @@ int libxl__domain_build_info_setdefault(libxl__gc
>> *gc,
>>>          libxl_defbool_setdefault(&b_info->u.hvm.hpet,               true);
>>>          libxl_defbool_setdefault(&b_info->u.hvm.vpt_align,          true);
>>>          libxl_defbool_setdefault(&b_info->u.hvm.nested_hvm,         false);
>>> +        libxl_defbool_setdefault(&b_info->u.hvm.altp2m,             false);
>>>          libxl_defbool_setdefault(&b_info->u.hvm.usb,                false);
>>>          libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
>>>
>>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c index
>>> bdc0465..2f1200e 100644
>>> --- a/tools/libxl/libxl_dom.c
>>> +++ b/tools/libxl/libxl_dom.c
>>> @@ -300,6 +300,8 @@ static void hvm_set_conf_params(xc_interface
>> *handle, uint32_t domid,
>>>                      libxl_defbool_val(info->u.hvm.vpt_align));
>>>      xc_hvm_param_set(handle, domid, HVM_PARAM_NESTEDHVM,
>>>                      libxl_defbool_val(info->u.hvm.nested_hvm));
>>> +    xc_hvm_param_set(handle, domid, HVM_PARAM_ALTP2MHVM,
>>> +                    libxl_defbool_val(info->u.hvm.altp2m));
>>>  }
>>>
>>>  int libxl__build_pre(libxl__gc *gc, uint32_t domid, diff --git
>>> a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index
>>> e1632fa..fb641fe 100644
>>> --- a/tools/libxl/libxl_types.idl
>>> +++ b/tools/libxl/libxl_types.idl
>>> @@ -440,6 +440,7 @@ libxl_domain_build_info =
>> Struct("domain_build_info",[
>>>                                         ("mmio_hole_memkb",  MemKB),
>>>                                         ("timer_mode",       libxl_timer_mode),
>>>                                         ("nested_hvm",       libxl_defbool),
>>> +                                       ("altp2m",           libxl_defbool),
>>>                                         ("smbios_firmware",  string),
>>>                                         ("acpi_firmware",    string),
>>>                                         ("nographic",        libxl_defbool),
>>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index
>>> c858068..43cf6bf 100644
>>> --- a/tools/libxl/xl_cmdimpl.c
>>> +++ b/tools/libxl/xl_cmdimpl.c
>>> @@ -1500,6 +1500,16 @@ static void parse_config_data(const char
>>> *config_source,
>>>
>>>          xlu_cfg_get_defbool(config, "nestedhvm",
>>> &b_info->u.hvm.nested_hvm, 0);
>>>
>>> +        xlu_cfg_get_defbool(config, "altp2mhvm",
>>> + &b_info->u.hvm.altp2m, 0);
>>> +
>>> +        if (!libxl_defbool_is_default(b_info->u.hvm.nested_hvm) &&
>>> +            libxl_defbool_val(b_info->u.hvm.nested_hvm) &&
>>> +            !libxl_defbool_is_default(b_info->u.hvm.altp2m) &&
>>> +            libxl_defbool_val(b_info->u.hvm.altp2m)) {
>>> +            fprintf(stderr, "ERROR: nestedhvm and altp2mhvm cannot be used
>> together\n");
>>> +            exit(1);
>>> +        }
>>> +
>>>          xlu_cfg_replace_string(config, "smbios_firmware",
>>>                                 &b_info->u.hvm.smbios_firmware, 0);
>>>          xlu_cfg_replace_string(config, "acpi_firmware", diff --git
>>> a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index
>>> 23cd507..6e59e68 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -5750,6 +5750,7 @@ static int hvm_allow_set_param(struct domain *d,
>>>      case HVM_PARAM_VIRIDIAN:
>>>      case HVM_PARAM_IOREQ_SERVER_PFN:
>>>      case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
>>> +    case HVM_PARAM_ALTP2MHVM:
>>
>> Sorry I missed this -- when I was skimming the reviews of the previous
>> version, I assumed that when Wei asked "hvm" to be taken out because it
>> was redundant, it would include the HVM at the end of this HVM_PARAM.  It
>> seems fairly redundant to have HVM both at the beginning and the end.
>> (Note that argument doesn't apply to NESTEDHVM, because in that case, it's
>> the HVM itself which is nested.)
>>
>> (I also have an idea this may have been discussed before, but I can't find the
>> relevant conversation now, so let me know if I'm covering old
>> ground...)
> 
> 
> Wei has acked this today morning.

That means he has no objections, but it doesn't mean nobody else has
objections.  :-)

 -George