diff mbox series

[for-5.2,v4,10/10] s390: Recognize host-trust-limitation option

Message ID 20200724025744.69644-11-david@gibson.dropbear.id.au
State New, archived
Headers show
Series Generalize memory encryption models | expand

Commit Message

David Gibson July 24, 2020, 2:57 a.m. UTC
At least some s390 cpu models support "Protected Virtualization" (PV),
a mechanism to protect guests from eavesdropping by a compromised
hypervisor.

This is similar in function to other mechanisms like AMD's SEV and
POWER's PEF, which are controlled bythe "host-trust-limitation"
machine option.  s390 is a slightly special case, because we already
supported PV, simply by using a CPU model with the required feature
(S390_FEAT_UNPACK).

To integrate this with the option used by other platforms, we
implement the following compromise:

 - When the host-trust-limitation option is set, s390 will recognize
   it, verify that the CPU can support PV (failing if not) and set
   virtio default options necessary for encrypted or protected guests,
   as on other platforms.  i.e. if host-trust-limitation is set, we
   will either create a guest capable of entering PV mode, or fail
   outright

 - If host-trust-limitation is not set, guest's might still be able to
   enter PV mode, if the CPU has the right model.  This may be a
   little surprising, but shouldn't actually be harmful.

To start a guest supporting Protected Virtualization using the new
option use the command line arguments:
    -object s390-pv-guest,id=pv0 -machine host-trust-limitation=pv0

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/s390x/pv.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

Comments

Cornelia Huck July 27, 2020, 3:50 p.m. UTC | #1
On Fri, 24 Jul 2020 12:57:44 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> At least some s390 cpu models support "Protected Virtualization" (PV),
> a mechanism to protect guests from eavesdropping by a compromised
> hypervisor.
> 
> This is similar in function to other mechanisms like AMD's SEV and
> POWER's PEF, which are controlled bythe "host-trust-limitation"
> machine option.  s390 is a slightly special case, because we already
> supported PV, simply by using a CPU model with the required feature
> (S390_FEAT_UNPACK).
> 
> To integrate this with the option used by other platforms, we
> implement the following compromise:
> 
>  - When the host-trust-limitation option is set, s390 will recognize
>    it, verify that the CPU can support PV (failing if not) and set
>    virtio default options necessary for encrypted or protected guests,
>    as on other platforms.  i.e. if host-trust-limitation is set, we
>    will either create a guest capable of entering PV mode, or fail
>    outright
> 
>  - If host-trust-limitation is not set, guest's might still be able to
>    enter PV mode, if the CPU has the right model.  This may be a
>    little surprising, but shouldn't actually be harmful.

This could be workable, I guess. Would like a second opinion, though.

> 
> To start a guest supporting Protected Virtualization using the new
> option use the command line arguments:
>     -object s390-pv-guest,id=pv0 -machine host-trust-limitation=pv0
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/s390x/pv.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index ab3a2482aa..4bf3b345b6 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -14,8 +14,11 @@
>  #include <linux/kvm.h>
>  
>  #include "cpu.h"
> +#include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/kvm.h"
> +#include "qom/object_interfaces.h"
> +#include "exec/host-trust-limitation.h"
>  #include "hw/s390x/ipl.h"
>  #include "hw/s390x/pv.h"
>  
> @@ -111,3 +114,61 @@ void s390_pv_inject_reset_error(CPUState *cs)
>      /* Report that we are unable to enter protected mode */
>      env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
>  }
> +
> +#define TYPE_S390_PV_GUEST "s390-pv-guest"
> +#define S390_PV_GUEST(obj)                              \
> +    OBJECT_CHECK(S390PVGuestState, (obj), TYPE_S390_PV_GUEST)
> +
> +typedef struct S390PVGuestState S390PVGuestState;
> +
> +/**
> + * S390PVGuestState:
> + *
> + * The S390PVGuestState object is basically a dummy used to tell the
> + * host trust limitation system to use s390's PV mechanism.  guest.
> + *
> + * # $QEMU \
> + *         -object s390-pv-guest,id=pv0 \
> + *         -machine ...,host-trust-limitation=pv0
> + */
> +struct S390PVGuestState {
> +    Object parent_obj;
> +};
> +
> +static int s390_pv_kvm_init(HostTrustLimitation *gmpo, Error **errp)
> +{
> +    if (!s390_has_feat(S390_FEAT_UNPACK)) {
> +        error_setg(errp,
> +                   "CPU model does not support Protected Virtualization");
> +        return -1;
> +    }
> +
> +    return 0;
> +}

So here's where I'm confused: If I follow the code correctly, the
->kvm_init callback is invoked before kvm_arch_init() is called. The
kvm_arch_init() implementation for s390x checks whether
KVM_CAP_S390_PROTECTED is available, which is a pre-req for
S390_FEAT_UNPACK. Am I missing something? Can someone with access to PV
hardware check whether this works as intended?

