Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
On 09.03.20 15:40, Christian Borntraeger wrote:
> something like the following?
>
>
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index c513f8efe0..cd12c29b9a 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -185,13 +185,18 @@ static void s390_cpu_disas_set_info(CPUState *cpu,
> disassemble_info *info)
> static bool machine_is_pv(MachineState *ms)
> {
> Object *obj;
> +static S390CcwMachineState *ccw;
> +
> +if (ccw)
> + return ccw->pv;
And I think we need curly braces: {}
Sorry. Was doing too many kernel things :-)
Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
>>
>>> +CPUS390XState *env;
>>>
>>> /* get the reset parameters, reset them once done */
>>> s390_ipl_get_reset_request(, _type);
>>> @@ -327,9 +411,16 @@ static void s390_machine_reset(MachineState *machine)
>>> /* all CPUs are paused and synchronized at this point */
>>> s390_cmma_reset();
>>>
>>> +cpu = S390_CPU(cs);
>>> +env = >env;
>>
>> Can you just pass "cpu" to s390_pv_prepare_reset() and handle it in there?
>
> Wouldn't it make more sense to test the machine state here anyway?
Whatever you prefer. Just saying, that introducing "CPUS390XState *env"
in this function can be avoided.
>
>>
>>> +
>>> switch (reset_type) {
>>> case S390_RESET_EXTERNAL:
>>> case S390_RESET_REIPL:
>>> +if (ms->pv) {
>>> +s390_machine_unprotect(ms);
>>> +}
>>> +
>>> qemu_devices_reset();
>>> s390_crypto_reset();
>>>
>>> @@ -337,22 +428,52 @@ static void s390_machine_reset(MachineState *machine)
>>> run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>>> break;
>>> case S390_RESET_MODIFIED_CLEAR:
>>> +/*
>>> + * Susbsystem reset needs to be done before we unshare memory
>>> + * and loose access to VIRTIO structures in guest memory.
>>> + */
>>> +subsystem_reset();
>>> +s390_crypto_reset();
>>> +s390_pv_prepare_reset(env);
>>> CPU_FOREACH(t) {
>>> run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
>>> }
>>> -subsystem_reset();
>>> -s390_crypto_reset();
>>> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>>> break;
>>> case S390_RESET_LOAD_NORMAL:
>>> +/*
>>> + * Susbsystem reset needs to be done before we unshare memory
>>> + * and loose access to VIRTIO structures in guest memory.
>>> + */
>>> +subsystem_reset();
>>> +s390_pv_prepare_reset(env);
>>> CPU_FOREACH(t) {
>>> if (t == cs) {
>>> continue;
>>> }
>>> run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL);
>>> }
>>> -subsystem_reset();
>>> run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL);
>>> +run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>>> +break;
>>> +case S390_RESET_PV: /* Subcode 10 */
>>> +subsystem_reset();
>>> +s390_crypto_reset();
>>> +
>>> +CPU_FOREACH(t) {
>>> +if (t == cs) {
>>> +continue;
>>> +}
>>> +run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
>>> +}
>>> +run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL);
>>> +
>>> +if (s390_machine_protect(ms)) {
>>> +s390_machine_inject_pv_error(cs);
>>> +s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>> +return;
>>> +}
>>> +
>>> run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>>
>> [...]
>>
>>>
>>> +#if !defined(CONFIG_USER_ONLY)
>>> +static bool machine_is_pv(MachineState *ms)
>>> +{
>>> +Object *obj;
>>> +
>>> +/* we have to bail out for the "none" machine */
>>> +obj = object_dynamic_cast(OBJECT(ms), TYPE_S390_CCW_MACHINE);
>>> + if (!obj) {
>>> +return false;
>>> +}
>>> +return S390_CCW_MACHINE(obj)->pv;
>>
>> Maybe you want to cache the machine, so you can avoid the
>> lookup+conversion on every new CPU.
>
> Christian has provided me with a fix to this code, I'll squash it.
>
>>
>>> +}
>>> +#endif
>>
>> [...]
>>> static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t
>>> addr,
>>>uintptr_t ra, bool write)
>>> {
>>> +/* Handled by the Ultravisor */
>>> +if (env->pv) {
>>> +return 0;
>>> +}
>>> if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) {
>>> s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>> return -1;
>>> @@ -93,6 +101,11 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1,
>>> uint64_t r3, uintptr_t ra)
>>> return;
>>> }
>>>
>>> +if (subcode > 7 && !s390_has_feat(S390_FEAT_UNPACK)) {
>>
>>> = DIAG308_PV_SET
>
> ?
Instead of the magic number seven, check against "subcode >= DIAG308_PV_SET"
--
Thanks,
David / dhildenb
Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
On 3/9/20 2:37 PM, David Hildenbrand wrote:
> On 09.03.20 12:21, Janosch Frank wrote:
>> The unpack facility provides the means to setup a protected guest. A
>> protected guest can not be introspected by the hypervisor or any
>> user/administrator of the machine it is running on.
>>
>> Protected guests are encrypted at rest and need a special boot
>> mechanism via diag308 subcode 8 and 10.
>>
>> Code 8 sets the PV specific IPLB which is retained seperately from
>> those set via code 5.
>>
>> Code 10 is used to unpack the VM into protected memory, verify its
>> integrity and start it.
>>
>> Signed-off-by: Janosch Frank
>> Signed-off-by: Christian Borntraeger [Changes
>> to machine]
>
> As you signed this patch off, Maybe this should rather be a Co-developed-by:
>
Ack
> [...]
>
>> {
>> S390IPLState *ipl = get_ipl_device();
>> @@ -561,7 +581,8 @@ void s390_ipl_reset_request(CPUState *cs, enum
>> s390_reset reset_type)
>> {
>> S390IPLState *ipl = get_ipl_device();
>>
>> -if (reset_type == S390_RESET_EXTERNAL || reset_type ==
>> S390_RESET_REIPL) {
>> +if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL
>> ||
>> +reset_type == S390_RESET_PV) {
>> /* use CPU 0 for full resets */
>> ipl->reset_cpu_index = 0;
>
> This looks wrong. In case of an error, you modify the registers of a
> theoretically unrelated CPU.
It does indeed, will fix.
>
>
>> } else {
>> @@ -635,6 +656,38 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
>> cpu_physical_memory_unmap(addr, len, 1, len);
>> }
>>
>> +int s390_ipl_prepare_pv_header(void)
>> +{
>> +S390IPLState *ipl = get_ipl_device();
>> +IPLBlockPV *ipib_pv = >iplb_pv.pv;
>> +void *hdr = g_malloc(ipib_pv->pv_header_len);
>
> Should there be an upper limit? The guest can allocate quite some memory
> this way and theoretically crash the VM.
Yes, it's currently two pages max.
>
>> +int rc;
>> +
>> +cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
>> + ipib_pv->pv_header_len);
>
> Shouldn't we validate if this memory is accessible at all?
Yes, I moved the check into iplb_valid so we don't need to worry about
that here.
>
>> +rc = s390_pv_set_sec_parms((uint64_t)hdr,
>> + ipib_pv->pv_header_len);
>> +g_free(hdr);
>> +return rc;
>> +}
>> +
>> +int s390_ipl_pv_unpack(void)
>> +{
>> +int i, rc = 0;
>
> NIT: These declarations last.
Ack
>
>> +S390IPLState *ipl = get_ipl_device();
>> +IPLBlockPV *ipib_pv = >iplb_pv.pv;
>
> use s390_ipl_get_iplb_pv() and assert that we don't get NULL?
We already check for NULL in the diag code before the DIAG308_PV_START
reset is set. I've used your other suggestion though.
>
>> +
>> +for (i = 0; i < ipib_pv->num_comp; i++) {
>> +rc = s390_pv_unpack(ipib_pv->components[i].addr,
>> +TARGET_PAGE_ALIGN(ipib_pv->components[i].size),
>> +ipib_pv->components[i].tweak_pref);
>> +if (rc) {
>> +break;
>> +}
>> +}
>> +return rc;
>> +}
>> +
>> void s390_ipl_prepare_cpu(S390CPU *cpu)
>> {
>> S390IPLState *ipl = get_ipl_device();
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index d4813105db..b2ccdd9dae 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -15,6 +15,24 @@
>> #include "cpu.h"
>> #include "hw/qdev-core.h"
>>
>> +struct IPLBlockPVComp {
>> +uint64_t tweak_pref;
>> +uint64_t addr;
>> +uint64_t size;
>> +} QEMU_PACKED;
>> +typedef struct IPLBlockPVComp IPLBlockPVComp;
>> +
>> +struct IPLBlockPV {
>> +uint8_t reserved18[87];/* 0x18 */
>> +uint8_t version; /* 0x6f */
>> +uint32_t reserved70;/* 0x70 */
>> +uint32_t num_comp; /* 0x70 */
>> +uint64_t pv_header_addr;/* 0x74 */
>> +uint64_t pv_header_len; /* 0x7c */
>> +struct IPLBlockPVComp components[];
>> +} QEMU_PACKED;
>> +typedef struct IPLBlockPV IPLBlockPV;
>> +
>> struct IplBlockCcw {
>> uint8_t reserved0[85];
>> uint8_t ssid;
>> @@ -71,6 +89,7 @@ union IplParameterBlock {
>> union {
>> IplBlockCcw ccw;
>> IplBlockFcp fcp;
>> +IPLBlockPV pv;
>> IplBlockQemuScsi scsi;
>> };
>> } QEMU_PACKED;
>> @@ -85,8 +104,11 @@ typedef union IplParameterBlock IplParameterBlock;
>>
>> int s390_ipl_set_loadparm(uint8_t *loadparm);
>> void s390_ipl_update_diag308(IplParameterBlock *iplb);
>> +int s390_ipl_prepare_pv_header(void);
>> +int s390_ipl_pv_unpack(void);
>> void s390_ipl_prepare_cpu(S390CPU *cpu);
>> IplParameterBlock *s390_ipl_get_iplb(void);
>> +IplParameterBlock *s390_ipl_get_iplb_pv(void);
>>
>> enum s390_reset {
>> /* default is a reset not triggered by a CPU e.g. issued by QMP */
>> @@ -94,6 +116,7 @@ enum s390_reset {
>> S390_RESET_REIPL,
>> S390_RESET_MODIFIED_CLEAR,
>>
Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
On 3/9/20 3:28 PM, Viktor Mihajlovski wrote:
>
>
> On 3/9/20 12:21 PM, Janosch Frank wrote:
>> The unpack facility provides the means to setup a protected guest. A
>> protected guest can not be introspected by the hypervisor or any
>> user/administrator of the machine it is running on.
>>
>> Protected guests are encrypted at rest and need a special boot
>> mechanism via diag308 subcode 8 and 10.
>>
>> Code 8 sets the PV specific IPLB which is retained seperately from
>> those set via code 5.
>>
>> Code 10 is used to unpack the VM into protected memory, verify its
>> integrity and start it.
>>
>> Signed-off-by: Janosch Frank
>> Signed-off-by: Christian Borntraeger [Changes
>> to machine]
> A nit: [Changes...] should go before the s-o-b.
Will be changed anyway
>> ---
>> hw/s390x/Makefile.objs | 1 +
>> hw/s390x/ipl.c | 59 -
>> hw/s390x/ipl.h | 72 ++--
>> hw/s390x/pv.c | 104 +++
>> hw/s390x/pv.h | 34
>> hw/s390x/s390-virtio-ccw.c | 127 +++-
>> include/hw/s390x/s390-virtio-ccw.h | 1 +
>> target/s390x/cpu.c | 17
>> target/s390x/cpu.h | 1 +
>> target/s390x/cpu_features_def.inc.h | 1 +
>> target/s390x/diag.c | 34 +++-
>> 11 files changed, 436 insertions(+), 15 deletions(-)
>> create mode 100644 hw/s390x/pv.c
>> create mode 100644 hw/s390x/pv.h
>>
>> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
>> index e02ed80b68..a46a1c7894 100644
>> --- a/hw/s390x/Makefile.objs
>> +++ b/hw/s390x/Makefile.objs
>> @@ -31,6 +31,7 @@ obj-y += tod-qemu.o
>> obj-$(CONFIG_KVM) += tod-kvm.o
>> obj-$(CONFIG_KVM) += s390-skeys-kvm.o
>> obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
>> +obj-$(CONFIG_KVM) += pv.o
>> obj-y += s390-ccw.o
>> obj-y += ap-device.o
>> obj-y += ap-bridge.o
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 9c1ecd423c..ba3eac30c6 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -33,6 +33,7 @@
>> #include "qemu/cutils.h"
>> #include "qemu/option.h"
>> #include "exec/exec-all.h"
>> +#include "pv.h"
>>
>> #define KERN_IMAGE_START0x01UL
>> #define LINUX_MAGIC_ADDR0x010008UL
>> @@ -542,11 +543,30 @@ void s390_ipl_update_diag308(IplParameterBlock *iplb)
>> {
>> S390IPLState *ipl = get_ipl_device();
>>
>> -ipl->iplb = *iplb;
>> -ipl->iplb_valid = true;
>> +/*
>> + * The IPLB set and retrieved by subcodes 8/9 is completely
>> + * separate from the one managed via subcodes 5/6.
>> + */
>> +if (iplb->pbt == S390_IPL_TYPE_PV) {
>> +ipl->iplb_pv = *iplb;
>> +ipl->iplb_valid_pv = true;
>> +} else {
>> +ipl->iplb = *iplb;
>> +ipl->iplb_valid = true;
>> +}
>> ipl->netboot = is_virtio_net_device(iplb);
>> }
>>
>> +IplParameterBlock *s390_ipl_get_iplb_pv(void)
>> +{
>> +S390IPLState *ipl = get_ipl_device();
>> +
>> +if (!ipl->iplb_valid_pv) {
>> +return NULL;
>> +}
>> +return >iplb_pv;
>> +}
>> +
>> IplParameterBlock *s390_ipl_get_iplb(void)
>> {
>> S390IPLState *ipl = get_ipl_device();
>> @@ -561,7 +581,8 @@ void s390_ipl_reset_request(CPUState *cs, enum
>> s390_reset reset_type)
>> {
>> S390IPLState *ipl = get_ipl_device();
>>
>> -if (reset_type == S390_RESET_EXTERNAL || reset_type ==
>> S390_RESET_REIPL) {
>> +if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL
>> ||
>> +reset_type == S390_RESET_PV) {
>> /* use CPU 0 for full resets */
>> ipl->reset_cpu_index = 0;
>> } else {
>> @@ -635,6 +656,38 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
>> cpu_physical_memory_unmap(addr, len, 1, len);
>> }
>>
>> +int s390_ipl_prepare_pv_header(void)
>> +{
>> +S390IPLState *ipl = get_ipl_device();
>> +IPLBlockPV *ipib_pv = >iplb_pv.pv;
>> +void *hdr = g_malloc(ipib_pv->pv_header_len);
>> +int rc;
>> +
>> +cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
>> + ipib_pv->pv_header_len);
>> +rc = s390_pv_set_sec_parms((uint64_t)hdr,
>> + ipib_pv->pv_header_len);
>> +g_free(hdr);
>> +return rc;
>> +}
>> +
>> +int s390_ipl_pv_unpack(void)
>> +{
>> +int i, rc = 0;
>> +S390IPLState *ipl = get_ipl_device();
>> +IPLBlockPV *ipib_pv = >iplb_pv.pv;
>> +
>> +for (i = 0; i < ipib_pv->num_comp; i++) {
>> +rc = s390_pv_unpack(ipib_pv->components[i].addr,
>> +TARGET_PAGE_ALIGN(ipib_pv->components[i].size),
>> +ipib_pv->components[i].tweak_pref);
>> +if (rc) {
>> +break;
>> +}
>> +}
>> +return rc;
>> +}
>> +
>> void s390_ipl_prepare_cpu(S390CPU *cpu)
>> {
>> S390IPLState *ipl
Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
On 09.03.20 15:40, Christian Borntraeger wrote:
>
>
> On 09.03.20 14:37, David Hildenbrand wrote:
>
>>>
>>> +#if !defined(CONFIG_USER_ONLY)
>>> +static bool machine_is_pv(MachineState *ms)
>>> +{
>>> +Object *obj;
>>> +
>>> +/* we have to bail out for the "none" machine */
>>> +obj = object_dynamic_cast(OBJECT(ms), TYPE_S390_CCW_MACHINE);
>>> + if (!obj) {
>>> +return false;
>>> +}
>>> +return S390_CCW_MACHINE(obj)->pv;
>>
>> Maybe you want to cache the machine, so you can avoid the
>> lookup+conversion on every new CPU.
>>
>
> something like the following?
>
>
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index c513f8efe0..cd12c29b9a 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -185,13 +185,18 @@ static void s390_cpu_disas_set_info(CPUState *cpu,
> disassemble_info *info)
> static bool machine_is_pv(MachineState *ms)
> {
> Object *obj;
> +static S390CcwMachineState *ccw;
> +
Move this up please (reverse christmas tree ;) )
> +if (ccw)
> + return ccw->pv;
>
> /* we have to bail out for the "none" machine */
> obj = object_dynamic_cast(OBJECT(ms), TYPE_S390_CCW_MACHINE);
> if (!obj) {
> return false;
> }
> -return S390_CCW_MACHINE(obj)->pv;
> +ccw = S390_CCW_MACHINE(obj);
> +return ccw->pv;
> }
> #endif
Yeah, guess this makes sense.
--
Thanks,
David / dhildenb
Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
On 09.03.20 14:37, David Hildenbrand wrote:
>>
>> +#if !defined(CONFIG_USER_ONLY)
>> +static bool machine_is_pv(MachineState *ms)
>> +{
>> +Object *obj;
>> +
>> +/* we have to bail out for the "none" machine */
>> +obj = object_dynamic_cast(OBJECT(ms), TYPE_S390_CCW_MACHINE);
>> + if (!obj) {
>> +return false;
>> +}
>> +return S390_CCW_MACHINE(obj)->pv;
>
> Maybe you want to cache the machine, so you can avoid the
> lookup+conversion on every new CPU.
>
something like the following?
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index c513f8efe0..cd12c29b9a 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -185,13 +185,18 @@ static void s390_cpu_disas_set_info(CPUState *cpu,
disassemble_info *info)
static bool machine_is_pv(MachineState *ms)
{
Object *obj;
+static S390CcwMachineState *ccw;
+
+if (ccw)
+ return ccw->pv;
/* we have to bail out for the "none" machine */
obj = object_dynamic_cast(OBJECT(ms), TYPE_S390_CCW_MACHINE);
if (!obj) {
return false;
}
-return S390_CCW_MACHINE(obj)->pv;
+ccw = S390_CCW_MACHINE(obj);
+return ccw->pv;
}
#endif
Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
On 3/9/20 12:21 PM, Janosch Frank wrote:
The unpack facility provides the means to setup a protected guest. A
protected guest can not be introspected by the hypervisor or any
user/administrator of the machine it is running on.
Protected guests are encrypted at rest and need a special boot
mechanism via diag308 subcode 8 and 10.
Code 8 sets the PV specific IPLB which is retained seperately from
those set via code 5.
Code 10 is used to unpack the VM into protected memory, verify its
integrity and start it.
Signed-off-by: Janosch Frank
Signed-off-by: Christian Borntraeger [Changes
to machine]
A nit: [Changes...] should go before the s-o-b.
---
hw/s390x/Makefile.objs | 1 +
hw/s390x/ipl.c | 59 -
hw/s390x/ipl.h | 72 ++--
hw/s390x/pv.c | 104 +++
hw/s390x/pv.h | 34
hw/s390x/s390-virtio-ccw.c | 127 +++-
include/hw/s390x/s390-virtio-ccw.h | 1 +
target/s390x/cpu.c | 17
target/s390x/cpu.h | 1 +
target/s390x/cpu_features_def.inc.h | 1 +
target/s390x/diag.c | 34 +++-
11 files changed, 436 insertions(+), 15 deletions(-)
create mode 100644 hw/s390x/pv.c
create mode 100644 hw/s390x/pv.h
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index e02ed80b68..a46a1c7894 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -31,6 +31,7 @@ obj-y += tod-qemu.o
obj-$(CONFIG_KVM) += tod-kvm.o
obj-$(CONFIG_KVM) += s390-skeys-kvm.o
obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
+obj-$(CONFIG_KVM) += pv.o
obj-y += s390-ccw.o
obj-y += ap-device.o
obj-y += ap-bridge.o
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 9c1ecd423c..ba3eac30c6 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -33,6 +33,7 @@
#include "qemu/cutils.h"
#include "qemu/option.h"
#include "exec/exec-all.h"
+#include "pv.h"
#define KERN_IMAGE_START0x01UL
#define LINUX_MAGIC_ADDR0x010008UL
@@ -542,11 +543,30 @@ void s390_ipl_update_diag308(IplParameterBlock *iplb)
{
S390IPLState *ipl = get_ipl_device();
-ipl->iplb = *iplb;
-ipl->iplb_valid = true;
+/*
+ * The IPLB set and retrieved by subcodes 8/9 is completely
+ * separate from the one managed via subcodes 5/6.
+ */
+if (iplb->pbt == S390_IPL_TYPE_PV) {
+ipl->iplb_pv = *iplb;
+ipl->iplb_valid_pv = true;
+} else {
+ipl->iplb = *iplb;
+ipl->iplb_valid = true;
+}
ipl->netboot = is_virtio_net_device(iplb);
}
+IplParameterBlock *s390_ipl_get_iplb_pv(void)
+{
+S390IPLState *ipl = get_ipl_device();
+
+if (!ipl->iplb_valid_pv) {
+return NULL;
+}
+return >iplb_pv;
+}
+
IplParameterBlock *s390_ipl_get_iplb(void)
{
S390IPLState *ipl = get_ipl_device();
@@ -561,7 +581,8 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset
reset_type)
{
S390IPLState *ipl = get_ipl_device();
-if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL) {
+if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL ||
+reset_type == S390_RESET_PV) {
/* use CPU 0 for full resets */
ipl->reset_cpu_index = 0;
} else {
@@ -635,6 +656,38 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
cpu_physical_memory_unmap(addr, len, 1, len);
}
+int s390_ipl_prepare_pv_header(void)
+{
+S390IPLState *ipl = get_ipl_device();
+IPLBlockPV *ipib_pv = >iplb_pv.pv;
+void *hdr = g_malloc(ipib_pv->pv_header_len);
+int rc;
+
+cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
+ ipib_pv->pv_header_len);
+rc = s390_pv_set_sec_parms((uint64_t)hdr,
+ ipib_pv->pv_header_len);
+g_free(hdr);
+return rc;
+}
+
+int s390_ipl_pv_unpack(void)
+{
+int i, rc = 0;
+S390IPLState *ipl = get_ipl_device();
+IPLBlockPV *ipib_pv = >iplb_pv.pv;
+
+for (i = 0; i < ipib_pv->num_comp; i++) {
+rc = s390_pv_unpack(ipib_pv->components[i].addr,
+TARGET_PAGE_ALIGN(ipib_pv->components[i].size),
+ipib_pv->components[i].tweak_pref);
+if (rc) {
+break;
+}
+}
+return rc;
+}
+
void s390_ipl_prepare_cpu(S390CPU *cpu)
{
S390IPLState *ipl = get_ipl_device();
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index d4813105db..b2ccdd9dae 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -15,6 +15,24 @@
#include "cpu.h"
#include "hw/qdev-core.h"
+struct IPLBlockPVComp {
+uint64_t tweak_pref;
+uint64_t addr;
+uint64_t size;
+} QEMU_PACKED;
+typedef struct IPLBlockPVComp IPLBlockPVComp;
+
+struct IPLBlockPV {
+uint8_t reserved18[87];/* 0x18 */
+uint8_t version; /* 0x6f */
+
Re: [PATCH v7 02/15] s390x: protvirt: Support unpack facility
On 09.03.20 12:21, Janosch Frank wrote:
> The unpack facility provides the means to setup a protected guest. A
> protected guest can not be introspected by the hypervisor or any
> user/administrator of the machine it is running on.
>
> Protected guests are encrypted at rest and need a special boot
> mechanism via diag308 subcode 8 and 10.
>
> Code 8 sets the PV specific IPLB which is retained seperately from
> those set via code 5.
>
> Code 10 is used to unpack the VM into protected memory, verify its
> integrity and start it.
>
> Signed-off-by: Janosch Frank
> Signed-off-by: Christian Borntraeger [Changes
> to machine]
As you signed this patch off, Maybe this should rather be a Co-developed-by:
[...]
> {
> S390IPLState *ipl = get_ipl_device();
> @@ -561,7 +581,8 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset
> reset_type)
> {
> S390IPLState *ipl = get_ipl_device();
>
> -if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL)
> {
> +if (reset_type == S390_RESET_EXTERNAL || reset_type == S390_RESET_REIPL
> ||
> +reset_type == S390_RESET_PV) {
> /* use CPU 0 for full resets */
> ipl->reset_cpu_index = 0;
This looks wrong. In case of an error, you modify the registers of a
theoretically unrelated CPU.
> } else {
> @@ -635,6 +656,38 @@ static void s390_ipl_prepare_qipl(S390CPU *cpu)
> cpu_physical_memory_unmap(addr, len, 1, len);
> }
>
> +int s390_ipl_prepare_pv_header(void)
> +{
> +S390IPLState *ipl = get_ipl_device();
> +IPLBlockPV *ipib_pv = >iplb_pv.pv;
> +void *hdr = g_malloc(ipib_pv->pv_header_len);
Should there be an upper limit? The guest can allocate quite some memory
this way and theoretically crash the VM.
> +int rc;
> +
> +cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
> + ipib_pv->pv_header_len);
Shouldn't we validate if this memory is accessible at all?
> +rc = s390_pv_set_sec_parms((uint64_t)hdr,
> + ipib_pv->pv_header_len);
> +g_free(hdr);
> +return rc;
> +}
> +
> +int s390_ipl_pv_unpack(void)
> +{
> +int i, rc = 0;
NIT: These declarations last.
> +S390IPLState *ipl = get_ipl_device();
> +IPLBlockPV *ipib_pv = >iplb_pv.pv;
use s390_ipl_get_iplb_pv() and assert that we don't get NULL?
> +
> +for (i = 0; i < ipib_pv->num_comp; i++) {
> +rc = s390_pv_unpack(ipib_pv->components[i].addr,
> +TARGET_PAGE_ALIGN(ipib_pv->components[i].size),
> +ipib_pv->components[i].tweak_pref);
> +if (rc) {
> +break;
> +}
> +}
> +return rc;
> +}
> +
> void s390_ipl_prepare_cpu(S390CPU *cpu)
> {
> S390IPLState *ipl = get_ipl_device();
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index d4813105db..b2ccdd9dae 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -15,6 +15,24 @@
> #include "cpu.h"
> #include "hw/qdev-core.h"
>
> +struct IPLBlockPVComp {
> +uint64_t tweak_pref;
> +uint64_t addr;
> +uint64_t size;
> +} QEMU_PACKED;
> +typedef struct IPLBlockPVComp IPLBlockPVComp;
> +
> +struct IPLBlockPV {
> +uint8_t reserved18[87];/* 0x18 */
> +uint8_t version; /* 0x6f */
> +uint32_t reserved70;/* 0x70 */
> +uint32_t num_comp; /* 0x70 */
> +uint64_t pv_header_addr;/* 0x74 */
> +uint64_t pv_header_len; /* 0x7c */
> +struct IPLBlockPVComp components[];
> +} QEMU_PACKED;
> +typedef struct IPLBlockPV IPLBlockPV;
> +
> struct IplBlockCcw {
> uint8_t reserved0[85];
> uint8_t ssid;
> @@ -71,6 +89,7 @@ union IplParameterBlock {
> union {
> IplBlockCcw ccw;
> IplBlockFcp fcp;
> +IPLBlockPV pv;
> IplBlockQemuScsi scsi;
> };
> } QEMU_PACKED;
> @@ -85,8 +104,11 @@ typedef union IplParameterBlock IplParameterBlock;
>
> int s390_ipl_set_loadparm(uint8_t *loadparm);
> void s390_ipl_update_diag308(IplParameterBlock *iplb);
> +int s390_ipl_prepare_pv_header(void);
> +int s390_ipl_pv_unpack(void);
> void s390_ipl_prepare_cpu(S390CPU *cpu);
> IplParameterBlock *s390_ipl_get_iplb(void);
> +IplParameterBlock *s390_ipl_get_iplb_pv(void);
>
> enum s390_reset {
> /* default is a reset not triggered by a CPU e.g. issued by QMP */
> @@ -94,6 +116,7 @@ enum s390_reset {
> S390_RESET_REIPL,
> S390_RESET_MODIFIED_CLEAR,
> S390_RESET_LOAD_NORMAL,
> +S390_RESET_PV,
> };
> void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type);
> void s390_ipl_get_reset_request(CPUState **cs, enum s390_reset *reset_type);
> @@ -133,6 +156,7 @@ struct S390IPLState {
> /*< private >*/
> DeviceState parent_obj;
> IplParameterBlock iplb;
> +IplParameterBlock iplb_pv;
> QemuIplParameters qipl;
> uint64_t start_addr;
> uint64_t compat_start_addr;
> @@ -140,6 +164,7 @@ struct S390IPLState {
>
