Re: [PATCH v3 32/32] libxl: allow the creation of HVM domains without a device model.

["Roger Pau Monné" <roger.pau@citrix.com>]

El 28/07/15 a les 13.22, Wei Liu ha escrit:
> On Fri, Jul 03, 2015 at 01:35:10PM +0200, Roger Pau Monne wrote:
>> Replace the firmware loaded into HVM guests with an OS kernel. Since the HVM
>> builder now uses the PV xc_dom_* set of functions this kernel will be parsed
>> and loaded inside the guest like on PV, but the container is a pure HVM
>> guest.
>>
>> Also, if device_model_version is set to none unconditinally set the nic type
>> to LIBXL_NIC_TYPE_VIF.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  docs/man/xl.cfg.pod.5       |  5 ++++
>>  tools/libxl/libxl.c         |  7 +++---
>>  tools/libxl/libxl_create.c  | 18 ++++++++++++++
>>  tools/libxl/libxl_dom.c     | 60 ++++++++++++++++++++++++++++++---------------
>>  tools/libxl/libxl_types.idl |  1 +
>>  tools/libxl/libxl_x86.c     |  4 ++-
>>  tools/libxl/xl_cmdimpl.c    |  2 ++
>>  7 files changed, 72 insertions(+), 25 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index a3e0e2e..1270605 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -1631,6 +1631,11 @@ This device-model is the default for Linux dom0.
>>  Use the device-model based upon the historical Xen fork of Qemu.
>>  This device-model is still the default for NetBSD dom0.
>>  
>> +=item B<none>
>> +
>> +Don't use any device model. This requires a kernel capable of booting
>> +in this mode.
>> +
>>  =back
>>  
>>  It is recommended to accept the default value for new guests.  If
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index e9a2d26..a6690cf 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -1590,11 +1590,10 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
>>  
>>      switch (libxl__domain_type(gc, domid)) {
>>      case LIBXL_DOMAIN_TYPE_HVM:
>> -        if (!libxl_get_stubdom_id(CTX, domid))
>> -            dm_present = 1;
>> -        else
>> +        if (libxl_get_stubdom_id(CTX, domid)) {
>>              dm_present = 0;
>> -        break;
>> +            break;
>> +        }
> 
> There is a path that dm_present contains garbage.

Please take into account that I've removed the last break in
LIBXL_DOMAIN_TYPE_HVM, so now the LIBXL_DOMAIN_TYPE_HVM case expands
into the LIBXL_DOMAIN_TYPE_PV case.

>>      case LIBXL_DOMAIN_TYPE_PV:
>>          pid = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "/local/domain/%d/image/device-model-pid", domid));
>>          dm_present = (pid != NULL);

dm_present is set here for both PV and HVM guests without a stubdom.