> +
> +static void s390_pv_guest_class_init(ObjectClass *oc, void *data)
> +{
> +    HostTrustLimitationClass *gmpc = HOST_TRUST_LIMITATION_CLASS(oc);
> +
> +    gmpc->kvm_init = s390_pv_kvm_init;
> +}
> +
> +static const TypeInfo s390_pv_guest_info = {
> +    .parent = TYPE_OBJECT,
> +    .name = TYPE_S390_PV_GUEST,
> +    .instance_size = sizeof(S390PVGuestState),
> +    .class_init = s390_pv_guest_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOST_TRUST_LIMITATION },
> +        { TYPE_USER_CREATABLE },
> +        { }
> +    }
> +};
> +
> +static void
> +s390_pv_register_types(void)
> +{
> +    type_register_static(&s390_pv_guest_info);
> +}
> +
> +type_init(s390_pv_register_types);
Janosch Frank Aug. 3, 2020, 7:40 a.m. UTC | #2
On 7/27/20 5:50 PM, Cornelia Huck wrote:
> On Fri, 24 Jul 2020 12:57:44 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> At least some s390 cpu models support "Protected Virtualization" (PV),
>> a mechanism to protect guests from eavesdropping by a compromised
>> hypervisor.
>>
>> This is similar in function to other mechanisms like AMD's SEV and
>> POWER's PEF, which are controlled bythe "host-trust-limitation"
>> machine option.  s390 is a slightly special case, because we already
>> supported PV, simply by using a CPU model with the required feature
>> (S390_FEAT_UNPACK).
>>
>> To integrate this with the option used by other platforms, we
>> implement the following compromise:
>>
>>  - When the host-trust-limitation option is set, s390 will recognize
>>    it, verify that the CPU can support PV (failing if not) and set
>>    virtio default options necessary for encrypted or protected guests,
>>    as on other platforms.  i.e. if host-trust-limitation is set, we
>>    will either create a guest capable of entering PV mode, or fail
>>    outright
>>
>>  - If host-trust-limitation is not set, guest's might still be able to
>>    enter PV mode, if the CPU has the right model.  This may be a
>>    little surprising, but shouldn't actually be harmful.
> 
> This could be workable, I guess. Would like a second opinion, though.
> 
>>
>> To start a guest supporting Protected Virtualization using the new
>> option use the command line arguments:
>>     -object s390-pv-guest,id=pv0 -machine host-trust-limitation=pv0
>>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  hw/s390x/pv.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>>
>> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
>> index ab3a2482aa..4bf3b345b6 100644
>> --- a/hw/s390x/pv.c
>> +++ b/hw/s390x/pv.c
>> @@ -14,8 +14,11 @@
>>  #include <linux/kvm.h>
>>  
>>  #include "cpu.h"
>> +#include "qapi/error.h"
>>  #include "qemu/error-report.h"
>>  #include "sysemu/kvm.h"
>> +#include "qom/object_interfaces.h"
>> +#include "exec/host-trust-limitation.h"
>>  #include "hw/s390x/ipl.h"
>>  #include "hw/s390x/pv.h"
>>  
>> @@ -111,3 +114,61 @@ void s390_pv_inject_reset_error(CPUState *cs)
>>      /* Report that we are unable to enter protected mode */
>>      env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
>>  }
>> +
>> +#define TYPE_S390_PV_GUEST "s390-pv-guest"
>> +#define S390_PV_GUEST(obj)                              \
>> +    OBJECT_CHECK(S390PVGuestState, (obj), TYPE_S390_PV_GUEST)
>> +
>> +typedef struct S390PVGuestState S390PVGuestState;
>> +
>> +/**
>> + * S390PVGuestState:
>> + *
>> + * The S390PVGuestState object is basically a dummy used to tell the
>> + * host trust limitation system to use s390's PV mechanism.  guest.
>> + *
>> + * # $QEMU \
>> + *         -object s390-pv-guest,id=pv0 \
>> + *         -machine ...,host-trust-limitation=pv0
>> + */
>> +struct S390PVGuestState {
>> +    Object parent_obj;
>> +};
>> +
>> +static int s390_pv_kvm_init(HostTrustLimitation *gmpo, Error **errp)
>> +{
>> +    if (!s390_has_feat(S390_FEAT_UNPACK)) {
>> +        error_setg(errp,
>> +                   "CPU model does not support Protected Virtualization");
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
> 
> So here's where I'm confused: If I follow the code correctly, the
> ->kvm_init callback is invoked before kvm_arch_init() is called. The
> kvm_arch_init() implementation for s390x checks whether
> KVM_CAP_S390_PROTECTED is available, which is a pre-req for
> S390_FEAT_UNPACK. Am I missing something? Can someone with access to PV
> hardware check whether this works as intended?

Doesn't look good:

./s390x-run s390x/stsi.img -object s390-pv-guest,id=pv0 -machine
host-trust-limitation=pv0
/usr/local/bin/qemu-system-s390x -nodefaults -nographic -machine
s390-ccw-virtio,accel=kvm -chardev stdio,id=con0 -device
sclpconsole,chardev=con0 -kernel s390x/stsi.img -object
s390-pv-guest,id=pv0 -machine host-trust-limitation=pv0 # -initrd
/tmp/tmp.uacr85fJnw
qemu-system-s390x: CPU model does not support Protected Virtualization
qemu-system-s390x: failed to initialize kvm: Operation not permitted


Without the htl it's happy.

> 
>> +
>> +static void s390_pv_guest_class_init(ObjectClass *oc, void *data)
>> +{
>> +    HostTrustLimitationClass *gmpc = HOST_TRUST_LIMITATION_CLASS(oc);
>> +
>> +    gmpc->kvm_init = s390_pv_kvm_init;
>> +}
>> +
>> +static const TypeInfo s390_pv_guest_info = {
>> +    .parent = TYPE_OBJECT,
>> +    .name = TYPE_S390_PV_GUEST,
>> +    .instance_size = sizeof(S390PVGuestState),
>> +    .class_init = s390_pv_guest_class_init,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_HOST_TRUST_LIMITATION },
>> +        { TYPE_USER_CREATABLE },
>> +        { }
>> +    }
>> +};
>> +
>> +static void
>> +s390_pv_register_types(void)
>> +{
>> +    type_register_static(&s390_pv_guest_info);
>> +}
>> +
>> +type_init(s390_pv_register_types);
>
Janosch Frank Aug. 3, 2020, 7:49 a.m. UTC | #3
On 7/24/20 4:57 AM, David Gibson wrote:
> At least some s390 cpu models support "Protected Virtualization" (PV),
> a mechanism to protect guests from eavesdropping by a compromised
> hypervisor.
> 
> This is similar in function to other mechanisms like AMD's SEV and
> POWER's PEF, which are controlled bythe "host-trust-limitation"
> machine option.  s390 is a slightly special case, because we already
> supported PV, simply by using a CPU model with the required feature
> (S390_FEAT_UNPACK).
> 
> To integrate this with the option used by other platforms, we
> implement the following compromise:
> 
>  - When the host-trust-limitation option is set, s390 will recognize
>    it, verify that the CPU can support PV (failing if not) and set
>    virtio default options necessary for encrypted or protected guests,
>    as on other platforms.  i.e. if host-trust-limitation is set, we
>    will either create a guest capable of entering PV mode, or fail
>    outright
> 
>  - If host-trust-limitation is not set, guest's might still be able to
>    enter PV mode, if the CPU has the right model.  This may be a
>    little surprising, but shouldn't actually be harmful.

As I already explained, they have to continue to work without any change
to the VM's configuration.

Our users already expect PV to work without HTL. This feature is already
being used and the documentation has been online for a few months. I've
already heard enough complains because users found small errors in our
documentation. I'm not looking forward to complains because suddenly we
need to specify new command line arguments depending on the QEMU version.

@Cornelia: QEMU is not my expertise, am I missing something here?

> 
> To start a guest supporting Protected Virtualization using the new
> option use the command line arguments:
>     -object s390-pv-guest,id=pv0 -machine host-trust-limitation=pv0
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/s390x/pv.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index ab3a2482aa..4bf3b345b6 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -14,8 +14,11 @@
>  #include <linux/kvm.h>
>  
>  #include "cpu.h"
> +#include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/kvm.h"
> +#include "qom/object_interfaces.h"
> +#include "exec/host-trust-limitation.h"
>  #include "hw/s390x/ipl.h"
>  #include "hw/s390x/pv.h"
>  
> @@ -111,3 +114,61 @@ void s390_pv_inject_reset_error(CPUState *cs)
>      /* Report that we are unable to enter protected mode */
>      env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
>  }
> +
> +#define TYPE_S390_PV_GUEST "s390-pv-guest"
> +#define S390_PV_GUEST(obj)                              \
> +    OBJECT_CHECK(S390PVGuestState, (obj), TYPE_S390_PV_GUEST)
> +
> +typedef struct S390PVGuestState S390PVGuestState;
> +
> +/**
> + * S390PVGuestState:
> + *
> + * The S390PVGuestState object is basically a dummy used to tell the
> + * host trust limitation system to use s390's PV mechanism.  guest.
> + *
> + * # $QEMU \
> + *         -object s390-pv-guest,id=pv0 \
> + *         -machine ...,host-trust-limitation=pv0
> + */
> +struct S390PVGuestState {
> +    Object parent_obj;
> +};
> +
> +static int s390_pv_kvm_init(HostTrustLimitation *gmpo, Error **errp)
> +{
> +    if (!s390_has_feat(S390_FEAT_UNPACK)) {
> +        error_setg(errp,
> +                   "CPU model does not support Protected Virtualization");
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static void s390_pv_guest_class_init(ObjectClass *oc, void *data)
> +{
> +    HostTrustLimitationClass *gmpc = HOST_TRUST_LIMITATION_CLASS(oc);
> +
> +    gmpc->kvm_init = s390_pv_kvm_init;
> +}
> +
> +static const TypeInfo s390_pv_guest_info = {
> +    .parent = TYPE_OBJECT,
> +    .name = TYPE_S390_PV_GUEST,
> +    .instance_size = sizeof(S390PVGuestState),
> +    .class_init = s390_pv_guest_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOST_TRUST_LIMITATION },
> +        { TYPE_USER_CREATABLE },
> +        { }
> +    }
> +};
> +
> +static void
> +s390_pv_register_types(void)
> +{
> +    type_register_static(&s390_pv_guest_info);
> +}
> +
> +type_init(s390_pv_register_types);
>
David Gibson Aug. 3, 2020, 7:54 a.m. UTC | #4
On Mon, Aug 03, 2020 at 09:49:42AM +0200, Janosch Frank wrote:
> On 7/24/20 4:57 AM, David Gibson wrote:
> > At least some s390 cpu models support "Protected Virtualization" (PV),
> > a mechanism to protect guests from eavesdropping by a compromised
> > hypervisor.
> > 
> > This is similar in function to other mechanisms like AMD's SEV and
> > POWER's PEF, which are controlled bythe "host-trust-limitation"
> > machine option.  s390 is a slightly special case, because we already
> > supported PV, simply by using a CPU model with the required feature
> > (S390_FEAT_UNPACK).
> > 
> > To integrate this with the option used by other platforms, we
> > implement the following compromise:
> > 
> >  - When the host-trust-limitation option is set, s390 will recognize
> >    it, verify that the CPU can support PV (failing if not) and set
> >    virtio default options necessary for encrypted or protected guests,
> >    as on other platforms.  i.e. if host-trust-limitation is set, we
> >    will either create a guest capable of entering PV mode, or fail
> >    outright
> > 
> >  - If host-trust-limitation is not set, guest's might still be able to
> >    enter PV mode, if the CPU has the right model.  This may be a
> >    little surprising, but shouldn't actually be harmful.
> 
> As I already explained, they have to continue to work without any change
> to the VM's configuration.

