| Message ID | 20200724025744.69644-11-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generalize memory encryption models | expand |
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);
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); >
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); >
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).
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.
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.
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...
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.
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.
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) {
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) {
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
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.
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 ?
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 --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);
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(+)