Re: [PATCH] i386: revert defaults to 'legacy-vm-type=true' for SEV(-ES) guests

[Michael Roth via]

On Fri, Jun 14, 2024 at 11:39:24AM +0100, Daniel P. Berrangé wrote:
> The KVM_SEV_INIT2 ioctl was only introduced in Linux 6.10, which will
> only have been released for a bit over a month when QEMU 9.1 is
> released.
> 
> The SEV(-ES) support in QEMU has been present since 2.12 dating back
> to 2018. With this in mind, the overwhealming majority of users of
> SEV(-ES) are unlikely to be running Linux >= 6.10, any time in the
> forseeable future.
> 
> IOW, defaulting new QEMU to 'legacy-vm-type=false' means latest QEMU
> machine types will be broken out of the box for most SEV(-ES) users.
> Even if the kernel is new enough, it also affects the guest measurement,
> which means that their existing tools for validating measurements will
> also be broken by the new default.
> 
> This is not a sensible default choice at this point in time. Revert to
> the historical behaviour which is compatible with what most users are
> currently running.

Part of the reason for the change is that SEV-ES measurements are
already affected by some short-comings of the legacy KVM_SEV_ES_INIT
API. Namely, if the kvm_amd.debug-swap module param is used to enable
that SEV-ES feature, then that feature will get enabled on the KVM side
and change the initial guest measurement (due to VMSA_FEATURES field
of the vCPU's VMSA changing), and userspace has no way to control that
on a per-VM basis, so measurement for any particular invocation will
be somewhat random depending on the system configuration and kernel
level.

I think that's why users of newer QEMU machine types are highly
encouraged to switch to the new KVM_SEV_INIT2 interface. I do see this
causing issues for older QEMU machine types that previously relied on
the legacy interface, since we do want to avoid measurement changing
for an existing guest that was previously working on an older kernel,
which is why this flag defaults to true for pre-9.1 machine types. But
on newer kernels there is still potential for issues relating to
debug-swap (and other VMSA features that get added to KVM in the future)
and how they may cause measurement changes underneath the covers if we
don't allow userspace the ability to control what is/isn't disabled.

Because of that I think it's less headache for userspace to have to
opt-in to legacy interface when using newer machine models. It should be
a concious decision to keep using this deprecated interface with known
limitations that could affect measurement in unexpected ways.

I was actually planning to go the other direction on this because
currently for 9.1+, QEMU will try to use KVM_SEV_INIT2 if
KVM_CAP_VM_TYPES advertises its availability, but otherwise fall back to
the above KVM_SEV_ES_INIT interface and potential inherit the issues
noted above. So I was planning on getting rid of the fallback, and
basically only allowing legacy KVM_SEV_ES_INIT for 9.1+ if the user
manually sets sev_guest->legacy_vm_type via cmdline.

-Mike

> 
> This can be re-evaluated a few years down the line, though it is more
> likely that all attention will be on SEV-SNP by this time. Distro
> vendors may still choose to change this default downstream to align
> with their new major releases where they can guarantee the kernel
> will always provide the required functionality.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  hw/i386/pc.c  |  1 -
>  qapi/qom.json | 12 ++--
>  target/i386/sev.c |  7 +++
>  3 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 0469af00a7..b65843c559 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -82,7 +82,6 @@
>  GlobalProperty pc_compat_9_0[] = {
>  { TYPE_X86_CPU, "x-l1-cache-per-thread", "false" },
>  { TYPE_X86_CPU, "guest-phys-bits", "0" },
> -{ "sev-guest", "legacy-vm-type", "true" },
>  { TYPE_X86_CPU, "legacy-multi-node", "on" },
>  };
>  const size_t pc_compat_9_0_len = G_N_ELEMENTS(pc_compat_9_0);
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 8bd299265e..714ebeec8b 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -912,12 +912,12 @@
>  # @handle: SEV firmware handle (default: 0)
>  #
>  # @legacy-vm-type: Use legacy KVM_SEV_INIT KVM interface for creating the VM.
> -#  The newer KVM_SEV_INIT2 interface syncs additional vCPU
> -#  state when initializing the VMSA structures, which will
> -#  result in a different guest measurement. Set this to
> -#  maintain compatibility with older QEMU or kernel versions
> -#  that rely on legacy KVM_SEV_INIT behavior.
> -#  (default: false) (since 9.1)
> +#  The newer KVM_SEV_INIT2 interface, from Linux >= 6.10, 
> syncs
> +#  additional vCPU state when initializing the VMSA 
> structures,
> +#  which will result in a different guest measurement. Toggle
> +#  this to control compatibility with older QEMU or kernel
> +# 

Re: [PATCH v4 29/31] hw/i386/sev: Allow use of pflash in conjunction with -bios

[Michael Roth via]

On Mon, Jun 03, 2024 at 12:55:43PM +0100, Daniel P. Berrangé wrote:
> On Thu, May 30, 2024 at 06:16:41AM -0500, Pankaj Gupta wrote:
> > From: Michael Roth 
> > 
> > SEV-ES and SEV-SNP support OVMF images with non-volatile storage in
> > cases where the storage area is generated as a separate image as part
> > of the OVMF build process.
> 
> IIUC, right now all OVMF builds for SEV/-ES/-SNP should be done as so
> called "stateless" image. ie *without* any separate NVRAM image, because
> that image will not be covered by the VM boot measurement and thus the
> NVRAM state is liable to undermine  trust of the VM.

Technically both stateless and stateful are supported for SEV-ES, so
this is mainly to provide similar support for SNP. Unfortunately we
can't re-use the existing QEMU topology because of the limitations are
using read-only ROMs.

But I'm not sure it's something we have a hard requirement for, and even
then maybe there are other alternatives to consider that would offer
better security.

In any case, I think it makes total sense to decouple this from the
series and re-submit this as a separate patchset if we determine that
it's truly necessary (and if so, address Paolo's comments, and handle the
64K-alignment thing in the context of that work).

So for now maybe we should plan to drop it from qemu-coco-queue and
focus on the stateless builds for the initial code merge.

-Mike