Yes.. that's what I'm saying will happen.

> Our users already expect PV to work without HTL. This feature is already
> being used and the documentation has been online for a few months. I've
> already heard enough complains because users found small errors in our
> documentation. I'm not looking forward to complains because suddenly we
> need to specify new command line arguments depending on the QEMU version.
> 
> @Cornelia: QEMU is not my expertise, am I missing something here?

What I'm saying here is that you don't need a new option.  I'm only
suggesting we make the new option the preferred way for future
upstream releases.  (the new option has the advantage that you *just*
need to specify it, and any necessary virtio or other options to be
compatible should be handled for you).

But existing configurations should work as is (I'm not sure they do
with the current patch, because I'm not familiar with the s390 code
and have no means to test PV, but that can be sorted out before
merge).
Janosch Frank Aug. 3, 2020, 8:07 a.m. UTC | #5
On 8/3/20 9:54 AM, David Gibson wrote:
> On Mon, Aug 03, 2020 at 09:49:42AM +0200, Janosch Frank wrote:
>> On 7/24/20 4:57 AM, David Gibson wrote:
>>> At least some s390 cpu models support "Protected Virtualization" (PV),
>>> a mechanism to protect guests from eavesdropping by a compromised
>>> hypervisor.
>>>
>>> This is similar in function to other mechanisms like AMD's SEV and
>>> POWER's PEF, which are controlled bythe "host-trust-limitation"
>>> machine option.  s390 is a slightly special case, because we already
>>> supported PV, simply by using a CPU model with the required feature
>>> (S390_FEAT_UNPACK).
>>>
>>> To integrate this with the option used by other platforms, we
>>> implement the following compromise:
>>>
>>>  - When the host-trust-limitation option is set, s390 will recognize
>>>    it, verify that the CPU can support PV (failing if not) and set
>>>    virtio default options necessary for encrypted or protected guests,
>>>    as on other platforms.  i.e. if host-trust-limitation is set, we
>>>    will either create a guest capable of entering PV mode, or fail
>>>    outright
>>>
>>>  - If host-trust-limitation is not set, guest's might still be able to
>>>    enter PV mode, if the CPU has the right model.  This may be a
>>>    little surprising, but shouldn't actually be harmful.
>>
>> As I already explained, they have to continue to work without any change
>> to the VM's configuration.
> 
> Yes.. that's what I'm saying will happen.
> 
>> Our users already expect PV to work without HTL. This feature is already
>> being used and the documentation has been online for a few months. I've
>> already heard enough complains because users found small errors in our
>> documentation. I'm not looking forward to complains because suddenly we
>> need to specify new command line arguments depending on the QEMU version.
>>
>> @Cornelia: QEMU is not my expertise, am I missing something here?
> 
> What I'm saying here is that you don't need a new option.  I'm only
> suggesting we make the new option the preferred way for future
> upstream releases.  (the new option has the advantage that you *just*
> need to specify it, and any necessary virtio or other options to be
> compatible should be handled for you).
> 
> But existing configurations should work as is (I'm not sure they do
> with the current patch, because I'm not familiar with the s390 code
> and have no means to test PV, but that can be sorted out before
> merge).
> 
OK, should and might are two different things so I was a bit concerned.
That's fine then, thanks for the answer.
David Gibson Aug. 3, 2020, 8:14 a.m. UTC | #6
On Mon, Aug 03, 2020 at 10:07:42AM +0200, Janosch Frank wrote:
> On 8/3/20 9:54 AM, David Gibson wrote:
> > On Mon, Aug 03, 2020 at 09:49:42AM +0200, Janosch Frank wrote:
> >> On 7/24/20 4:57 AM, David Gibson wrote:
> >>> At least some s390 cpu models support "Protected Virtualization" (PV),
> >>> a mechanism to protect guests from eavesdropping by a compromised
> >>> hypervisor.
> >>>
> >>> This is similar in function to other mechanisms like AMD's SEV and
> >>> POWER's PEF, which are controlled bythe "host-trust-limitation"
> >>> machine option.  s390 is a slightly special case, because we already
> >>> supported PV, simply by using a CPU model with the required feature
> >>> (S390_FEAT_UNPACK).
> >>>
> >>> To integrate this with the option used by other platforms, we
> >>> implement the following compromise:
> >>>
> >>>  - When the host-trust-limitation option is set, s390 will recognize
> >>>    it, verify that the CPU can support PV (failing if not) and set
> >>>    virtio default options necessary for encrypted or protected guests,
> >>>    as on other platforms.  i.e. if host-trust-limitation is set, we
> >>>    will either create a guest capable of entering PV mode, or fail
> >>>    outright
> >>>
> >>>  - If host-trust-limitation is not set, guest's might still be able to
> >>>    enter PV mode, if the CPU has the right model.  This may be a
> >>>    little surprising, but shouldn't actually be harmful.
> >>
> >> As I already explained, they have to continue to work without any change
> >> to the VM's configuration.
> > 
> > Yes.. that's what I'm saying will happen.
> > 
> >> Our users already expect PV to work without HTL. This feature is already
> >> being used and the documentation has been online for a few months. I've
> >> already heard enough complains because users found small errors in our
> >> documentation. I'm not looking forward to complains because suddenly we
> >> need to specify new command line arguments depending on the QEMU version.
> >>
> >> @Cornelia: QEMU is not my expertise, am I missing something here?
> > 
> > What I'm saying here is that you don't need a new option.  I'm only
> > suggesting we make the new option the preferred way for future
> > upstream releases.  (the new option has the advantage that you *just*
> > need to specify it, and any necessary virtio or other options to be
> > compatible should be handled for you).
> > 
> > But existing configurations should work as is (I'm not sure they do
> > with the current patch, because I'm not familiar with the s390 code
> > and have no means to test PV, but that can be sorted out before
> > merge).
> > 
> OK, should and might are two different things so I was a bit concerned.
> That's fine then, thanks for the answer.