>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index f366a09..1444e40 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -164,6 +164,8 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>                  b_info->u.hvm.bios = LIBXL_BIOS_TYPE_ROMBIOS; break;
>>              case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
>>                  b_info->u.hvm.bios = LIBXL_BIOS_TYPE_SEABIOS; break;
>> +            case LIBXL_DEVICE_MODEL_VERSION_NONE:
>> +                break;
>>              default:return ERROR_INVAL;
>>              }
>>  
>> @@ -177,6 +179,8 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>              if (b_info->u.hvm.bios == LIBXL_BIOS_TYPE_ROMBIOS)
>>                  return ERROR_INVAL;
>>              break;
>> +        case LIBXL_DEVICE_MODEL_VERSION_NONE:
>> +            break;
>>          default:abort();
>>          }
>>  
>> @@ -278,6 +282,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>                  break;
>>              }
>>              break;
>> +        case LIBXL_DEVICE_MODEL_VERSION_NONE:
>> +            b_info->video_memkb = 0;
>> +            break;
>>          case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
>>          default:
>>              switch (b_info->u.hvm.vga.kind) {
>> @@ -945,6 +952,11 @@ static void initiate_domain_create(libxl__egc *egc,
>>          ret = libxl__device_nic_setdefault(gc, &d_config->nics[i], domid);
>>          if (ret) goto error_out;
>>  
>> +        /* HVM guests without a device model only have PV nics. */
>> +        if (d_config->b_info.device_model_version ==
>> +            LIBXL_DEVICE_MODEL_VERSION_NONE)
>> +            d_config->nics[i].nictype = LIBXL_NIC_TYPE_VIF;
>> +
> 
> Better to check if users have asked for emulated NIC and bail.

IMHO, I think it would be better to move this to
libxl__device_nic_setdefault and I will do so in the next version.

>>          if (d_config->nics[i].devid > last_devid)
>>              last_devid = d_config->nics[i].devid;
>>      }
>> @@ -1293,6 +1305,12 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
>>          libxl__device_console_add(gc, domid, &console, state, &device);
>>          libxl__device_console_dispose(&console);
>>  
>> +        if (d_config->b_info.device_model_version ==
>> +            LIBXL_DEVICE_MODEL_VERSION_NONE) {
>> +            domcreate_devmodel_started(egc, &dcs->dmss.dm, 0);
>> +            return;
>> +        }
>> +
>>          libxl_device_vkb_init(&vkb);
>>          libxl__device_vkb_add(gc, domid, &vkb);
>>          libxl_device_vkb_dispose(&vkb);
>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> index 6ce4115..59854a1 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
>> @@ -789,21 +789,23 @@ static int hvm_build_set_params(xc_interface *handle, uint32_t domid,
>>      uint64_t str_mfn, cons_mfn;
>>      int i;
>>  
>> -    va_map = xc_map_foreign_range(handle, domid,
>> -                                  XC_PAGE_SIZE, PROT_READ | PROT_WRITE,
>> -                                  HVM_INFO_PFN);
>> -    if (va_map == NULL)
>> -        return -1;
>> -
>> -    va_hvm = (struct hvm_info_table *)(va_map + HVM_INFO_OFFSET);
>> -    va_hvm->apic_mode = libxl_defbool_val(info->u.hvm.apic);
>> -    va_hvm->nr_vcpus = info->max_vcpus;
>> -    memset(va_hvm->vcpu_online, 0, sizeof(va_hvm->vcpu_online));
>> -    memcpy(va_hvm->vcpu_online, info->avail_vcpus.map, info->avail_vcpus.size);
>> -    for (i = 0, sum = 0; i < va_hvm->length; i++)
>> -        sum += ((uint8_t *) va_hvm)[i];
>> -    va_hvm->checksum -= sum;
>> -    munmap(va_map, XC_PAGE_SIZE);
>> +    if ( info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE) {
> 
> Stray space.
> 
>> +        va_map = xc_map_foreign_range(handle, domid,
>> +                                      XC_PAGE_SIZE, PROT_READ | PROT_WRITE,
>> +                                      HVM_INFO_PFN);
>> +        if (va_map == NULL)
>> +            return -1;
>> +
>> +        va_hvm = (struct hvm_info_table *)(va_map + HVM_INFO_OFFSET);
>> +        va_hvm->apic_mode = libxl_defbool_val(info->u.hvm.apic);
>> +        va_hvm->nr_vcpus = info->max_vcpus;
>> +        memset(va_hvm->vcpu_online, 0, sizeof(va_hvm->vcpu_online));
>> +        memcpy(va_hvm->vcpu_online, info->avail_vcpus.map, info->avail_vcpus.size);
>> +        for (i = 0, sum = 0; i < va_hvm->length; i++)
>> +            sum += ((uint8_t *) va_hvm)[i];
>> +        va_hvm->checksum -= sum;
>> +        munmap(va_map, XC_PAGE_SIZE);
>> +    }
>>  
>>      xc_hvm_param_get(handle, domid, HVM_PARAM_STORE_PFN, &str_mfn);
>>      xc_hvm_param_get(handle, domid, HVM_PARAM_CONSOLE_PFN, &cons_mfn);
>> @@ -884,6 +886,12 @@ static int libxl__domain_firmware(libxl__gc *gc,
>>          case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
>>              firmware = "hvmloader";
>>              break;
>> +        case LIBXL_DEVICE_MODEL_VERSION_NONE:
>> +            if (info->kernel == NULL) {
>> +                LOG(ERROR, "no device model requested without a kernel");
>> +                return ERROR_FAIL;
> 
> goto out
> 
> Maybe fix the other return as well...

While there I've done a fixup of all the error paths in the function,
because AFAICT some of them were doing goto out without rc set.


Thanks, Roger.