> 
> Using NVRAM for SNP is theoretically possible in future but would be
> reliant on SVSM providing a secure encryption mechanism on the storage.
> 
> 
> 
> > 
> > Currently these are exposed with unit=0 corresponding to the actual BIOS
> > image, and unit=1 corresponding to the storage image. However, pflash
> > images are mapped guest memory using read-only memslots, which are not
> > allowed in conjunction with guest_memfd-backed ranges. This makes that
> > approach unusable for SEV-SNP, where the BIOS range will be encrypted
> > and mapped as private guest_memfd-backed memory. For this reason,
> > SEV-SNP will instead rely on -bios to handle loading the BIOS image.
> > 
> > To allow for pflash to still be used for the storage image, rework the
> > existing logic to remove assumptions that unit=0 contains the BIOS image
> > when SEV-SNP, so that it can instead be used to handle only the storage
> > image.
> 
> Mixing both BIOS and pflash is pretty undesirable, not least because
> that setup cannot be currently represented by the firmware descriptor
> format described by docs/interop/firmware.json.
> 
> So at the very least this patch is incomplete, as it would need to
> propose changes to the firmware.json to allow this setup to be expressed.
> 
> I really wish we didn't have to introduce this though - is there really
> no way to make it possible to use pflash for both CODE & VARS with SNP,
> as is done with traditional VMs, so we don't diverge in setup, needing
> yet more changes up the mgmt stack ?
> 
> > 
> > Signed-off-by: Michael Roth 
> > Signed-off-by: Pankaj Gupta 
> > ---
> >  hw/i386/pc_sysfw.c | 47 +-
> >  1 file changed, 30 insertions(+), 17 deletions(-)
> > 
> > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > index def77a442d..7f97e62b16 100644
> > --- a/hw/i386/pc_sysfw.c
> > +++ b/hw/i386/pc_sysfw.c
> > @@ -125,21 +125,10 @@ void pc_system_flash_cleanup_unused(PCMachineState 
> > *pcms)
> >  }
> >  }
> >  
> > -/*
> > - * Map the pcms->flash[] from 4GiB downward, and realize.
> > - * Map them in descending order, i.e. pcms->flash[0] at the top,
> > - * without gaps.
> > - * Stop at the first pcms->flash[0] lacking a block backend.
> > - * Set each flash's size from its block backend.  Fatal error if the
> > - * size isn't a non-zero multiple of 4KiB, or the total size exceeds
> > - * pcms->max_fw_size.
> > - *
> > - * If pcms->flash[0] has a block backend, its memory is passed to
> > - * pc_isa_bios_init().  Merging several flash devices for isa-bios is
> > - * not supported.
> > - */
> > -static void pc_system_flash_map(PCMachineState *pcms,
> > -MemoryRegion *rom_memory)
> > +static void pc_system_flash_map_partial(PCMachineState *pcms,
> > +MemoryRegion *rom_memory,
> > +hwaddr offset,
> > +bool storage_only)
> >  {
> >  X86MachineState *x86ms = X86_MACHINE(pcms);
> >  PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> > @@ -154,6 +143,8 @@ static void pc_system_flash_map(PCMachineState *pcms,
> >  
> >  assert(PC_MACHINE_GET_CLASS(pcms)->pci_enabled);
> >  
> > +total_size = offset;
> > +
> >  for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> >  hwaddr gpa;
> >  
> > @@ -192,7 +183,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
> >  sysbus_realize_and_unref(SYS_BUS_DEVICE(system_flash), 
> > _fatal);
> >  

Re: [PATCH v3 47/49] hw/i386/sev: Add support to encrypt BIOS when SEV-SNP is enabled

[Michael Roth via]

On Wed, Mar 20, 2024 at 12:22:34PM +, Daniel P. Berrangé wrote:
> On Wed, Mar 20, 2024 at 03:39:43AM -0500, Michael Roth wrote:
> > TODO: Brijesh as author, me as co-author (vice-versa depending)
> >   drop flash handling? we only support BIOS now
> 
> A reminder that this commit message needs fixing.

Sorry, definitely meant to fix this one up before submitting. I've
gone ahead and force-pushed an updated tree to same qemu-v3-rc branch.
The only change is proper attribution/commit message for this patch:

  https://github.com/AMDESE/qemu/commit/c54618a1cc23f2398e6c3af6f3cf140c4901347c

-Mike

> 
> > 
> > Signed-off-by: Michael Roth 
> > ---
> >  hw/i386/pc_sysfw.c| 12 +++-
> >  hw/i386/x86.c |  2 +-
> >  include/hw/i386/x86.h |  2 +-
> >  target/i386/sev-sysemu-stub.c |  2 +-
> >  target/i386/sev.c | 15 +++
> >  target/i386/sev.h |  2 +-
> >  6 files changed, 22 insertions(+), 13 deletions(-)
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
> 

Re: [PATCH v3 31/49] i386/sev: Update query-sev QAPI format to handle SEV-SNP

[Michael Roth via]

On Wed, Mar 20, 2024 at 12:10:04PM +, Daniel P. Berrangé wrote:
> On Wed, Mar 20, 2024 at 03:39:27AM -0500, Michael Roth wrote:
> > Most of the current 'query-sev' command is relevant to both legacy
> > SEV/SEV-ES guests and SEV-SNP guests, with 2 exceptions:
> > 
> >   - 'policy' is a 64-bit field for SEV-SNP, not 32-bit, and
> > the meaning of the bit positions has changed
> >   - 'handle' is not relevant to SEV-SNP
> > 
> > To address this, this patch adds a new 'sev-type' field that can be
> > used as a discriminator to select between SEV and SEV-SNP-specific
> > fields/formats without breaking compatibility for existing management
> > tools (so long as management tools that add support for launching
> > SEV-SNP guest update their handling of query-sev appropriately).
> > 
> > The corresponding HMP command has also been fixed up similarly.
> > 
> > Signed-off-by: Michael Roth 
> > ---
> >  qapi/misc-target.json | 71 ++-
> >  target/i386/sev.c | 50 --
> >  target/i386/sev.h |  3 ++
> >  3 files changed, 94 insertions(+), 30 deletions(-)
> > 
> > diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> > index 4e0a6492a9..daceb85d95 100644
> > --- a/qapi/misc-target.json
> > +++ b/qapi/misc-target.json
> > @@ -47,6 +47,49 @@
> > 'send-update', 'receive-update' ],
> >'if': 'TARGET_I386' }
> >  
> > +##
> > +# @SevGuestType:
> > +#
> > +# An enumeration indicating the type of SEV guest being run.
> > +#
> > +# @sev: The guest is a legacy SEV or SEV-ES guest.
> > +# @sev-snp: The guest is an SEV-SNP guest.
> > +#
> > +# Since: 6.2
> 
> Now 9.1 at the earliest.
> 
> > +##
> > +{ 'enum': 'SevGuestType',
> > +  'data': [ 'sev', 'sev-snp' ],
> > +  'if': 'TARGET_I386' }
> > +
> > +##
> > +# @SevGuestInfo:
> > +#
> > +# Information specific to legacy SEV/SEV-ES guests.
> > +#
> > +# @policy: SEV policy value
> > +#
> > +# @handle: SEV firmware handle
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'struct': 'SevGuestInfo',
> > +  'data': { 'policy': 'uint32',
> > +'handle': 'uint32' },
> > +  'if': 'TARGET_I386' }
> > +
> > +##
> > +# @SevSnpGuestInfo:
> > +#
> > +# Information specific to SEV-SNP guests.
> > +#
> > +# @snp-policy: SEV-SNP policy value
> > +#
> > +# Since: 6.2
> > +##
> > +{ 'struct': 'SevSnpGuestInfo',
> > +  'data': { 'snp-policy': 'uint64' },
> > +  'if': 'TARGET_I386' }
> 
> IMHO it can just be called 'policy' still, since
> it is implicitly within a 'Snp' specific type.
> 
> 
> > +
> >  ##
> >  # @SevInfo:
> >  #
> > @@ -60,25 +103,25 @@
> >  #
> >  # @build-id: SEV FW build id
> >  #
> > -# @policy: SEV policy value
> > -#
> >  # @state: SEV guest state
> >  #
> > -# @handle: SEV firmware handle
> > +# @sev-type: Type of SEV guest being run
> >  #
> >  # Since: 2.12
> >  ##
> > -{ 'struct': 'SevInfo',
> > -'data': { 'enabled': 'bool',
> > -  'api-major': 'uint8',
> > -  'api-minor' : 'uint8',
> > -  'build-id' : 'uint8',
> > -  'policy' : 'uint32',
> > -  'state' : 'SevState',
> > -  'handle' : 'uint32'
> > -},
> > -  'if': 'TARGET_I386'
> > -}
> > +{ 'union': 'SevInfo',
> > +  'base': { 'enabled': 'bool',
> > +'api-major': 'uint8',
> > +'api-minor' : 'uint8',
> > +'build-id' : 'uint8',
> > +'state' : 'SevState',
> > +'sev-type' : 'SevGuestType' },
> > +  'discriminator': 'sev-type',
> > +  'data': {
> > +  'sev': 'SevGuestInfo',
> > +  'sev-snp': 'SevSnpGuestInfo' },
> > +  'if': 'TARGET_I386' }
> > +
> >  
> >  ##
> >  # @query-sev:
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 43e6c0172f..b03d70a3d1 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -353,25 +353,27 @@ static SevInfo *sev_get_info(void)
> >  {
> >  SevInfo *info;
> >  SevCommonState *sev_common = 
> > SEV_COMMON(MACHINE(qdev_get_machine())->cgs);
> > -SevGuestState *sev_guest =
> > -(SevGuestState *)object_dynamic_cast(OBJECT(sev_common),
> > - TYPE_SEV_GUEST);
> >  
> >  info = g_new0(SevInfo, 1);
> >  info->enabled = sev_enabled();
> >  
> >  if (info->enabled) {
> > -if (sev_guest) {
> > -info->handle = sev_guest->handle;
> > -}
> >  info->api_major = sev_common->api_major;
> >  info->api_minor = sev_common->api_minor;
> >  info->build_id = sev_common->build_id;
> >  info->state = sev_common->state;
> > -/* we only report the lower 32-bits of policy for SNP, ok for 
> > now... */
> > -info->policy =
> > -(uint32_t)object_property_get_uint(OBJECT(sev_common),
> > -   "policy", NULL);
> > +
> > +if (sev_snp_enabled()) {
> > +info->sev_type = SEV_GUEST_TYPE_SEV_SNP;
> > +

Re: [PATCH v3 23/49] i386/sev: Add a sev_snp_enabled() helper

[Michael Roth via]

On Wed, Mar 20, 2024 at 12:35:09PM +, Daniel P. Berrangé wrote:
> On Wed, Mar 20, 2024 at 03:39:19AM -0500, Michael Roth wrote:
> > Add a simple helper to check if the current guest type is SNP. Also have
> > SNP-enabled imply that SEV-ES is enabled as well, and fix up any places
> > where the sev_es_enabled() check is expecting a pure/non-SNP guest.
> > 
> > Signed-off-by: Michael Roth 
> > ---
> >  target/i386/sev.c | 13 -
> >  target/i386/sev.h |  2 ++
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 7e6dab642a..2eb13ba639 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> 
> 
> > @@ -933,7 +942,9 @@ static int sev_kvm_init(ConfidentialGuestSupport *cgs, 
> > Error **errp)
> >   __func__);
> >  goto err;
> >  }
> > +}
> >  
> > +if (sev_es_enabled() && !sev_snp_enabled()) {
> >  if (!(status.flags & SEV_STATUS_FLAGS_CONFIG_ES)) {
> >  error_report("%s: guest policy requires SEV-ES, but "
> >   "host SEV-ES support unavailable",
> 
> Opps, pre-existing bug here - this method has an 'Error **errp'
> parameter, so should be using 'error_report'.
> 
> There are several more examples of this in this method that
> predate your patch series.  Can you put a patch at the start
> of this series that fixes them before introducing SNP.

Sure, will add a pre-patch to fix up all the pre-existing issues
you've noted.

-Mike

> 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
> 

Re: [PATCH v3 22/49] i386/sev: Introduce 'sev-snp-guest' object

[Michael Roth via]

On Wed, Mar 20, 2024 at 11:58:57AM +, Daniel P. Berrangé wrote:
> On Wed, Mar 20, 2024 at 03:39:18AM -0500, Michael Roth wrote:
> > From: Brijesh Singh 
> > 
> > SEV-SNP support relies on a different set of properties/state than the
> > existing 'sev-guest' object. This patch introduces the 'sev-snp-guest'
> > object, which can be used to configure an SEV-SNP guest. For example,
> > a default-configured SEV-SNP guest with no additional information
> > passed in for use with attestation:
> > 
> >   -object sev-snp-guest,id=sev0
> > 
> > or a fully-specified SEV-SNP guest where all spec-defined binary
> > blobs are passed in as base64-encoded strings:
> > 
> >   -object sev-snp-guest,id=sev0, \
> > policy=0x3, \
> > init-flags=0, \
> > id-block=YWFhYWFhYWFhYWFhYWFhCg==, \
> > id-auth=CxHK/OKLkXGn/KpAC7Wl1FSiisWDbGTEKz..., \
> > auth-key-enabled=on, \
> > host-data=LNkCWBRC5CcdGXirbNUV1OrsR28s..., \
> > guest-visible-workarounds=AA==, \
> > 
> > See the QAPI schema updates included in this patch for more usage
> > details.
> > 
> > In some cases these blobs may be up to 4096 characters, but this is
> > generally well below the default limit for linux hosts where
> > command-line sizes are defined by the sysconf-configurable ARG_MAX
> > value, which defaults to 2097152 characters for Ubuntu hosts, for
> > example.
> > 
> > Signed-off-by: Brijesh Singh 
> > Co-developed-by: Michael Roth 
> > Acked-by: Markus Armbruster  (for QAPI schema)
> > Signed-off-by: Michael Roth 
> > ---
> >  docs/system/i386/amd-memory-encryption.rst |  78 ++-
> >  qapi/qom.json  |  51 +
> >  target/i386/sev.c  | 241 +
> >  target/i386/sev.h  |   1 +
> >  4 files changed, 369 insertions(+), 2 deletions(-)
> > 
> 
> > +##
> > +# @SevSnpGuestProperties:
> > +#
> > +# Properties for sev-snp-guest objects. Most of these are direct arguments
> > +# for the KVM_SNP_* interfaces documented in the linux kernel source
> > +# under Documentation/virt/kvm/amd-memory-encryption.rst, which are in
> > +# turn closely coupled with the SNP_INIT/SNP_LAUNCH_* firmware commands
> > +# documented in the SEV-SNP Firmware ABI Specification (Rev 0.9).
> > +#
> > +# More usage information is also available in the QEMU source tree under
> > +# docs/amd-memory-encryption.
> > +#
> > +# @policy: the 'POLICY' parameter to the SNP_LAUNCH_START command, as
> > +#  defined in the SEV-SNP firmware ABI (default: 0x3)
> > +#
> > +# @guest-visible-workarounds: 16-byte, base64-encoded blob to report
> > +# hypervisor-defined workarounds, corresponding
> > +# to the 'GOSVW' parameter of the
> > +# SNP_LAUNCH_START command defined in the
> > +# SEV-SNP firmware ABI (default: all-zero)
> > +#
> > +# @id-block: 96-byte, base64-encoded blob to provide the 'ID Block'
> > +#structure for the SNP_LAUNCH_FINISH command defined in the
> > +#SEV-SNP firmware ABI (default: all-zero)
> > +#
> > +# @id-auth: 4096-byte, base64-encoded blob to provide the 'ID 
> > Authentication
> > +#   Information Structure' for the SNP_LAUNCH_FINISH command 
> > defined
> > +#   in the SEV-SNP firmware ABI (default: all-zero)
> > +#
> > +# @auth-key-enabled: true if 'id-auth' blob contains the 'AUTHOR_KEY' field
> > +#defined SEV-SNP firmware ABI (default: false)
> > +#
> > +# @host-data: 32-byte, base64-encoded, user-defined blob to provide to the
> > +# guest, as documented for the 'HOST_DATA' parameter of the
> > +# SNP_LAUNCH_FINISH command in the SEV-SNP firmware ABI
> > +# (default: all-zero)
> > +#
> > +# Since: 7.2
> 
> This will be 9.1 at the earliest now.

Amazing how good I am at remembering these once I see a reply to a
schema patch I'd already hit 'send' on :)

> 
> > +##
> > +{ 'struct': 'SevSnpGuestProperties',
> > +  'base': 'SevCommonProperties',
> > +  'data': {
> > +'*policy': 'uint64',
> > +'*guest-visible-workarounds': 'str',
> > +'*id-block': 'str',
> > +'*id-auth': 'str',
> > +'*auth-key-enabled': 'bool',
> > +'*host-data': 'str' } }
> > +
> 
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 63a220de5e..7e6dab642a 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -42,6 +42,7 @@
> >  
> >  OBJECT_DECLARE_SIMPLE_TYPE(SevCommonState, SEV_COMMON)
> >  OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
> > +OBJECT_DECLARE_SIMPLE_TYPE(SevSnpGuestState, SEV_SNP_GUEST)
> >  
> >  struct SevCommonState {
> >  X86ConfidentialGuest parent_obj;
> > @@ -87,8 +88,22 @@ struct SevGuestState {
> >  bool kernel_hashes;
> >  };
> >  
> > +struct SevSnpGuestState {
> > +SevCommonState sev_common;
> > +
> > +/* 

Re: [PATCH v3 21/49] i386/sev: Introduce "sev-common" type to encapsulate common SEV state

[Michael Roth via]

On Wed, Mar 20, 2024 at 11:47:28AM +, Daniel P. Berrangé wrote:
> On Wed, Mar 20, 2024 at 03:39:17AM -0500, Michael Roth wrote:
> > Currently all SEV/SEV-ES functionality is managed through a single
> > 'sev-guest' QOM type. With upcoming support for SEV-SNP, taking this
> > same approach won't work well since some of the properties/state
> > managed by 'sev-guest' is not applicable to SEV-SNP, which will instead
> > rely on a new QOM type with its own set of properties/state.
> > 
> > To prepare for this, this patch moves common state into an abstract
> > 'sev-common' parent type to encapsulate properties/state that are
> > common to both SEV/SEV-ES and SEV-SNP, leaving only SEV/SEV-ES-specific
> > properties/state in the current 'sev-guest' type. This should not
> > affect current behavior or command-line options.
> > 
> > As part of this patch, some related changes are also made:
> > 
> >   - a static 'sev_guest' variable is currently used to keep track of
> > the 'sev-guest' instance. SEV-SNP would similarly introduce an
> > 'sev_snp_guest' static variable. But these instances are now
> > available via qdev_get_machine()->cgs, so switch to using that
> > instead and drop the static variable.
> > 
> >   - 'sev_guest' is currently used as the name for the static variable
> > holding a pointer to the 'sev-guest' instance. Re-purpose the name
> > as a local variable referring the 'sev-guest' instance, and use
> > that consistently throughout the code so it can be easily
> > distinguished from sev-common/sev-snp-guest instances.
> > 
> >   - 'sev' is generally used as the name for local variables holding a
> > pointer to the 'sev-guest' instance. In cases where that now points
> > to common state, use the name 'sev_common'; in cases where that now
> > points to state specific to 'sev-guest' instance, use the name
> > 'sev_guest'
> > 
> > Signed-off-by: Michael Roth 
> > ---
> >  qapi/qom.json |  32 ++--
> >  target/i386/sev.c | 457 ++
> >  target/i386/sev.h |   3 +
> >  3 files changed, 281 insertions(+), 211 deletions(-)
> > 
> 
> >  static SevInfo *sev_get_info(void)
> >  {
> >  SevInfo *info;
> > +SevCommonState *sev_common = 
> > SEV_COMMON(MACHINE(qdev_get_machine())->cgs);
> > +SevGuestState *sev_guest =
> > +(SevGuestState *)object_dynamic_cast(OBJECT(sev_common),
> > + TYPE_SEV_GUEST);
> >  
> >  info = g_new0(SevInfo, 1);
> >  info->enabled = sev_enabled();
> >  
> >  if (info->enabled) {
> > -info->api_major = sev_guest->api_major;
> > -info->api_minor = sev_guest->api_minor;
> > -info->build_id = sev_guest->build_id;
> > -info->policy = sev_guest->policy;
> > -info->state = sev_guest->state;
> > -info->handle = sev_guest->handle;
> > +if (sev_guest) {
> > +info->handle = sev_guest->handle;
> > +}
> 
> If we're not going to provide a value for 'handle', then
> we should update the QAPI for this to mark the property
> as optional, which would then require doing
> 
>   info->has_handle = true;
> 
> inside this 'if' block.

I think this is another temporarily-awkward case that gets resolved
with:

  i386/sev: Update query-sev QAPI format to handle SEV-SNP

With that patch 'handle' is always available for SEV guests, and never
available for SNP, and that's managed through a discriminated union
type. I think that info->handle should be treated the same as the
other fields as part of this patch and any changes in how they are
reported should be kept in the above-mentioned patch.

This might be another artifact from v2's handling. Will get this fixed
up.

-Mike

> > +}

> 
> > +info->api_major = sev_common->api_major;
> > +info->api_minor = sev_common->api_minor;
> > +info->build_id = sev_common->build_id;
> > +info->state = sev_common->state;
> > +/* we only report the lower 32-bits of policy for SNP, ok for 
> > now... */
> > +info->policy =
> > +(uint32_t)object_property_get_uint(OBJECT(sev_common),
> > +   "policy", NULL);
> >  }
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
> 

Re: [PATCH v3 21/49] i386/sev: Introduce "sev-common" type to encapsulate common SEV state

[Michael Roth via]

On Wed, Mar 20, 2024 at 11:44:13AM +, Daniel P. Berrangé wrote:
> On Wed, Mar 20, 2024 at 03:39:17AM -0500, Michael Roth wrote:
> > Currently all SEV/SEV-ES functionality is managed through a single
> > 'sev-guest' QOM type. With upcoming support for SEV-SNP, taking this
> > same approach won't work well since some of the properties/state
> > managed by 'sev-guest' is not applicable to SEV-SNP, which will instead
> > rely on a new QOM type with its own set of properties/state.
> > 
> > To prepare for this, this patch moves common state into an abstract
> > 'sev-common' parent type to encapsulate properties/state that are
> > common to both SEV/SEV-ES and SEV-SNP, leaving only SEV/SEV-ES-specific
> > properties/state in the current 'sev-guest' type. This should not
> > affect current behavior or command-line options.
> > 
> > As part of this patch, some related changes are also made:
> > 
> >   - a static 'sev_guest' variable is currently used to keep track of
> > the 'sev-guest' instance. SEV-SNP would similarly introduce an
> > 'sev_snp_guest' static variable. But these instances are now
> > available via qdev_get_machine()->cgs, so switch to using that
> > instead and drop the static variable.
> > 
> >   - 'sev_guest' is currently used as the name for the static variable
> > holding a pointer to the 'sev-guest' instance. Re-purpose the name
> > as a local variable referring the 'sev-guest' instance, and use
> > that consistently throughout the code so it can be easily
> > distinguished from sev-common/sev-snp-guest instances.
> > 
> >   - 'sev' is generally used as the name for local variables holding a
> > pointer to the 'sev-guest' instance. In cases where that now points
> > to common state, use the name 'sev_common'; in cases where that now
> > points to state specific to 'sev-guest' instance, use the name
> > 'sev_guest'
> > 
> > Signed-off-by: Michael Roth 
> > ---
> >  qapi/qom.json |  32 ++--
> >  target/i386/sev.c | 457 ++
> >  target/i386/sev.h |   3 +
> >  3 files changed, 281 insertions(+), 211 deletions(-)
> > 
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index baae3a183f..66b5781ca6 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -875,12 +875,29 @@
> >'data': { '*filename': 'str' } }
> >  
> >  ##
> > -# @SevGuestProperties:
> > +# @SevCommonProperties:
> >  #
> > -# Properties for sev-guest objects.
> > +# Properties common to objects that are derivatives of sev-common.
> >  #
> >  # @sev-device: SEV device to use (default: "/dev/sev")
> >  #
> > +# @cbitpos: C-bit location in page table entry (default: 0)
> > +#
> > +# @reduced-phys-bits: number of bits in physical addresses that become
> > +# unavailable when SEV is enabled
> > +#
> > +# Since: 2.12
> 
> Not quite sure what we've done in this scenario before.
> It feels wierd to use '2.12' for the new base type, even
> though in effect the properties all existed since 2.12 in
> the sub-class.
> 
> Perhaps 'Since: 9.1' for the type, but 'Since: 2.12' for the
> properties, along with an explanatory comment about stuff
> moving into the new base type ?
> 
> Markus, opinions ?

My thinking is that the internal details are less important than what's
actually exposed to users in the form of command-line options/etc. So
in that context the "Since: 2.12" sort of becomes the "default" for when
those properties were first made available to users, and then anything we
add after would then get special treatment with the per-property
versioning. But no issue with taking a different approach if that's
preferred.

> 
> > +##
> > +{ 'struct': 'SevCommonProperties',
> > +  'data': { '*sev-device': 'str',
> > +'*cbitpos': 'uint32',
> > +'reduced-phys-bits': 'uint32' } }
> > +
> > +##
> > +# @SevGuestProperties:
> > +#
> > +# Properties for sev-guest objects.
> > +#
> >  # @dh-cert-file: guest owners DH certificate (encoded with base64)
> >  #
> >  # @session-file: guest owners session parameters (encoded with base64)
> > @@ -889,11 +906,6 @@
> >  #
> >  # @handle: SEV firmware handle (default: 0)
> >  #
> > -# @cbitpos: C-bit location in page table entry (default: 0)
> > -#
> > -# @reduced-phys-bits: number of bits in physical addresses that become
> > -# unavailable when SEV is enabled
> > -#
> >  # @kernel-hashes: if true, add hashes of kernel/initrd/cmdline to a
> >  # designated guest firmware page for measured boot with -kernel
> >  # (default: false) (since 6.2)
> > @@ -901,13 +913,11 @@
> >  # Since: 2.12
> >  ##
> >  { 'struct': 'SevGuestProperties',
> > -  'data': { '*sev-device': 'str',
> > -'*dh-cert-file': 'str',
> > +  'base': 'SevCommonProperties',
> > +  'data': { '*dh-cert-file': 'str',
> >  '*session-file': 'str',
> >  '*policy': 'uint32',
> >  '*handle': 'uint32',
> > -'*cbitpos': 'uint32',
> > -

Re: [PATCH v2 for-8.2?] i386/sev: Avoid SEV-ES crash due to missing MSR_EFER_LMA bit

[Michael Roth via]

On Wed, Dec 06, 2023 at 12:48:35PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Michael,
> 
> (Cc'ing Lara, Vitaly and Maxim)
> 
> On 5/12/23 23:28, Michael Roth wrote:
> > Commit 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
> > added error checking for KVM_SET_SREGS/KVM_SET_SREGS2. In doing so, it
> > exposed a long-running bug in current KVM support for SEV-ES where the
> > kernel assumes that MSR_EFER_LMA will be set explicitly by the guest
> > kernel, in which case EFER write traps would result in KVM eventually
> > seeing MSR_EFER_LMA get set and recording it in such a way that it would
> > be subsequently visible when accessing it via KVM_GET_SREGS/etc.
> > 
> > However, guests kernels currently rely on MSR_EFER_LMA getting set
> > automatically when MSR_EFER_LME is set and paging is enabled via
> > CR0_PG_MASK. As a result, the EFER write traps don't actually expose the
> > MSR_EFER_LMA even though it is set internally, and when QEMU
> > subsequently tries to pass this EFER value back to KVM via
> > KVM_SET_SREGS* it will fail various sanity checks and return -EINVAL,
> > which is now considered fatal due to the aforementioned QEMU commit.
> > 
> > This can be addressed by inferring the MSR_EFER_LMA bit being set when
> > paging is enabled and MSR_EFER_LME is set, and synthesizing it to ensure
> > the expected bits are all present in subsequent handling on the host
> > side.
> > 
> > Ultimately, this handling will be implemented in the host kernel, but to
> > avoid breaking QEMU's SEV-ES support when using older host kernels, the
> > same handling can be done in QEMU just after fetching the register
> > values via KVM_GET_SREGS*. Implement that here.
> > 
> > Cc: Paolo Bonzini 
> > Cc: Marcelo Tosatti 
> > Cc: Tom Lendacky 
> > Cc: Akihiko Odaki 
> > Cc: k...@vger.kernel.org
> > Fixes: 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
> 

Hi Philippe,

> This 'Fixes:' tag is misleading, since as you mentioned this commit
> only exposes the issue.

That's true, a "Workaround-for: " tag or something like that might be more
appropriate. I just wanted to make it clear that SEV-ES support is no longer
working with that patch applied, so I used Fixes: and elaborated on the
commit message. I can change it if there's a better way to convey this
though.

> 
> Commit d499f196fe ("target/i386: Added consistency checks for EFER")
> or around it seems more appropriate.

Those checks seem to be more for TCG. The actual bug is in the host
kernel, and it seems to have been there basically since the original
SEV-ES host support went in in 2020. I've also sent a patch to address
this in KVM:

  
https://lore.kernel.org/lkml/20231205234956.1156210-1-michael.r...@amd.com/T/#u

but in the meantime it means that QEMU 8.2+ SEV-ES support would no
longer work for any current/older host kernels, so I'm hoping a targeted
workaround is warranted to cover that gap.

> 
> Is this feature easily testable on our CI, on a x86 runner with KVM
> access?

SEV-ES support was introduced with EPYC Zen2 architecture (EPYC 7002
series processors, aka "Rome"). If there are any systems in the test pool
that are Zen2 or greater, then a simple boot of a SEV-ES linux guest would
be enough to trigger the QEMU crash. I'm not sure if there are any systems
of that sort in the pool though.

Thanks,

Mike

Re: [RFC PATCH 00/19] QEMU gmem implemention

[Michael Roth via]

On Tue, Aug 01, 2023 at 09:45:41AM +0800, Xiaoyao Li wrote:
> On 8/1/2023 12:51 AM, Daniel P. Berrangé wrote:
> > On Mon, Jul 31, 2023 at 12:21:42PM -0400, Xiaoyao Li wrote:
> > > This is the first RFC version of enabling KVM gmem[1] as the backend for
> > > private memory of KVM_X86_PROTECTED_VM.
> > > 
> > > It adds the support to create a specific KVM_X86_PROTECTED_VM type VM,
> > > and introduces 'private' property for memory backend. When the vm type
> > > is KVM_X86_PROTECTED_VM and memory backend has private enabled as below,
> > > it will call KVM gmem ioctl to allocate private memory for the backend.
> > > 
> > >  $qemu -object memory-backend-ram,id=mem0,size=1G,private=on \
> > >-machine q35,kvm-type=sw-protected-vm,memory-backend=mem0 \
> > > ...
> > > 
> > > Unfortunately this patch series fails the boot of OVMF at very early
> > > stage due to triple fault because KVM doesn't support emulate string IO
> > > to private memory. We leave it as an open to be discussed.
> > > 
> > > There are following design opens that need to be discussed:
> > > 
> > > 1. how to determine the vm type?
> > > 
> > > a. like this series, specify the vm type via machine property
> > >'kvm-type'
> > > b. check the memory backend, if any backend has 'private' property
> > >set, the vm-type is set to KVM_X86_PROTECTED_VM.
> > > 
> > > 2. whether 'private' property is needed if we choose 1.b as design
> > > 
> > > with 1.b, QEMU can decide whether the memory region needs to be
> > > private (allocates gmem fd for it) or not, on its own.
> > > 
> > > 3. What is KVM_X86_SW_PROTECTED_VM going to look like? What's the
> > > purose of it and what's the requirement on it. I think it's the
> > > questions for KVM folks than QEMU folks.
> > > 
> > > Any other idea/open/question is welcomed.
> > > 
> > > 
> > > Beside, TDX QEMU implemetation is based on this series to provide
> > > private gmem for TD private memory, which can be found at [2].
> > > And it can work corresponding KVM [3] to boot TDX guest.
> > 
> > We already have a general purpose configuration mechanism for
> > confidential guests.  The -machine argument has a property
> > confidential-guest-support=$OBJECT-ID, for pointing to an
> > object that implements the TYPE_CONFIDENTIAL_GUEST_SUPPORT
> > interface in QEMU. This is implemented with SEV, PPC PEF
> > mode, and s390 protvirt.
> > 
> > I would expect TDX to follow this same design ie
> > 
> >  qemu-system-x86_64 \
> >-object tdx-guest,id=tdx0,. \
> >-machine q35,confidential-guest-support=tdx0 \
> >...
> > 
> > and not require inventing the new 'kvm-type' attribute at least.
> 
> yes.
> 
> TDX is initialized exactly as the above.
> 
> This RFC series introduces the 'kvm-type' for KVM_X86_SW_PROTECTED_VM. It's
> my fault that forgot to list the option of introducing sw_protected_vm
> object with CONFIDENTIAL_GUEST_SUPPORT interface.
> Thanks for Isaku to raise it 
> https://lore.kernel.org/qemu-devel/20230731171041.gb1807...@ls.amr.corp.intel.com/
> 
> we can specify KVM_X86_SW_PROTECTED_VM this way:
> 
> qemu  \
>   -object sw-protected,id=swp0,... \
>   -machine confidential-guest-support=swp0 \
>   ...
> 
> > For the memory backend though, I'm not so sure - possibly that
> > might be something that still wants an extra property to identify
> > the type of memory to allocate, since we use memory-backend-ram
> > for a variety of use cases.  Or it could be an entirely new object
> > type such as "memory-backend-gmem"
> 
> What I want to discuss is whether providing the interface to users to allow
> them configuring which memory is/can be private. For example, QEMU can do it
> internally. If users wants a confidential guest, QEMU allocates private gmem
> for normal RAM automatically.

I think handling it automatically simplifies things a good deal on the
QEMU side. I think it's still worthwhile to still allow:

 -object memory-backend-memfd-private,...

because it provides a nice mechanism to set up a pair of shared/private
memfd's to enable hole-punching via fallocate() to avoid doubling memory
allocations for shared/private. It's also a nice place to control
potentially-configurable things like:

 - whether or not to enable discard/hole-punching
 - if discard is enabled, whether or not to register the range via
   RamDiscardManager interface so that VFIO/IOMMU mappings get updated
   when doing PCI passthrough. SNP relies on this for PCI passthrough
   when discard is enabled, otherwise DMA occurs to stale mappings of
   discarded bounce-buffer pages:

 
https://github.com/AMDESE/qemu/blob/snp-latest/backends/hostmem-memfd-private.c#L449

But for other memory ranges, it doesn't do a lot of good to rely on
users to control those via -object memory-backend-memfd-private, since
QEMU will set up some regions internally, like the UEFI ROM.

It also isn't ideal for QEMU itself to internally control what
should/shouldn't 

Re: QEMU stable 7.2.1

[Michael Roth via]

On Wed, Apr 05, 2023 at 02:54:47PM +0300, Michael Tokarev wrote:
> So let it be, with a delay of about a week.
> 
> Since no one from the qemu team replied to my final-release steps, I'm
> making it available on my site instead:
> 
>   http://www.corpit.ru/mjt/qemu/qemu-7.2.1.tar.xz
>   http://www.corpit.ru/mjt/qemu/qemu-7.2.1.tar.xz.sig - signed with my GPG key
>   http://www.corpit.ru/mjt/qemu/qemu-7.2.1.diff - whole difference from 7.2.0.
> 
> The tag (v7.2.1) is in the main qemu repository.

Hi Michael,

Thanks for handling this release!

Somehow I missed your final steps email, but for future releases I would
recommend going ahead and tagging your release (also signed with your GPG
key) in your local tree once you've got everything ready, and then sending
me an email to directly so I can push that to gitlab and then handle
creating the tarball and publish it with my GPG key. That's basically what
we do for the normal QEMU releases as well.

Then once you get your accounts set up by gitlab/qemu.org admins you can
handle the tag-pushing/tarball-uploading on your end. Would be good to
have someone else involved with that process so we have some redundancy
just in case.

For 7.2.1:

I could also upload your tarball, but we'd also want a signed .bz2 tarball
to accomodate any environments that still try to consume the .bz2 versions
(even though we don't actively advertise them on the website). We'd also
need to update the GPG key published on the website at
https://www.qemu.org/download/

So for this one it might make sense to keep the tarball published where you
have it, and I'll just mirror your 7.2.1 tag to QEMU gitlab under a new
stable-7.2 branch like we've done in the past. Then for subsequent
releases I'll publish as mentioned above until you have upload/push access.

If that sounds reasonable to you (and everyone else) I'll go ahead and
push the stable-7.2/7.2.1 branches to by gitlab EOD tomorrow my time.

Thanks!

-Mike

> 
> The shortlog:
> 
> Akihiko Odaki (4):
>   vhost-user-gpio: Configure vhost_dev when connecting
>   vhost-user-i2c: Back up vqs before cleaning up vhost_dev
>   vhost-user-rng: Back up vqs before cleaning up vhost_dev
>   hw/timer/hpet: Fix expiration time overflow
> 
> Alex Bennée (2):
>   target/arm: fix handling of HLT semihosting in system mode
>   tests/tcg: fix unused variable in linux-test
> 
> Anton Johansson (1):
>   block: Handle curl 7.55.0, 7.85.0 version changes
> 
> Carlos López (2):
>   vhost: avoid a potential use of an uninitialized variable in 
> vhost_svq_poll()
>   libvhost-user: check for NULL when allocating a virtqueue element
> 
> Chenyi Qiang (2):
>   virtio-mem: Fix the bitmap index of the section offset
>   virtio-mem: Fix the iterator variable in a vmem->rdl_list loop
> 
> David Hildenbrand (2):
>   migration/ram: Fix error handling in ram_write_tracking_start()
>   migration/ram: Fix populate_read_range()
> 
> Dr. David Alan Gilbert (2):
>   virtio-rng-pci: fix migration compat for vectors
>   virtio-rng-pci: fix transitional migration compat for vectors
> 
> Eugenio Pérez (1):
>   vdpa: stop all svq on device deletion
> 
> Evgeny Iakovlev (1):
>   target/arm: allow writes to SCR_EL3.HXEn bit when FEAT_HCX is enabled
> 
> Guenter Roeck (1):
>   target/sh4: Mask restore of env->flags from tb->flags
> 
> Jason Wang (3):
>   vhost: fix vq dirty bitmap syncing when vIOMMU is enabled
>   intel-iommu: fail MAP notifier without caching mode
>   intel-iommu: fail DEVIOTLB_UNMAP without dt mode
> 
> Julia Suvorova (1):
>   hw/smbios: fix field corruption in type 4 table
> 
> Kevin Wolf (1):
>   qcow2: Fix theoretical corruption in store_bitmap() error path
> 
> Klaus Jensen (2):
>   hw/nvme: fix missing endian conversions for doorbell buffers
>   hw/nvme: fix missing cq eventidx update
> 
> Laszlo Ersek (1):
>   acpi: cpuhp: fix guest-visible maximum access size to the legacy reg 
> block
> 
> Marc-André Lureau (1):
>   build-sys: fix crlf-ending C code
> 
> Michael S. Tsirkin (6):
>   Revert "x86: do not re-randomize RNG seed on snapshot load"
>   Revert "x86: re-initialize RNG seed when selecting kernel"
>   Revert "x86: reinitialize RNG seed on system reboot"
>   Revert "x86: use typedef for SetupData struct"
>   Revert "x86: return modified setup_data only if read as memory, not as 
> file"
>   Revert "hw/i386: pass RNG seed via setup_data entry"
> 
> Michael Tokarev (1):
>   Update version for 7.2.1 release
> 
> Paolo Bonzini (4):
>   meson: accept relative symlinks in "meson introspect --installed" data
>   configure: fix GLIB_VERSION for cross-compilation
>   target/i386: fix ADOX followed by ADCX
>   block/iscsi: fix double-free on BUSY or similar statuses
> 
> Richard Henderson (8):
>   target/riscv: Set pc_succ_insn for !rvc illegal insn
>   target/arm: Fix sve_probe_page
>   

Re: stable releases

[Michael Roth via]

On Mon, Mar 06, 2023 at 09:57:58AM +0100, Thomas Huth wrote:
> On 05/03/2023 11.27, Michael Tokarev wrote:
> > Hi!
> > 
> > For a few qemu major releases already, we did not have any stable minor
> > releases.
> > I'd love to change that, in order to consolidate efforts and to make better
> > software in the end.  But I need some (hopefully minor) help here.
> > 
> > I collected changes from qemu/master which apparently should go to -stable.
> > Published at git://isrv.corpit.ru/qemu.git , branch stable-7.2-staging
> > (probably should publish it on github or gitlab).
> > This contains stuff which I use on Debian in qemu package, which is based
> > on 7.2 version now.  The last 18 patches are not in Debian package yet,
> > going to push it today or tomorrow.
> > 
> > If nothing, this can be used as a base for actual 7.2.1 stable release,
> > maybe with more changes added (or some changes removed).
> > 
> > The help which I need is to be able to run some wider testing. Debian is
> > a relatively good testbed, and it is used by qemu already in terms of
> > bullseye-backports to run other tests, so it should be good, but I'd
> > love to have wider coverage still. Maybe some CI stuff which qemu has
> > for master, if not only just before actual release.

Hi Michael,

Thank you for offering to help with the stable releases. I think it
would be in great hands and am happy to help in any way with getting
things going there.

I totally agree on Thomas' suggestions for next steps, and tried to
list out whatever else came to mind regarding stable releases in
general (sorry if things get a bit rambly).

> 
> I'd suggest to get a gitlab.com account, and fork the qemu repository there.
> Then you can run the CI on your own by pushing your patch to your forked
> repository.
> 
> You can also get some test additional coverage by running the avocado tests
> with "make check-avocado" ... but beware that this downloads quite some
> hundreds of MBs from the internet. And some tests are known to fail, so it's
> maybe best to run them on the plain 7.2.0 tag first to see what works for
> you and what does not work properly.

As far as testing, I think some other good tests/areas to consider eventually
adding would be:

  - qemu-iotests
  - VFIO/PCI passthrough tests of some sort if possible
  - live migration tests (especially things like 7.2.1 <-> 7.2.0
migration compatibility to allow for rolling out/back live upgrades and
things of that sort)
  - Windows guests

> 
> > And as usual, this needs help in picking up changes which should go to
> > stable. But this is something which is always needed anyway.
> > 
> > Let's resurrect qemu-stable :)

The "current" stable process is documented at docs/devel/stable-process.rst
As written, 7.2 support would end when 7.3/8.0 is released, and then
8.0.1 would be the next stable release afterward, but maybe there are more
optimal ways to integrate/schedule things in your case so don't feel
tied this approach.

As far as this release goes I'd recommend going ahead and getting your
stable-7.2-staging tree rebased on 7.2.0, along with whatever other patches
you think should definitely be included. For me this would include all CVE
fixes that went in after 7.2.0, and any patches tagged Cc:qemu-stable or
forwarded to qemu-stable mailing list. But that's another area to decide on
how you want to handle things. Maybe in some cases some important fixes
are better to get out sooner rather than trying to make every release
"complete".

I'd usually then post the staging tree to the mailing list to see if
there were any more candidates, which I think has pretty much always
identified more commits to pull in and proven worthwhile; but probably
depends on how frequently you cut releases. Maybe it makes sense to not
even do tagged releases, and just have a rolling stable tree? Things to
consider.

Ideally you'd be able to eventually push trees to the official QEMU repo
like I've done in the past. You'll need to work with the gitlab project
maintainers on that. But if you need to host/tag them in your own repo
until then that would probably be fine. Will want some way to
communicate this to QEMU community though, and official git repo is
probably the best way. I can also push your tree/tags as interim solution.

Will also need to figure out how to handle uploading tarballs (assuming
you still intend on distributing release tarballs). I can upload them
in the meantime, but we will probably want to work with Paolo getting
you access.

If there's anything else I can help with please let me know.

Thanks,

Mike

> 
> Please make sure to CC: qemu-stable and Michael Roth - I hope he can give
> some directions for this effort.
> 
>  Thomas
> 

Re: [PATCH 00/47] Patch Round-up for stable 6.1.1, freeze on 2021-12-21

[Michael Roth via]

On Tue, Dec 14, 2021 at 06:00:38PM -0600, Michael Roth wrote:
> Hi everyone,
> 
> The following new patches are queued for QEMU stable v6.1.1:
> 
>   https://gitlab.com/qemu-project/qemu/-/commits/stable-6.1-staging/
> 
> Patch freeze is 2021-12-21, and the release is planned for 2021-12-23:
> 
>   https://wiki.qemu.org/Planning/6.1
> 
> Please respond here or CC qemu-sta...@nongnu.org on any additional patches
> you think should (or shouldn't) be included in the release.

Thank you for the suggestions so far. The following additional patches have
been pushed to the staging tree:

  fddd169de5 tests: tcg: Fix PVH test with binutils 2.36+
  711bd602cc tcg/arm: Reduce vector alignment requirement for NEON
  e88636b4d4 target/i386: add missing bits to CR4_RESERVED_MASK
  34833f361b qxl: fix pre-save logic

  https://gitlab.com/mdroth/qemu/-/commits/stable-6.1-staging/

Patch freeze is 2021-21-21 EOD.

-Mike

> 
> Thanks!
> 
> 
> Ani Sinha (6):
>   bios-tables-test: allow changes in DSDT ACPI tables for q35
>   hw/i386/acpi: fix conflicting IO address range for acpi pci hotplug in 
> q35
>   bios-tables-test: Update ACPI DSDT table golden blobs for q35
>   tests/acpi/bios-tables-test: add and allow changes to a new q35 DSDT 
> table blob
>   tests/acpi/pcihp: add unit tests for hotplug on multifunction bridges 
> for q35
>   tests/acpi/bios-tables-test: update DSDT blob for multifunction bridge 
> test
> 
> Ari Sundholm (1):
>   block/file-posix: Fix return value translation for AIO discards
> 
> Christian Schoenebeck (1):
>   9pfs: fix crash in v9fs_walk()
> 
> Daniil Tatianin (1):
>   chardev/wctable: don't free the instance in wctablet_chr_finalize
> 
> David Hildenbrand (3):
>   virtio-balloon: don't start free page hinting if postcopy is possible
>   virtio-mem-pci: Fix memory leak when creating MEMORY_DEVICE_SIZE_CHANGE 
> event
>   libvhost-user: fix VHOST_USER_REM_MEM_REG skipping mmap_addr
> 
> Eric Blake (1):
>   nbd/server: Don't complain on certain client disconnects
> 
> Gerd Hoffmann (1):
>   uas: add stream number sanity checks.
> 
> Greg Kurz (2):
>   rcu: Introduce force_rcu notifier
>   accel/tcg: Register a force_rcu notifier
> 
> Helge Deller (1):
>   hw/display/artist: Fix bug in coordinate extraction in 
> artist_vram_read() and artist_vram_write()
> 
> Igor Mammedov (1):
>   pcie: rename 'native-hotplug' to 'x-native-hotplug'
> 
> Jason Wang (3):
>   virtio-net: fix use after unmap/free for sg
>   virtio: use virtio accessor to access packed descriptor flags
>   virtio: use virtio accessor to access packed event
> 
> Jean-Philippe Brucker (2):
>   hw/arm/virt: Rename default_bus_bypass_iommu
>   hw/i386: Rename default_bus_bypass_iommu
> 
> Jessica Clarke (1):
>   Partially revert "build: -no-pie is no functional linker flag"
> 
> Jon Maloy (1):
>   e1000: fix tx re-entrancy problem
> 
> Klaus Jensen (1):
>   hw/nvme: fix buffer overrun in nvme_changed_nslist (CVE-2021-3947)
> 
> Laurent Vivier (1):
>   hw: m68k: virt: Add compat machine for 6.1
> 
> Mahmoud Mandour (1):
>   plugins/execlog: removed unintended "s" at the end of log lines.
> 
> Mark Mielke (1):
>   virtio-blk: Fix clean up of host notifiers for single MR transaction.
> 
> Markus Armbruster (1):
>   hmp: Unbreak "change vnc"
> 
> Mauro Matteo Cascella (1):
>   hw/scsi/scsi-disk: MODE_PAGE_ALLS not allowed in MODE SELECT commands
> 
> Michael S. Tsirkin (1):
>   pci: fix PCI resource reserve capability on BE
> 
> Michael Tokarev (1):
>   qemu-sockets: fix unix socket path copy (again)
> 
> Nir Soffer (1):
>   qemu-nbd: Change default cache mode to writeback
> 
> Paolo Bonzini (4):
>   plugins: do not limit exported symbols if modules are active
>   block: introduce max_hw_iov for use in scsi-generic
>   target-i386: mmu: use pg_mode instead of HF_LMA_MASK
>   target-i386: mmu: fix handling of noncanonical virtual addresses
> 
> Peng Liang (1):
>   vfio: Fix memory leak of hostwin
> 
> Peter Maydell (1):
>   target/arm: Don't skip M-profile reset entirely in user mode
> 
> Philippe Mathieu-Daudé (3):
>   hw/block/fdc: Extract blk_create_empty_drive()
>   hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196
>   tests/qtest/fdc-test: Add a regression test for CVE-2021-20196
> 
> Prasad J Pandit (1):
>   net: vmxnet3: validate configuration values during activate 
> (CVE-2021-20203)
> 
> Stefano Garzarella (1):
>   vhost-vsock: fix migration issue when seqpacket is supported
> 
> Xueming Li (1):
>   vhost-user: fix duplicated notifier MR init
> 
> Yang Zhong (1):
>   i386/cpu: Remove AVX_VNNI feature from Cooperlake cpu model
> 
>  accel/tcg/tcg-accel-ops-mttcg.c   |  26 
>  accel/tcg/tcg-accel-ops-rr.c  |  10 
>  

Re: [PATCH 00/47] Patch Round-up for stable 6.1.1, freeze on 2021-12-21

[Michael Roth via]

On Wed, Dec 15, 2021 at 09:20:31AM +, Daniel P. Berrangé wrote:
> On Tue, Dec 14, 2021 at 06:00:38PM -0600, Michael Roth wrote:
> > Hi everyone,
> > 
> > The following new patches are queued for QEMU stable v6.1.1:
> > 
> >   https://gitlab.com/qemu-project/qemu/-/commits/stable-6.1-staging/
> 
> FYI, this branch doesn't appear to have been pushed.

Argh, sorry, outdated link format in my email template, the correct URL is:

  https://gitlab.com/mdroth/qemu/-/commits/stable-6.1-staging/

> 
> > Patch freeze is 2021-12-21, and the release is planned for 2021-12-23:
> > 
> >   
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwiki.qemu.org%2FPlanning%2F6.1data=04%7C01%7Cmichael.roth%40amd.com%7Cc2deb18e48d7428bcd7a08d9bfac34df%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637751568618799581%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=yyM%2FWVt9UcJocOhBwRVXRN5MJmhttQ022gRhB1XlRcg%3Dreserved=0
> > 
> > Please respond here or CC qemu-sta...@nongnu.org on any additional patches
> > you think should (or shouldn't) be included in the release.
> 
> Based on critical fixes Fedora users hit in 6.1 we pulled in
> the following fixes that you've not queued yet:
> 
>   eb94846280df3f1e2a91b6179fc05f9890b7e384 qxl: fix pre-save logic
> 
>   69e3895f9d37ca39536775b13ce63e8c291427ba target/i386: add missing bits to 
> CR4_RESERVED_MASK
> 
>   b9537d5904f6e3df896264a6144883ab07db9608 tcg/arm: Reduce vector alignment 
> requirement for NEON
> 
>   8e751e9c38e324737fd3d3aa0562f886313bba3c tests: tcg: Fix PVH test with 
> binutils 2.36+

Will get these applied. Thanks!

Re: [PATCH-for-6.0.1 0/2] gitlab-ci: Only push docker images to mainstream registry from /master

[Michael Roth via]

On Wed, Oct 27, 2021 at 12:09:39PM +0200, Philippe Mathieu-Daudé wrote:
> +Richard/Peter
> 
> On 10/27/21 10:49, Daniel P. Berrangé wrote:
> > On Wed, Oct 27, 2021 at 07:26:54AM +0200, Philippe Mathieu-Daudé wrote:
> >> Hi Michael,
> >>
> >> 2 more patches to avoid gitlab-ci mayhem when you push the
> >> stable tags. See this cover for more info:
> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.mail-archive.com%2Fqemu-devel%40nongnu.org%2Fmsg846861.htmldata=04%7C01%7Cmichael.roth%40amd.com%7C3c19b44a450a4db8aa1c08d99931e741%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637709261892671750%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=kPrWrqTCJSgz%2FLoCfYNDyIb6zqXY%2Fl8v1p4IgUg5psM%3Dreserved=0
> > 
> > Please don't push this to stable - Thomas points out that it is broken
> > when any changes to dockerfiles are made.
> 
> But we still don't want to update the registry with these old
> images...
> 
> What is the plan then, hold the stable tag until we figure out
> the best fix?
> 
> Otherwise Michael can use 'git-push --push-option=ci.skip' to
> not trigger a CI pipeline when pushing stable tags (running
> CI pipelines previously in his own gitlab namespace).

I can take this approach for now.

Thanks everyone for all the debugging/suggestions.

-Mike

> 
> >> Based-on: <20211019140944.152419-1-michael.r...@amd.com>
> >> "Patch Round-up for stable 6.0.1, freeze on 2021-10-26"
> >>
> >> Daniel P. Berrangé (1):
> >>   gitlab: only let pages be published from default branch
> >>
> >> Philippe Mathieu-Daudé (1):
> >>   gitlab-ci: Only push docker images to registry from /master branch
> >>
> >>  .gitlab-ci.d/containers.yml | 11 ++-
> >>  .gitlab-ci.d/edk2.yml   | 11 ++-
> >>  .gitlab-ci.d/opensbi.yml| 11 ++-
> >>  .gitlab-ci.yml  | 18 ++
> >>  4 files changed, 48 insertions(+), 3 deletions(-)
> >>
> >> -- 
> >> 2.31.1
> >>
> >>
> >>
> > 
> > Regards,
> > Daniel
> > 
> 

Re: [PATCH] block: introduce max_hw_iov for use in scsi-generic

[Michael Roth via]

On Fri, Sep 24, 2021 at 08:50:05AM +0200, Christian Borntraeger wrote:
> Peter, Michael,
> 
> do we still do stable releases for QEMU or has this stopped?

Hi Christian,

Yes, it's just been a perfect storm of job moves / bad timing / much-needed
testing rework. I plan to restart the stable releases starting with 6.0.1 and
6.1.1 shortly after. I'll get the release schedules posted on the release wiki
week.

Sorry for the delays on this.

-Mike

> 
> Am 24.09.21 um 07:27 schrieb Paolo Bonzini:
> > Yes, the question is whether it still exists... Paolo El jue., 23 sept. 
> > 2021 16:48, Christian Borntraeger  escribió: Am 
> > 23.09.21 um 15:04 schrieb Paolo Bonzini: > Linux limits the size of iovecs 
> > to 1024 (UIO_MAXIOV ZjQcmQRYFpfptBannerStart
> > This Message Is From an External Sender
> > This message came from outside your organization.
> > ZjQcmQRYFpfptBannerEnd
> > Yes, the question is whether it still exists...
> > 
> > Paolo
> > 
> > El jue., 23 sept. 2021 16:48, Christian Borntraeger  > > escribió:
> > 
> > 
> > 
> > Am 23.09.21 um 15:04 schrieb Paolo Bonzini:
> >  > Linux limits the size of iovecs to 1024 (UIO_MAXIOV in the kernel
> >  > sources, IOV_MAX in POSIX).  Because of this, on some host adapters
> >  > requests with many iovecs are rejected with -EINVAL by the
> >  > io_submit() or readv()/writev() system calls.
> >  >
> >  > In fact, the same limit applies to SG_IO as well.  To fix both the
> >  > EINVAL and the possible performance issues from using fewer iovecs
> >  > than allowed by Linux (some HBAs have max_segments as low as 128),
> >  > introduce a separate entry in BlockLimits to hold the max_segments
> >  > value from sysfs.  This new limit is used only for SG_IO and clamped
> >  > to bs->bl.max_iov anyway, just like max_hw_transfer is clamped to
> >  > bs->bl.max_transfer.
> >  >
> >  > Reported-by: Halil Pasic  > >
> >  > Cc: Hanna Reitz mailto:hre...@redhat.com>>
> >  > Cc: Kevin Wolf mailto:kw...@redhat.com>>
> >  > Cc: qemu-bl...@nongnu.org 
> >  > Fixes: 18473467d5 ("file-posix: try BLKSECTGET on block devices too, 
> > do not round to power of 2", 2021-06-25)
> > 
> > This sneaked in shortly before the 6.1 release (between rc0 and rc1 I 
> > think).
> > Shouldnt that go to stable in cass this still exist?
> > 
> > 
> >  > Signed-off-by: Paolo Bonzini  > >
> >  > ---
> >  >   block/block-backend.c          | 6 ++
> >  >   block/file-posix.c             | 2 +-
> >  >   block/io.c                     | 1 +
> >  >   hw/scsi/scsi-generic.c         | 2 +-
> >  >   include/block/block_int.h      | 7 +++
> >  >   include/sysemu/block-backend.h | 1 +
> >  >   6 files changed, 17 insertions(+), 2 deletions(-)
> >  >
> >  > diff --git a/block/block-backend.c b/block/block-backend.c
> >  > index 6140d133e2..ba2b5ebb10 100644
> >  > --- a/block/block-backend.c
> >  > +++ b/block/block-backend.c
> >  > @@ -1986,6 +1986,12 @@ uint32_t blk_get_max_transfer(BlockBackend 
> > *blk)
> >  >       return ROUND_DOWN(max, blk_get_request_alignment(blk));
> >  >   }
> >  >
> >  > +int blk_get_max_hw_iov(BlockBackend *blk)
> >  > +{
> >  > +    return MIN_NON_ZERO(blk->root->bs->bl.max_hw_iov,
> >  > +                        blk->root->bs->bl.max_iov);
> >  > +}
> >  > +
> >  >   int blk_get_max_iov(BlockBackend *blk)
> >  >   {
> >  >       return blk->root->bs->bl.max_iov;
> >  > diff --git a/block/file-posix.c b/block/file-posix.c
> >  > index cb9bffe047..1567edb3d5 100644
> >  > --- a/block/file-posix.c
> >  > +++ b/block/file-posix.c
> >  > @@ -1273,7 +1273,7 @@ static void 
> > raw_refresh_limits(BlockDriverState *bs, Error **errp)
> >  >
> >  >           ret = hdev_get_max_segments(s->fd, );
> >  >           if (ret > 0) {
> >  > -            bs->bl.max_iov = ret;
> >  > +            bs->bl.max_hw_iov = ret;
> >  >           }
> >  >       }
> >  >   }
> >  > diff --git a/block/io.c b/block/io.c
> >  > index a19942718b..f38e7f81d8 100644
> >  > --- a/block/io.c
> >  > +++ b/block/io.c
> >  > @@ -136,6 +136,7 @@ static void bdrv_merge_limits(BlockLimits *dst, 
> > const BlockLimits *src)
> >  >       dst->min_mem_alignment = MAX(dst->min_mem_alignment,
> >  >                                    src->min_mem_alignment);
> >  >       dst->max_iov = MIN_NON_ZERO(dst->max_iov, src->max_iov);
> >  > +    dst->max_hw_iov = MIN_NON_ZERO(dst->max_hw_iov, 
> > src->max_hw_iov);
> >  >   }
> >  >
> >  >   typedef struct BdrvRefreshLimitsState {
> >  > diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> >  > index 665baf900e..0306ccc7b1 100644
> >  > --- 

Re: [RFC PATCH v2 12/12] i386/sev: update query-sev QAPI format to handle SEV-SNP

[Michael Roth via]

On Tue, Sep 07, 2021 at 12:52:54PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berra...@redhat.com) wrote:
> > On Wed, Sep 01, 2021 at 04:14:10PM +0200, Markus Armbruster wrote:
> > > Michael Roth  writes:
> > > 
> > > > Most of the current 'query-sev' command is relevant to both legacy
> > > > SEV/SEV-ES guests and SEV-SNP guests, with 2 exceptions:
> > > >
> > > >   - 'policy' is a 64-bit field for SEV-SNP, not 32-bit, and
> > > > the meaning of the bit positions has changed
> > > >   - 'handle' is not relevant to SEV-SNP
> > > >
> > > > To address this, this patch adds a new 'sev-type' field that can be
> > > > used as a discriminator to select between SEV and SEV-SNP-specific
> > > > fields/formats without breaking compatibility for existing management
> > > > tools (so long as management tools that add support for launching
> > > > SEV-SNP guest update their handling of query-sev appropriately).
> > > 
> > > Technically a compatibility break: query-sev can now return an object
> > > that whose member @policy has different meaning, and also lacks @handle.
> > > 
> > > Matrix:
> > > 
> > > Old mgmt appNew mgmt app
> > > Old QEMU, SEV/SEV-ES   goodgood(1)
> > > New QEMU, SEV/SEV-ES   good(2) good
> > > New QEMU, SEV-SNP   bad(3) good
> > > 
> > > Notes:
> > > 
> > > (1) As long as the management application can cope with absent member
> > > @sev-type.
> > > 
> > > (2) As long as the management application ignores unknown member
> > > @sev-type.
> > > 
> > > (3) Management application may choke on missing member @handle, or
> > > worse, misinterpret member @policy.  Can only happen when something
> > > other than the management application created the SEV-SNP guest (or the
> > > user somehow made the management application create one even though it
> > > doesn't know how, say with CLI option passthrough, but that's always
> > > fragile, and I wouldn't worry about it here).
> > > 
> > > I think (1) and (2) are reasonable.  (3) is an issue for management
> > > applications that support attaching to existing guests.  Thoughts?
> > 
> > IIUC you can only reach scenario (3) if you have created a guest
> > using '-object sev-snp-guest', which is a new feature introduced
> > in patch 2.
> > 
> > IOW, scenario (3)  old mgmt app + new QEMU + sev-snp guest does
> > not exist as a combination. Thus the (bad) field is actually (n/a)
> > 
> > So I believe this proposed change is acceptable in all scenarios
> > with existing deployed usage, as well as all newly introduced
> > scenarios.
> 
> I wonder if it's worth going firther and renaming 'policy' in the 
> SNP world to 'snppolicy' just to reduce the risk of accidentally
> specifying the wrong one.

Seems reasonable. I'll plan on renaming to 'snp-policy' if there are no
objections.

> 
> Dave
> 
> > Regards,
> > Daniel
> > -- 
> > |: 
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fberrange.com%2Fdata=04%7C01%7Cmichael.roth%40amd.com%7Cb9a484cd5d4f484b542908d971f61073%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637666123947391605%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=D56oHIuVk%2FmAJaKYtpJ3ZEZpKZpDPWZXydV3tpYjcM4%3Dreserved=0
> >   -o-
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.flickr.com%2Fphotos%2Fdberrangedata=04%7C01%7Cmichael.roth%40amd.com%7Cb9a484cd5d4f484b542908d971f61073%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637666123947401567%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=A9YA65nj6En3f3E2wm%2BVZE%2F6DpdbDKyHSWN9VXHAk8U%3Dreserved=0
> >  :|
> > |: 
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flibvirt.org%2Fdata=04%7C01%7Cmichael.roth%40amd.com%7Cb9a484cd5d4f484b542908d971f61073%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637666123947401567%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=yf%2FV3f3%2FNxEDwmYESp7D0ZOn74aM6cXskVJrvHLvXRE%3Dreserved=0
> >  -o-
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ffstop138.berrange.com%2Fdata=04%7C01%7Cmichael.roth%40amd.com%7Cb9a484cd5d4f484b542908d971f61073%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637666123947401567%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=pUYNVu6WWgPtwjwrjvz3YCCY7S1Qli%2FfvQKmkaRu3gc%3Dreserved=0
> >  :|
> > |: 
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fentangle-photo.org%2Fdata=04%7C01%7Cmichael.roth%40amd.com%7Cb9a484cd5d4f484b542908d971f61073%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637666123947401567%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=opdXI%2BlyzxWhUbNNgka6sMKMiLmMHfk8WuZY6cMy7yE%3Dreserved=0
> > -o-
> > 

Re: [RFC PATCH v2 12/12] i386/sev: update query-sev QAPI format to handle SEV-SNP

[Michael Roth via]

On Fri, Sep 03, 2021 at 04:30:48PM +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 03, 2021 at 10:13:16AM -0500, Michael Roth wrote:
> > On Wed, Sep 01, 2021 at 04:14:10PM +0200, Markus Armbruster wrote:
> > > Michael Roth  writes:
> > > 
> > > > Most of the current 'query-sev' command is relevant to both legacy
> > > > SEV/SEV-ES guests and SEV-SNP guests, with 2 exceptions:
> > > >
> > > >   - 'policy' is a 64-bit field for SEV-SNP, not 32-bit, and
> > > > the meaning of the bit positions has changed
> > > >   - 'handle' is not relevant to SEV-SNP
> > > >
> > > > To address this, this patch adds a new 'sev-type' field that can be
> > > > used as a discriminator to select between SEV and SEV-SNP-specific
> > > > fields/formats without breaking compatibility for existing management
> > > > tools (so long as management tools that add support for launching
> > > > SEV-SNP guest update their handling of query-sev appropriately).
> > > 
> > > Technically a compatibility break: query-sev can now return an object
> > > that whose member @policy has different meaning, and also lacks @handle.
> > > 
> > > Matrix:
> > > 
> > > Old mgmt appNew mgmt app
> > > Old QEMU, SEV/SEV-ES   goodgood(1)
> > > New QEMU, SEV/SEV-ES   good(2) good
> > > New QEMU, SEV-SNP   bad(3) good
> > > 
> > > Notes:
> > > 
> > > (1) As long as the management application can cope with absent member
> > > @sev-type.
> > > 
> > > (2) As long as the management application ignores unknown member
> > > @sev-type.
> > > 
> > > (3) Management application may choke on missing member @handle, or
> > > worse, misinterpret member @policy.  Can only happen when something
> > > other than the management application created the SEV-SNP guest (or the
> > > user somehow made the management application create one even though it
> > > doesn't know how, say with CLI option passthrough, but that's always
> > > fragile, and I wouldn't worry about it here).
> > > 
> > > I think (1) and (2) are reasonable.  (3) is an issue for management
> > > applications that support attaching to existing guests.  Thoughts?
> > 
> > Hmm... yah I hadn't considering 'old mgmt' trying to interact with a SNP
> > guest started through some other means.
> > 
> > Don't really see an alternative other than introducing a new
> > 'query-sev-snp', but that would still leave 'old mgmt' broken, since
> > it might still call do weird stuff like try to interpret the SNP policy
> > as an SEV/SEV-ES and end up with some very unexpected results. So if I
> > did go this route, I would need to have QMP begin returning an error if
> > query-sev is run against an SNP guest. But currently for non-SEV guests
> > it already does:
> > 
> >   error_setg(errp, "SEV feature is not available")
> > 
> > so 'old mgmt' should be able to handle the error just fine.
> > 
> > Would that approach be reasonable?
> 
> This ties into the question I've just sent in my other mail.
> 
> If the hardware strictly requires that guest are created in SEV-SNP
> mode only, and will not support SEV/SEV-ES mode, then we need to
> ensure "query-sev" reports the feature as not-available, so that
> existing mgmt apps don't try to use SEV/SEV-ES.

An SEV-SNP-capable host can run both 'legacy' SEV/SEV-ES, as well as
SEV-SNP guests, at the same time. But as far as QEMU goes, we need
to specify one or the other explicitly at launch time, via existing
'sev-guest', or the new 'sev-snp-guest' ConfidentialGuestSupport type.

> 
> If the SEV-SNP hardware is functionally back-compatible with a gues
> configured in SEV/SEV-ES mode, then we souldn't need a new command,
> just augment th eexisting command with additional field(s), to
> indicate existance of SEV-SNP features.

query-sev-info provides information specific to the guest instance,
like the configured policy. Are you thinking of query-sev-capabilities,
which reports some host-wide information and should indeed remain
available for either case. (and maybe should also be updated to report
on SEV-SNP availability for the host?)

> 
> Regards,
> Daniel
> -- 
> |: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fberrange.com%2Fdata=04%7C01%7Cmichael.roth%40amd.com%7C86e4ceb7f55f4cf839e708d96eefd768%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637662798671249591%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=Fo%2FCcCEG7OrpVNj2ij2CzYJCMFXs30YUnRaClz17Okc%3Dreserved=0
>   -o-
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.flickr.com%2Fphotos%2Fdberrangedata=04%7C01%7Cmichael.roth%40amd.com%7C86e4ceb7f55f4cf839e708d96eefd768%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637662798671249591%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=ypMyoiFFTmgWnDD3UwJrKIcHGzDaKQ8nXnFASw7gyRE%3Dreserved=0
>  :|
> |: 
> 

Re: [RFC PATCH 2/6] i386/sev: extend sev-guest property to include SEV-SNP

[Michael Roth via]

On Wed, Jul 21, 2021 at 03:08:37PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé  writes:
> 
> > On Tue, Jul 20, 2021 at 02:42:12PM -0500, Michael Roth wrote:
> >> On Tue, Jul 13, 2021 at 03:46:19PM +0200, Markus Armbruster wrote:
> 
> [...]
> 
> >> > I recommend to do exactly what we've done before for complex
> >> > configuration: define it in the QAPI schema, so we can use both dotted
> >> > keys and JSON on the command line, and can have QMP, too.  Examples:
> >> > -blockdev, -display, -compat.
> >> > 
> >> > Questions?
> >> 
> >> Hi Markus, Daniel,
> >> 
> >> I'm dealing with similar considerations with some SNP config options
> >> relating to CPUID enforcement, so I've started looking into this as
> >> well, but am still a little confused on the best way to proceed.
> >> 
> >> I see that -blockdev supports both JSON command-line arguments (via
> >> qobject_input_visitor_new) and dotted keys
> >> (via qobject_input_vistior_new_keyval).
> 
> Yes.  Convenience function qobject_input_visitor_new_str() provides
> this.
> 
> >> We could introduce a new config group do the same, maybe something specific
> >> to ConfidentialGuestSupport objects, e.g.:
> >> 
> >>   -confidential-guest-support sev-guest,id=sev0,key_a.subkey_b=...
> >
> > We don't wnt to be adding new CLI options anymore. -object with json
> > syntx should ultimately be able to cover everything we'll ever need
> > to do.
> 
> Depends.  When you want a CLI option to create a single QOM object, then
> -object is commonly the way to go.

So if I've read things correctly the fact that this is a question of how to
define properties of a single QOM object lends itself to using -object rather
than attempting a new -blockdev-like option group, and as such if we
want to allow structured parameters we should use pure JSON instead of
attempting to layer anything on top of the current keyval parser.

> 
> >> and use the same mechanisms to parse the options, but this seems to
> >> either involve adding a layer of option translations between command-line
> >> and the underlying object properties, or, if we keep the 1:1 mapping
> >> between QAPI-defined keys and object properties, it basically becomes a
> >> way to pass a different Visitor implementation into object_property_set(),
> >> in this case one created by object_input_visitor_new_keyval() instead of
> >> opts_visitor_new().
> 
> qobject_input_visitor_new_str() provides 1:1, i.e. common abstract
> syntax, and concrete syntax either JSON or dotted keys.  Note that the
> latter is slightly less expressive (can't do empty arrays and objects,
> may fall apart for type 'any').  If you run into these limitations, use
> JSON.  Machines should always use JSON.
> 
> qobject_input_visitor_new_str() works by wrapping the "right" visitor
> around the option argument.  Another way to provide a human-friendly
> interface in addition to a machine-friendly one is to translate from
> human to the machine interface.  HMP works that way: HMP commands wrap
> around QMP commands.  The QMP commands are generated from the QAPI
> schema.  The HMP commands are written by hand, which is maximally
> flexible, but also laborious.
> 
> >> In either case, genericizing it beyond CGS/SEV would basically be
> >> introducing:
> >> 
> >>   -object2 sev-guest,id=sev0,key_a.subkey_b=...
> 
> That's because existing -object doesn't use keyval_parse() + the keyval
> QObjct input visitor, it uses QemuOpts and the options visitor, for
> backward compatibility with all their (barely understood) features and
> warts.
> 
> Unfortunate, because even new user-creatable objects can't escape
> QemuOpts.
> 
> QemuOpts needs to go.  But replacing it is difficult and scary in
> places.  -object is such a place.
> 
> >> Which one seems preferable? Or is the answer neither?
> >
> > Yep, neither is the answer.
> 
> Welcome to the QemuOpts swamp, here's your waders and a leaky bucket.

*backs slowly away from swamp* :)

So back to the question of whether we need structured parameters. The
main driver for this seems to be that the options are currently defined
via a config file, which was originally introduced to cope with:

a) lots of parameters (8)

   - not really that significant compared to some other objects/options.

b) large page-size parameters

   - mainly applies to 'id_auth', which could be broken out as individual
 files/blobs and passed in via normal file path string arguments
   - already how we handle dh-cert-file and session-file

c) separating SNP-specific parameters from the base sev-guest object
   properties

   - could possibly be done with a new 'sev-snp-guest' object, which
 would also help disambiguate stuff like the 32-bit sev/sev-es
 'policy' arguments from the 64-bit version in SNP, and can still
 re-use common properties like 'cbitpos' via some base object
   - maybe some other benefits, need to look into it more.

If they aren't any objections I'll take a stab at this early next week.
Will be on PTO