Well, the "should" and "might" are covering different things.
Existing working command lines should continue to work.  But those
command lines must already have the necessary tweaks to make virtio
work properly.  If you try to make a new command line for a PV guest
with a virtio device - or anything else that introduces extra PV
complications - then just chosing a CPU model with UNPACK might not be
enough.  By contrast, if you set host-trust-limitation, then it should
work and be PV capable with an arbitrary set of devices, or else fail
immediately with a meaningful error.
Cornelia Huck Aug. 3, 2020, 8:33 a.m. UTC | #7
On Mon, 3 Aug 2020 18:14:57 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Aug 03, 2020 at 10:07:42AM +0200, Janosch Frank wrote:
> > On 8/3/20 9:54 AM, David Gibson wrote:  
> > > On Mon, Aug 03, 2020 at 09:49:42AM +0200, Janosch Frank wrote:  
> > >> On 7/24/20 4:57 AM, David Gibson wrote:  
> > >>> At least some s390 cpu models support "Protected Virtualization" (PV),
> > >>> a mechanism to protect guests from eavesdropping by a compromised
> > >>> hypervisor.
> > >>>
> > >>> This is similar in function to other mechanisms like AMD's SEV and
> > >>> POWER's PEF, which are controlled bythe "host-trust-limitation"
> > >>> machine option.  s390 is a slightly special case, because we already
> > >>> supported PV, simply by using a CPU model with the required feature
> > >>> (S390_FEAT_UNPACK).
> > >>>
> > >>> To integrate this with the option used by other platforms, we
> > >>> implement the following compromise:
> > >>>
> > >>>  - When the host-trust-limitation option is set, s390 will recognize
> > >>>    it, verify that the CPU can support PV (failing if not) and set
> > >>>    virtio default options necessary for encrypted or protected guests,
> > >>>    as on other platforms.  i.e. if host-trust-limitation is set, we
> > >>>    will either create a guest capable of entering PV mode, or fail
> > >>>    outright
> > >>>
> > >>>  - If host-trust-limitation is not set, guest's might still be able to
> > >>>    enter PV mode, if the CPU has the right model.  This may be a
> > >>>    little surprising, but shouldn't actually be harmful.  
> > >>
> > >> As I already explained, they have to continue to work without any change
> > >> to the VM's configuration.  
> > > 
> > > Yes.. that's what I'm saying will happen.
> > >   
> > >> Our users already expect PV to work without HTL. This feature is already
> > >> being used and the documentation has been online for a few months. I've
> > >> already heard enough complains because users found small errors in our
> > >> documentation. I'm not looking forward to complains because suddenly we
> > >> need to specify new command line arguments depending on the QEMU version.
> > >>
> > >> @Cornelia: QEMU is not my expertise, am I missing something here?  
> > > 
> > > What I'm saying here is that you don't need a new option.  I'm only
> > > suggesting we make the new option the preferred way for future
> > > upstream releases.  (the new option has the advantage that you *just*
> > > need to specify it, and any necessary virtio or other options to be
> > > compatible should be handled for you).
> > > 
> > > But existing configurations should work as is (I'm not sure they do
> > > with the current patch, because I'm not familiar with the s390 code
> > > and have no means to test PV, but that can be sorted out before
> > > merge).
> > >   
> > OK, should and might are two different things so I was a bit concerned.
> > That's fine then, thanks for the answer.  
> 
> Well, the "should" and "might" are covering different things.
> Existing working command lines should continue to work.  But those
> command lines must already have the necessary tweaks to make virtio
> work properly.  If you try to make a new command line for a PV guest
> with a virtio device - or anything else that introduces extra PV
> complications - then just chosing a CPU model with UNPACK might not be
> enough.  By contrast, if you set host-trust-limitation, then it should
> work and be PV capable with an arbitrary set of devices, or else fail
> immediately with a meaningful error.

Yes, that was also my understanding.

