Re: [PATCH 35/41] cpu: Rework cpuUpdate

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

 




On 08/12/2016 09:33 AM, Jiri Denemark wrote:
> The reworked API is now called virCPUUpdate and it should change the
> provided CPU definition into a one which can be consumed by QEMU command

s/by/by the/

> line builder:
> 
>     - host-passthrough remains unchanged
>     - host-model is turned into custom CPU with a model and features
>       copied from host
>     - custom CPU with minimum match is converted similarly to host-model
>     - optional features are updated according to host's CPU
> 
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
>  po/POTFILES.in                                     |   1 +
>  src/cpu/cpu.c                                      |  59 ++++--
>  src/cpu/cpu.h                                      |  11 +-
>  src/cpu/cpu_arm.c                                  |  36 +++-
>  src/cpu/cpu_ppc64.c                                |  32 ++--
>  src/cpu/cpu_x86.c                                  | 212 ++++++++-------------
>  src/libvirt_private.syms                           |   2 +-
>  src/qemu/qemu_command.c                            |   2 +-
>  src/qemu/qemu_domain.c                             |   2 +-
>  src/qemu/qemu_process.c                            |   2 +-
>  tests/cputest.c                                    |  14 +-
>  .../cputestdata/x86-host+host-model-nofallback.xml |   2 +-
>  tests/cputestdata/x86-host+host-model.xml          |   2 +-
>  .../x86-host+host-passthrough-features.xml         |   4 +
>  tests/cputestdata/x86-host+host-passthrough.xml    |  19 +-
>  tests/cputestdata/x86-host+min.xml                 |  27 +--
>  tests/cputestdata/x86-host+pentium3.xml            |  39 ++--
>  tests/cputestdata/x86-host-invtsc+host-model.xml   |   2 +-
>  .../cputestdata/x86-host-passthrough-features.xml  |   4 +
>  19 files changed, 227 insertions(+), 245 deletions(-)
>  create mode 100644 tests/cputestdata/x86-host+host-passthrough-features.xml
>  create mode 100644 tests/cputestdata/x86-host-passthrough-features.xml
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 25dbc84..1469240 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -43,6 +43,7 @@ src/conf/virchrdev.c
>  src/conf/virdomainobjlist.c
>  src/conf/virsecretobj.c
>  src/cpu/cpu.c
> +src/cpu/cpu_arm.c
>  src/cpu/cpu_map.c
>  src/cpu/cpu_ppc64.c
>  src/cpu/cpu_x86.c
> diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
> index fae3885..f3eb211 100644
> --- a/src/cpu/cpu.c
> +++ b/src/cpu/cpu.c
> @@ -579,38 +579,71 @@ cpuBaseline(virCPUDefPtr *cpus,
>  
>  
>  /**
> - * cpuUpdate:
> + * virCPUUpdate:

In a way I'd expect a "virCPUUpdate" API to go in src/util/...
somewhere. Not a problem to keep it here, but I guess when I see vir
prefixed functions I think src/util/...

>   *
> - * @guest: guest CPU definition
> + * @arch: CPU architecture
> + * @guest: guest CPU definition to be updated
>   * @host: host CPU definition
>   *
>   * Updates @guest CPU definition according to @host CPU. This is required to
> - * support guest CPU definition which are relative to host CPU, such as CPUs
> - * with VIR_CPU_MODE_CUSTOM and optional features or VIR_CPU_MATCH_MINIMUM, or
> - * CPUs with non-custom mode (VIR_CPU_MODE_HOST_MODEL,
> - * VIR_CPU_MODE_HOST_PASSTHROUGH).
> + * support guest CPU definitions specified relatively to host CPU, such as
> + * CPUs with VIR_CPU_MODE_CUSTOM and optional features or
> + * VIR_CPU_MATCH_MINIMUM, or CPUs with VIR_CPU_MODE_HOST_MODEL.
> + * When the guest CPU was not specified relatively, the function does nothing
> + * and returns success.
>   *
>   * Returns 0 on success, -1 on error.
>   */
>  int
> -cpuUpdate(virCPUDefPtr guest,
> -          const virCPUDef *host)
> +virCPUUpdate(virArch arch,
> +             virCPUDefPtr guest,
> +             const virCPUDef *host)

Still not 100% clear whether if eventually (patch 40) it's possible to
enter here with 'host == NULL'... Keeping in mind from patch 26 that
virQEMUCapsInitHostCPUModel could have qemuCaps->cpuModel = NULL, thus
the virQEMUCapsGetHostModel could then return a NULL from
qemuProcessUpdateGuestCPU which calls this function. However, that
possibility is gated by whether virQEMUCapsIsCPUModeSupported succeeds
or not. Since you've added NULLSTR and host ? checks, I'm partially
assuming it's possible to get here with host == NULL. The intro comments
here don't help me determine that though.

I'm lost in the terminology between old/new that's described in patch
40. If your belief is things are going to be OK, then I'm fine with
that, but I figured I'd at least point out what I saw and what got
confusing eventually. Hopefully it's understandable.

>  {
>      struct cpuArchDriver *driver;
>  
> -    VIR_DEBUG("guest=%p, host=%p", guest, host);
> +    VIR_DEBUG("arch=%s, guest=%p mode=%s model=%s, host=%p model=%s",
> +              virArchToString(arch), guest, virCPUModeTypeToString(guest->mode),
> +              NULLSTR(guest->model), host, NULLSTR(host ? host->model : NULL));

The Coverity build throws up here... According to cpu.h, arg 3 is
NONNULL; however, as I pointed out above that could change.

So at the very least cpu.h needs to change to remove the NONNULL(3).

>  
> -    if ((driver = cpuGetSubDriver(host->arch)) == NULL)
> +    if (!(driver = cpuGetSubDriver(arch)))
>          return -1;
>  
> -    if (driver->update == NULL) {
> +    if (guest->mode == VIR_CPU_MODE_HOST_PASSTHROUGH)
> +        return 0;
> +
> +    if (guest->mode == VIR_CPU_MODE_CUSTOM &&
> +        guest->match != VIR_CPU_MATCH_MINIMUM) {
> +        size_t i;
> +        bool optional = false;
> +
> +        for (i = 0; i < guest->nfeatures; i++) {
> +            if (guest->features[i].policy == VIR_CPU_FEATURE_OPTIONAL) {
> +                optional = true;
> +                break;
> +            }
> +        }
> +
> +        if (!optional)
> +            return 0;
> +    }
> +
> +    /* We get here if guest CPU is either
> +     *  - host-model
> +     *  - custom with minimum match
> +     *  - custom with optional features
> +     */
> +    if (!driver->update) {
>          virReportError(VIR_ERR_NO_SUPPORT,
> -                       _("cannot update guest CPU data for %s architecture"),
> +                       _("cannot update guest CPU for %s architecture"),
>                         virArchToString(host->arch));'

Should this just be 'arch' ?  if not, if host == NULL, then this will core.

>          return -1;
>      }
>  
> -    return driver->update(guest, host);
> +    if (driver->update(guest, host) < 0)

Seems as though all implemented ->update functions will return an error
if 'host' is NULL, so that's good I guess. Whether it's expected is
something you can answer!

> +        return -1;
> +
> +    VIR_DEBUG("model=%s", NULLSTR(guest->model));
> +    return 0;
>  }
>  
>  
> diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
> index 422818e..f4bb51d 100644
> --- a/src/cpu/cpu.h
> +++ b/src/cpu/cpu.h
> @@ -87,8 +87,8 @@ typedef virCPUDefPtr
>                       unsigned int flags);
>  
>  typedef int
> -(*cpuArchUpdate)    (virCPUDefPtr guest,
> -                     const virCPUDef *host);
> +(*virCPUArchUpdate)(virCPUDefPtr guest,
> +                    const virCPUDef *host);

Why the name change? I see upcoming patches also use the new
nomenclature, although I'll assume not all driver functions will change.
Is that the goal for the future?

>  
>  typedef int
>  (*cpuArchHasFeature) (const virCPUData *data,
> @@ -114,7 +114,7 @@ struct cpuArchDriver {
>      cpuArchNodeData     nodeData;
>      cpuArchGuestData    guestData;
>      cpuArchBaseline     baseline;
> -    cpuArchUpdate       update;
> +    virCPUArchUpdate    update;
>      cpuArchHasFeature    hasFeature;
>      cpuArchDataFormat   dataFormat;
>      cpuArchDataParse    dataParse;
> @@ -182,9 +182,10 @@ cpuBaseline (virCPUDefPtr *cpus,
>      ATTRIBUTE_NONNULL(1);
>  
>  int
> -cpuUpdate   (virCPUDefPtr guest,
> +virCPUUpdate(virArch arch,
> +             virCPUDefPtr guest,
>               const virCPUDef *host)
> -    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);

Remove the ATTRIBUTE_NONNULL(3) as noted above or even go back to the
earlier void function and allow it to error...

>  
>  int
>  cpuHasFeature(const virCPUData *data,

[...]

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]