Getting the interaction with the cpu model right seems to be the tricky
part, though. The UNPACK feature would only be set automatically
_after_ the htl device has already checked for it...
David Gibson Aug. 6, 2020, 6:14 a.m. UTC | #8
On Mon, Jul 27, 2020 at 05:50:40PM +0200, Cornelia Huck wrote:
> On Fri, 24 Jul 2020 12:57:44 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > At least some s390 cpu models support "Protected Virtualization" (PV),
> > a mechanism to protect guests from eavesdropping by a compromised
> > hypervisor.
> > 
> > This is similar in function to other mechanisms like AMD's SEV and
> > POWER's PEF, which are controlled bythe "host-trust-limitation"
> > machine option.  s390 is a slightly special case, because we already
> > supported PV, simply by using a CPU model with the required feature
> > (S390_FEAT_UNPACK).
> > 
> > To integrate this with the option used by other platforms, we
> > implement the following compromise:
> > 
> >  - When the host-trust-limitation option is set, s390 will recognize
> >    it, verify that the CPU can support PV (failing if not) and set
> >    virtio default options necessary for encrypted or protected guests,
> >    as on other platforms.  i.e. if host-trust-limitation is set, we
> >    will either create a guest capable of entering PV mode, or fail
> >    outright
> > 
> >  - If host-trust-limitation is not set, guest's might still be able to
> >    enter PV mode, if the CPU has the right model.  This may be a
> >    little surprising, but shouldn't actually be harmful.
> 
> This could be workable, I guess. Would like a second opinion, though.
> 
> > 
> > To start a guest supporting Protected Virtualization using the new
> > option use the command line arguments:
> >     -object s390-pv-guest,id=pv0 -machine host-trust-limitation=pv0
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/s390x/pv.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> > 
> > diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> > index ab3a2482aa..4bf3b345b6 100644
> > --- a/hw/s390x/pv.c
> > +++ b/hw/s390x/pv.c
> > @@ -14,8 +14,11 @@
> >  #include <linux/kvm.h>
> >  
> >  #include "cpu.h"
> > +#include "qapi/error.h"
> >  #include "qemu/error-report.h"
> >  #include "sysemu/kvm.h"
> > +#include "qom/object_interfaces.h"
> > +#include "exec/host-trust-limitation.h"
> >  #include "hw/s390x/ipl.h"
> >  #include "hw/s390x/pv.h"
> >  
> > @@ -111,3 +114,61 @@ void s390_pv_inject_reset_error(CPUState *cs)
> >      /* Report that we are unable to enter protected mode */
> >      env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
> >  }
> > +
> > +#define TYPE_S390_PV_GUEST "s390-pv-guest"
> > +#define S390_PV_GUEST(obj)                              \
> > +    OBJECT_CHECK(S390PVGuestState, (obj), TYPE_S390_PV_GUEST)
> > +
> > +typedef struct S390PVGuestState S390PVGuestState;
> > +
> > +/**
> > + * S390PVGuestState:
> > + *
> > + * The S390PVGuestState object is basically a dummy used to tell the
> > + * host trust limitation system to use s390's PV mechanism.  guest.
> > + *
> > + * # $QEMU \
> > + *         -object s390-pv-guest,id=pv0 \
> > + *         -machine ...,host-trust-limitation=pv0
> > + */
> > +struct S390PVGuestState {
> > +    Object parent_obj;
> > +};
> > +
> > +static int s390_pv_kvm_init(HostTrustLimitation *gmpo, Error **errp)
> > +{
> > +    if (!s390_has_feat(S390_FEAT_UNPACK)) {
> > +        error_setg(errp,
> > +                   "CPU model does not support Protected Virtualization");
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> 
> So here's where I'm confused: If I follow the code correctly, the
> ->kvm_init callback is invoked before kvm_arch_init() is called. The
> kvm_arch_init() implementation for s390x checks whether
> KVM_CAP_S390_PROTECTED is available, which is a pre-req for
> S390_FEAT_UNPACK. Am I missing something? Can someone with access to PV
> hardware check whether this works as intended?

Ah, yes, I need to rethink this.  kvm_arch_init() happens
substantially earlier than I realized.  Plus the setup of s390 cpu
models is confusing to me, it seems to set up the model after the cpu
instance is created, rather than having cpu models correspond to cpu
classes and thus existing before the cpus are actually instantiated.
David Hildenbrand Aug. 6, 2020, 7:18 a.m. UTC | #9
On 06.08.20 08:14, David Gibson wrote:
> On Mon, Jul 27, 2020 at 05:50:40PM +0200, Cornelia Huck wrote:
>> On Fri, 24 Jul 2020 12:57:44 +1000
>> David Gibson <david@gibson.dropbear.id.au> wrote:
>>
>>> At least some s390 cpu models support "Protected Virtualization" (PV),
>>> a mechanism to protect guests from eavesdropping by a compromised
>>> hypervisor.
>>>
>>> This is similar in function to other mechanisms like AMD's SEV and
>>> POWER's PEF, which are controlled bythe "host-trust-limitation"
>>> machine option.  s390 is a slightly special case, because we already
>>> supported PV, simply by using a CPU model with the required feature
>>> (S390_FEAT_UNPACK).
>>>
>>> To integrate this with the option used by other platforms, we
>>> implement the following compromise:
>>>
>>>  - When the host-trust-limitation option is set, s390 will recognize
>>>    it, verify that the CPU can support PV (failing if not) and set
>>>    virtio default options necessary for encrypted or protected guests,
>>>    as on other platforms.  i.e. if host-trust-limitation is set, we
>>>    will either create a guest capable of entering PV mode, or fail
>>>    outright
>>>
>>>  - If host-trust-limitation is not set, guest's might still be able to
>>>    enter PV mode, if the CPU has the right model.  This may be a
>>>    little surprising, but shouldn't actually be harmful.
>>
>> This could be workable, I guess. Would like a second opinion, though.
>>
>>>
>>> To start a guest supporting Protected Virtualization using the new
>>> option use the command line arguments:
>>>     -object s390-pv-guest,id=pv0 -machine host-trust-limitation=pv0
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>>  hw/s390x/pv.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 61 insertions(+)
>>>
>>> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
>>> index ab3a2482aa..4bf3b345b6 100644
>>> --- a/hw/s390x/pv.c
>>> +++ b/hw/s390x/pv.c
>>> @@ -14,8 +14,11 @@
>>>  #include <linux/kvm.h>
>>>  
>>>  #include "cpu.h"
>>> +#include "qapi/error.h"
>>>  #include "qemu/error-report.h"
>>>  #include "sysemu/kvm.h"
>>> +#include "qom/object_interfaces.h"
>>> +#include "exec/host-trust-limitation.h"
>>>  #include "hw/s390x/ipl.h"
>>>  #include "hw/s390x/pv.h"
>>>  
>>> @@ -111,3 +114,61 @@ void s390_pv_inject_reset_error(CPUState *cs)
>>>      /* Report that we are unable to enter protected mode */
>>>      env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
>>>  }
>>> +
>>> +#define TYPE_S390_PV_GUEST "s390-pv-guest"
>>> +#define S390_PV_GUEST(obj)                              \
>>> +    OBJECT_CHECK(S390PVGuestState, (obj), TYPE_S390_PV_GUEST)
>>> +
>>> +typedef struct S390PVGuestState S390PVGuestState;
>>> +
>>> +/**
>>> + * S390PVGuestState:
>>> + *
>>> + * The S390PVGuestState object is basically a dummy used to tell the
>>> + * host trust limitation system to use s390's PV mechanism.  guest.
>>> + *
>>> + * # $QEMU \
>>> + *         -object s390-pv-guest,id=pv0 \
>>> + *         -machine ...,host-trust-limitation=pv0
>>> + */
>>> +struct S390PVGuestState {
>>> +    Object parent_obj;
>>> +};
>>> +
>>> +static int s390_pv_kvm_init(HostTrustLimitation *gmpo, Error **errp)
>>> +{
>>> +    if (!s390_has_feat(S390_FEAT_UNPACK)) {
>>> +        error_setg(errp,
>>> +                   "CPU model does not support Protected Virtualization");
>>> +        return -1;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>
>> So here's where I'm confused: If I follow the code correctly, the
>> ->kvm_init callback is invoked before kvm_arch_init() is called. The
>> kvm_arch_init() implementation for s390x checks whether
>> KVM_CAP_S390_PROTECTED is available, which is a pre-req for
>> S390_FEAT_UNPACK. Am I missing something? Can someone with access to PV
>> hardware check whether this works as intended?
> 
> Ah, yes, I need to rethink this.  kvm_arch_init() happens
> substantially earlier than I realized.  Plus the setup of s390 cpu
> models is confusing to me, it seems to set up the model after the cpu
> instance is created, rather than having cpu models correspond to cpu
> classes and thus existing before the cpus are actually instantiated.

The class only contains the cpu definition, the instance contains the
actual model. A definition is static and immutable at runtime (e.g., a
z15 with all possible features), a model is a variation of that (e.g.,
enable/disable features, e.g., a z15 with features available in the
current configuration).

We initialize the actual model in instance_init() (so during init, not
after init), where we create and initialize cpu->model, based on the cpu
definition in the class information (xcc->cpu_def).

In case of qemu/host/max model, we have to construct the cpu model at
init time, because the cpu model does not match an exact cpu definition
we have at hand.

So whenever we init a cpu, we already have to have kvm running such that
we can query the actual host/max model to construct cpu->model.
Halil Pasic Sept. 7, 2020, 3:22 p.m. UTC | #10
On Fri, 24 Jul 2020 12:57:44 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> At least some s390 cpu models support "Protected Virtualization" (PV),
> a mechanism to protect guests from eavesdropping by a compromised
> hypervisor.
> 
> This is similar in function to other mechanisms like AMD's SEV and
> POWER's PEF, which are controlled bythe "host-trust-limitation"
> machine option.  s390 is a slightly special case, because we already
> supported PV, simply by using a CPU model with the required feature
> (S390_FEAT_UNPACK).
> 
> To integrate this with the option used by other platforms, we
> implement the following compromise:
> 
>  - When the host-trust-limitation option is set, s390 will recognize
>    it, verify that the CPU can support PV (failing if not) and set
>    virtio default options necessary for encrypted or protected guests,
>    as on other platforms.  i.e. if host-trust-limitation is set, we
>    will either create a guest capable of entering PV mode, or fail
>    outright

Shouldn't we also fail outright if the virtio features are not PV
compatible (invalid configuration)?

I would like to see something like follows as a part of this series.
----------------------------8<--------------------------
From: Halil Pasic <pasic@linux.ibm.com>
Date: Mon, 7 Sep 2020 15:00:17 +0200
Subject: [PATCH] virtio: handle host trust limitation

If host_trust_limitation_enabled() returns true, then emulated virtio
devices must offer VIRTIO_F_ACCESS_PLATFORM, because the device is not
capable of accessing all of the guest memory. Otherwise we are in
violation of the virtio specification.

Let's fail realize if we detect that VIRTIO_F_ACCESS_PLATFORM feature is
obligatory but missing.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
 hw/virtio/virtio.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5bd2a2f621..19b4b0a37a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -27,6 +27,7 @@
 #include "hw/virtio/virtio-access.h"
 #include "sysemu/dma.h"
 #include "sysemu/runstate.h"
+#include "exec/host-trust-limitation.h"
 
 /*
  * The alignment to use between consumer and producer parts of vring.
@@ -3618,6 +3619,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
     /* Devices should either use vmsd or the load/save methods */
     assert(!vdc->vmsd || !vdc->load);
 
+    if (host_trust_limitation_enabled(MACHINE(qdev_get_machine()))
+        && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
+        error_setg(&err, "devices without VIRTIO_F_ACCESS_PLATFORM are not compatible with host trust imitation");
+        error_propagate(errp, err);
+        return;
+    }
     if (vdc->realize != NULL) {
         vdc->realize(dev, &err);
         if (err != NULL) {
Cornelia Huck Sept. 10, 2020, 11:36 a.m. UTC | #11
On Mon, 7 Sep 2020 17:22:53 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Fri, 24 Jul 2020 12:57:44 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > At least some s390 cpu models support "Protected Virtualization" (PV),
> > a mechanism to protect guests from eavesdropping by a compromised
> > hypervisor.
> > 
> > This is similar in function to other mechanisms like AMD's SEV and
> > POWER's PEF, which are controlled bythe "host-trust-limitation"
> > machine option.  s390 is a slightly special case, because we already
> > supported PV, simply by using a CPU model with the required feature
> > (S390_FEAT_UNPACK).
> > 
> > To integrate this with the option used by other platforms, we
> > implement the following compromise:
> > 
> >  - When the host-trust-limitation option is set, s390 will recognize
> >    it, verify that the CPU can support PV (failing if not) and set
> >    virtio default options necessary for encrypted or protected guests,
> >    as on other platforms.  i.e. if host-trust-limitation is set, we
> >    will either create a guest capable of entering PV mode, or fail
> >    outright  
> 
> Shouldn't we also fail outright if the virtio features are not PV
> compatible (invalid configuration)?
> 
> I would like to see something like follows as a part of this series.
> ----------------------------8<--------------------------
> From: Halil Pasic <pasic@linux.ibm.com>
> Date: Mon, 7 Sep 2020 15:00:17 +0200
> Subject: [PATCH] virtio: handle host trust limitation
> 
> If host_trust_limitation_enabled() returns true, then emulated virtio
> devices must offer VIRTIO_F_ACCESS_PLATFORM, because the device is not
> capable of accessing all of the guest memory. Otherwise we are in
> violation of the virtio specification.
> 
> Let's fail realize if we detect that VIRTIO_F_ACCESS_PLATFORM feature is
> obligatory but missing.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
>  hw/virtio/virtio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 5bd2a2f621..19b4b0a37a 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -27,6 +27,7 @@
>  #include "hw/virtio/virtio-access.h"
>  #include "sysemu/dma.h"
>  #include "sysemu/runstate.h"
> +#include "exec/host-trust-limitation.h"
>  
>  /*
>   * The alignment to use between consumer and producer parts of vring.
> @@ -3618,6 +3619,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
>      /* Devices should either use vmsd or the load/save methods */
>      assert(!vdc->vmsd || !vdc->load);
>  
> +    if (host_trust_limitation_enabled(MACHINE(qdev_get_machine()))
> +        && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> +        error_setg(&err, "devices without VIRTIO_F_ACCESS_PLATFORM are not compatible with host trust imitation");
> +        error_propagate(errp, err);
> +        return;

How can we get here? I assume only if the user explicitly turned the
feature off while turning HTL on, as otherwise patch 9 should have
taken care of it?

> +    }
>      if (vdc->realize != NULL) {
>          vdc->realize(dev, &err);
>          if (err != NULL) {
Halil Pasic Sept. 10, 2020, 6:29 p.m. UTC | #12
On Thu, 10 Sep 2020 13:36:09 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, 7 Sep 2020 17:22:53 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Fri, 24 Jul 2020 12:57:44 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > At least some s390 cpu models support "Protected Virtualization" (PV),
> > > a mechanism to protect guests from eavesdropping by a compromised
> > > hypervisor.
> > > 
> > > This is similar in function to other mechanisms like AMD's SEV and
> > > POWER's PEF, which are controlled bythe "host-trust-limitation"
> > > machine option.  s390 is a slightly special case, because we already
> > > supported PV, simply by using a CPU model with the required feature
> > > (S390_FEAT_UNPACK).
> > > 
> > > To integrate this with the option used by other platforms, we
> > > implement the following compromise:
> > > 
> > >  - When the host-trust-limitation option is set, s390 will recognize
> > >    it, verify that the CPU can support PV (failing if not) and set
> > >    virtio default options necessary for encrypted or protected guests,
> > >    as on other platforms.  i.e. if host-trust-limitation is set, we
> > >    will either create a guest capable of entering PV mode, or fail
> > >    outright  
> > 
> > Shouldn't we also fail outright if the virtio features are not PV
> > compatible (invalid configuration)?
> > 
> > I would like to see something like follows as a part of this series.
> > ----------------------------8<--------------------------
> > From: Halil Pasic <pasic@linux.ibm.com>
> > Date: Mon, 7 Sep 2020 15:00:17 +0200
> > Subject: [PATCH] virtio: handle host trust limitation
> > 
> > If host_trust_limitation_enabled() returns true, then emulated virtio
> > devices must offer VIRTIO_F_ACCESS_PLATFORM, because the device is not
> > capable of accessing all of the guest memory. Otherwise we are in
> > violation of the virtio specification.
> > 
> > Let's fail realize if we detect that VIRTIO_F_ACCESS_PLATFORM feature is
> > obligatory but missing.
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > ---
> >  hw/virtio/virtio.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 5bd2a2f621..19b4b0a37a 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -27,6 +27,7 @@
> >  #include "hw/virtio/virtio-access.h"
> >  #include "sysemu/dma.h"
> >  #include "sysemu/runstate.h"
> > +#include "exec/host-trust-limitation.h"
> >  
> >  /*
> >   * The alignment to use between consumer and producer parts of vring.
> > @@ -3618,6 +3619,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
> >      /* Devices should either use vmsd or the load/save methods */
> >      assert(!vdc->vmsd || !vdc->load);
> >  
> > +    if (host_trust_limitation_enabled(MACHINE(qdev_get_machine()))
> > +        && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> > +        error_setg(&err, "devices without VIRTIO_F_ACCESS_PLATFORM are not compatible with host trust imitation");
> > +        error_propagate(errp, err);
> > +        return;
> 
> How can we get here? I assume only if the user explicitly turned the
> feature off while turning HTL on, as otherwise patch 9 should have
> taken care of it?
> 

Yes, we can get here only if iommu_platform is explicitly turned off.

Regards,
Halil
David Gibson Sept. 11, 2020, 12:07 a.m. UTC | #13
On Thu, Sep 10, 2020 at 08:29:24PM +0200, Halil Pasic wrote:
> On Thu, 10 Sep 2020 13:36:09 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Mon, 7 Sep 2020 17:22:53 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> > 
> > > On Fri, 24 Jul 2020 12:57:44 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > 
> > > > At least some s390 cpu models support "Protected Virtualization" (PV),
> > > > a mechanism to protect guests from eavesdropping by a compromised
> > > > hypervisor.
> > > > 
> > > > This is similar in function to other mechanisms like AMD's SEV and
> > > > POWER's PEF, which are controlled bythe "host-trust-limitation"
> > > > machine option.  s390 is a slightly special case, because we already
> > > > supported PV, simply by using a CPU model with the required feature
> > > > (S390_FEAT_UNPACK).
> > > > 
> > > > To integrate this with the option used by other platforms, we
> > > > implement the following compromise:
> > > > 
> > > >  - When the host-trust-limitation option is set, s390 will recognize
> > > >    it, verify that the CPU can support PV (failing if not) and set
> > > >    virtio default options necessary for encrypted or protected guests,
> > > >    as on other platforms.  i.e. if host-trust-limitation is set, we
> > > >    will either create a guest capable of entering PV mode, or fail
> > > >    outright  
> > > 
> > > Shouldn't we also fail outright if the virtio features are not PV
> > > compatible (invalid configuration)?
> > > 
> > > I would like to see something like follows as a part of this series.
> > > ----------------------------8<--------------------------
> > > From: Halil Pasic <pasic@linux.ibm.com>
> > > Date: Mon, 7 Sep 2020 15:00:17 +0200
> > > Subject: [PATCH] virtio: handle host trust limitation
> > > 
> > > If host_trust_limitation_enabled() returns true, then emulated virtio
> > > devices must offer VIRTIO_F_ACCESS_PLATFORM, because the device is not
> > > capable of accessing all of the guest memory. Otherwise we are in
> > > violation of the virtio specification.
> > > 
> > > Let's fail realize if we detect that VIRTIO_F_ACCESS_PLATFORM feature is
> > > obligatory but missing.
> > > 
> > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > > ---
> > >  hw/virtio/virtio.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 5bd2a2f621..19b4b0a37a 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -27,6 +27,7 @@
> > >  #include "hw/virtio/virtio-access.h"
> > >  #include "sysemu/dma.h"
> > >  #include "sysemu/runstate.h"
> > > +#include "exec/host-trust-limitation.h"
> > >  
> > >  /*
> > >   * The alignment to use between consumer and producer parts of vring.
> > > @@ -3618,6 +3619,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
> > >      /* Devices should either use vmsd or the load/save methods */
> > >      assert(!vdc->vmsd || !vdc->load);
> > >  
> > > +    if (host_trust_limitation_enabled(MACHINE(qdev_get_machine()))
> > > +        && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> > > +        error_setg(&err, "devices without VIRTIO_F_ACCESS_PLATFORM are not compatible with host trust imitation");
> > > +        error_propagate(errp, err);
> > > +        return;
> > 
> > How can we get here? I assume only if the user explicitly turned the
> > feature off while turning HTL on, as otherwise patch 9 should have
> > taken care of it?
> > 
> 
> Yes, we can get here only if iommu_platform is explicitly turned off.

Right.. my assumption was that if you really want to specify
contradictory options, you get to keep both pieces.  Or, more
seriously, there might be some weird experimental cases where this
combination could do something useful if you really know what you're
doing, and explicitly telling qemu to do this implies you know what
you're doing.
Greg Kurz Sept. 11, 2020, 6:25 a.m. UTC | #14
On Fri, 11 Sep 2020 10:07:18 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Sep 10, 2020 at 08:29:24PM +0200, Halil Pasic wrote:
> > On Thu, 10 Sep 2020 13:36:09 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > On Mon, 7 Sep 2020 17:22:53 +0200
> > > Halil Pasic <pasic@linux.ibm.com> wrote:
> > > 
> > > > On Fri, 24 Jul 2020 12:57:44 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > 
> > > > > At least some s390 cpu models support "Protected Virtualization" (PV),
> > > > > a mechanism to protect guests from eavesdropping by a compromised
> > > > > hypervisor.
> > > > > 
> > > > > This is similar in function to other mechanisms like AMD's SEV and
> > > > > POWER's PEF, which are controlled bythe "host-trust-limitation"
> > > > > machine option.  s390 is a slightly special case, because we already
> > > > > supported PV, simply by using a CPU model with the required feature
> > > > > (S390_FEAT_UNPACK).
> > > > > 
> > > > > To integrate this with the option used by other platforms, we
> > > > > implement the following compromise:
> > > > > 
> > > > >  - When the host-trust-limitation option is set, s390 will recognize
> > > > >    it, verify that the CPU can support PV (failing if not) and set
> > > > >    virtio default options necessary for encrypted or protected guests,
> > > > >    as on other platforms.  i.e. if host-trust-limitation is set, we
> > > > >    will either create a guest capable of entering PV mode, or fail
> > > > >    outright  
> > > > 
> > > > Shouldn't we also fail outright if the virtio features are not PV
> > > > compatible (invalid configuration)?
> > > > 
> > > > I would like to see something like follows as a part of this series.
> > > > ----------------------------8<--------------------------
> > > > From: Halil Pasic <pasic@linux.ibm.com>
> > > > Date: Mon, 7 Sep 2020 15:00:17 +0200
> > > > Subject: [PATCH] virtio: handle host trust limitation
> > > > 
> > > > If host_trust_limitation_enabled() returns true, then emulated virtio
> > > > devices must offer VIRTIO_F_ACCESS_PLATFORM, because the device is not
> > > > capable of accessing all of the guest memory. Otherwise we are in
> > > > violation of the virtio specification.
> > > > 
> > > > Let's fail realize if we detect that VIRTIO_F_ACCESS_PLATFORM feature is
> > > > obligatory but missing.
> > > > 
> > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > > > ---
> > > >  hw/virtio/virtio.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index 5bd2a2f621..19b4b0a37a 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -27,6 +27,7 @@
> > > >  #include "hw/virtio/virtio-access.h"
> > > >  #include "sysemu/dma.h"
> > > >  #include "sysemu/runstate.h"
> > > > +#include "exec/host-trust-limitation.h"
> > > >  
> > > >  /*
> > > >   * The alignment to use between consumer and producer parts of vring.
> > > > @@ -3618,6 +3619,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
> > > >      /* Devices should either use vmsd or the load/save methods */
> > > >      assert(!vdc->vmsd || !vdc->load);
> > > >  
> > > > +    if (host_trust_limitation_enabled(MACHINE(qdev_get_machine()))
> > > > +        && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> > > > +        error_setg(&err, "devices without VIRTIO_F_ACCESS_PLATFORM are not compatible with host trust imitation");
> > > > +        error_propagate(errp, err);
> > > > +        return;
> > > 
> > > How can we get here? I assume only if the user explicitly turned the
> > > feature off while turning HTL on, as otherwise patch 9 should have
> > > taken care of it?
> > > 
> > 
> > Yes, we can get here only if iommu_platform is explicitly turned off.
> 
> Right.. my assumption was that if you really want to specify
> contradictory options, you get to keep both pieces.  Or, more
> seriously, there might be some weird experimental cases where this
> combination could do something useful if you really know what you're
> doing, and explicitly telling qemu to do this implies you know what
> you're doing.
> 

I guess this deserves at least a warning for the case of someone that
doesn't really know what they're doing, eg. borrowing a complex QEMU
command line or libvirt XML from somewhere else ?
Halil Pasic Sept. 11, 2020, 12:45 p.m. UTC | #15
On Fri, 11 Sep 2020 10:07:18 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Sep 10, 2020 at 08:29:24PM +0200, Halil Pasic wrote:
> > On Thu, 10 Sep 2020 13:36:09 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > On Mon, 7 Sep 2020 17:22:53 +0200
> > > Halil Pasic <pasic@linux.ibm.com> wrote:
> > > 
> > > > On Fri, 24 Jul 2020 12:57:44 +1000
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > 
> > > > > At least some s390 cpu models support "Protected Virtualization" (PV),
> > > > > a mechanism to protect guests from eavesdropping by a compromised
> > > > > hypervisor.
> > > > > 
> > > > > This is similar in function to other mechanisms like AMD's SEV and
> > > > > POWER's PEF, which are controlled bythe "host-trust-limitation"
> > > > > machine option.  s390 is a slightly special case, because we already
> > > > > supported PV, simply by using a CPU model with the required feature
> > > > > (S390_FEAT_UNPACK).
> > > > > 
> > > > > To integrate this with the option used by other platforms, we
> > > > > implement the following compromise:
> > > > > 
> > > > >  - When the host-trust-limitation option is set, s390 will recognize
> > > > >    it, verify that the CPU can support PV (failing if not) and set
> > > > >    virtio default options necessary for encrypted or protected guests,
> > > > >    as on other platforms.  i.e. if host-trust-limitation is set, we
> > > > >    will either create a guest capable of entering PV mode, or fail
> > > > >    outright  
> > > > 
> > > > Shouldn't we also fail outright if the virtio features are not PV
> > > > compatible (invalid configuration)?
> > > > 
> > > > I would like to see something like follows as a part of this series.
> > > > ----------------------------8<--------------------------
> > > > From: Halil Pasic <pasic@linux.ibm.com>
> > > > Date: Mon, 7 Sep 2020 15:00:17 +0200
> > > > Subject: [PATCH] virtio: handle host trust limitation
> > > > 
> > > > If host_trust_limitation_enabled() returns true, then emulated virtio
> > > > devices must offer VIRTIO_F_ACCESS_PLATFORM, because the device is not
> > > > capable of accessing all of the guest memory. Otherwise we are in
> > > > violation of the virtio specification.
> > > > 
> > > > Let's fail realize if we detect that VIRTIO_F_ACCESS_PLATFORM feature is
> > > > obligatory but missing.
> > > > 
> > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > > > ---
> > > >  hw/virtio/virtio.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index 5bd2a2f621..19b4b0a37a 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -27,6 +27,7 @@
> > > >  #include "hw/virtio/virtio-access.h"
> > > >  #include "sysemu/dma.h"
> > > >  #include "sysemu/runstate.h"
> > > > +#include "exec/host-trust-limitation.h"
> > > >  
> > > >  /*
> > > >   * The alignment to use between consumer and producer parts of vring.
> > > > @@ -3618,6 +3619,12 @@ static void virtio_device_realize(DeviceState *dev, Error **errp)
> > > >      /* Devices should either use vmsd or the load/save methods */
> > > >      assert(!vdc->vmsd || !vdc->load);
> > > >  
> > > > +    if (host_trust_limitation_enabled(MACHINE(qdev_get_machine()))
> > > > +        && !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> > > > +        error_setg(&err, "devices without VIRTIO_F_ACCESS_PLATFORM are not compatible with host trust imitation");
> > > > +        error_propagate(errp, err);
> > > > +        return;
> > > 
> > > How can we get here? I assume only if the user explicitly turned the
> > > feature off while turning HTL on, as otherwise patch 9 should have
> > > taken care of it?
> > > 
> > 
> > Yes, we can get here only if iommu_platform is explicitly turned off.
> 
> Right.. my assumption was that if you really want to specify
> contradictory options, you get to keep both pieces.  Or, more
> seriously, there might be some weird experimental cases where this
> combination could do something useful if you really know what you're
> doing, and explicitly telling qemu to do this implies you know what
> you're doing.
> 

According to Michael, the correctness of a hypervisor is depending on
this (if device has restricted access to guest memory, but does not
present F_ACCESS_PLATFORM then the hypervisor is buggy).

Also a hotplug of such a misconfigured device is at the moment likely
bring down the guest on s390x.

The only scenario in which the guest has protected memory and the device
is able to access it, is a trusted HW device. But then we would need 
F_ACCESS_PLATFORM because it is a HW device. So I consider this combination
doing something useful very unlikely. Does anybody have a scenario where
this combination is legit and useful?

If such a scenario emerges later, I think the check can be refined or
dropped.

Regards,
Halil
diff mbox series

Patch

diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index ab3a2482aa..4bf3b345b6 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -14,8 +14,11 @@ 
 #include <linux/kvm.h>
 
 #include "cpu.h"
+#include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/kvm.h"
+#include "qom/object_interfaces.h"
+#include "exec/host-trust-limitation.h"
 #include "hw/s390x/ipl.h"
 #include "hw/s390x/pv.h"
 
@@ -111,3 +114,61 @@  void s390_pv_inject_reset_error(CPUState *cs)
     /* Report that we are unable to enter protected mode */
     env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;
 }
+
+#define TYPE_S390_PV_GUEST "s390-pv-guest"
+#define S390_PV_GUEST(obj)                              \
+    OBJECT_CHECK(S390PVGuestState, (obj), TYPE_S390_PV_GUEST)
+
+typedef struct S390PVGuestState S390PVGuestState;
+
+/**
+ * S390PVGuestState:
+ *
+ * The S390PVGuestState object is basically a dummy used to tell the
+ * host trust limitation system to use s390's PV mechanism.  guest.
+ *
+ * # $QEMU \
+ *         -object s390-pv-guest,id=pv0 \
+ *         -machine ...,host-trust-limitation=pv0
+ */
+struct S390PVGuestState {
+    Object parent_obj;
+};
+
+static int s390_pv_kvm_init(HostTrustLimitation *gmpo, Error **errp)
+{
+    if (!s390_has_feat(S390_FEAT_UNPACK)) {
+        error_setg(errp,
+                   "CPU model does not support Protected Virtualization");
+        return -1;
+    }
+
+    return 0;
+}
+
+static void s390_pv_guest_class_init(ObjectClass *oc, void *data)
+{
+    HostTrustLimitationClass *gmpc = HOST_TRUST_LIMITATION_CLASS(oc);
+
+    gmpc->kvm_init = s390_pv_kvm_init;
+}
+
+static const TypeInfo s390_pv_guest_info = {
+    .parent = TYPE_OBJECT,
+    .name = TYPE_S390_PV_GUEST,
+    .instance_size = sizeof(S390PVGuestState),
+    .class_init = s390_pv_guest_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOST_TRUST_LIMITATION },
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
+static void
+s390_pv_register_types(void)
+{
+    type_register_static(&s390_pv_guest_info);
+}
+
+type_init(s390_pv_register_types);