On 3/5/20 1:04 PM, Janosch Frank wrote: > On 3/4/20 6:04 PM, David Hildenbrand wrote: >> On 04.03.20 12:42, Janosch Frank wrote: >>> For diag308 subcodes 8 - 10 we have a new ipib of type 5. The ipib >>> holds the address and length of the secure execution header, as well >>> as a list of guest components. >>> >>> Each component is a block of memory, for example kernel or initrd, >>> which needs to be decrypted by the Ultravisor in order to run a >>> protected VM. The secure execution header instructs the Ultravisor on >>> how to handle the protected VM and its components. >>> >>> Subcodes 8 and 9 are similiar to 5 and 6 and subcode 10 will finally >>> start the protected guest. >>> >>> Subcodes 8-10 are not valid in protected mode, we have to do a subcode >>> 3 and then the 8 and 10 combination for a protected reboot. >>> >>> Signed-off-by: Janosch Frank <fran...@linux.ibm.com> >>> --- >>> hw/s390x/ipl.c | 47 ++++++++++++++++++++++++++++++++++++++++++--- >>> hw/s390x/ipl.h | 32 ++++++++++++++++++++++++++++++ >>> target/s390x/diag.c | 26 ++++++++++++++++++++++--- >>> 3 files changed, 99 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >>> index 9c1ecd423c..80c6ab233a 100644 >>> --- a/hw/s390x/ipl.c >>> +++ b/hw/s390x/ipl.c >>> @@ -538,15 +538,55 @@ static bool is_virtio_scsi_device(IplParameterBlock >>> *iplb) >>> return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_SCSI); >>> } >>> >>> +int s390_ipl_pv_check_components(IplParameterBlock *iplb) >> >> What about making this >> >> bool s390_ipl_pv_valid(IplParameterBlock *iplb) >> >> and return true/false? > > We already have iplb_valid_pv() and ipl->iplb_valid_pv. > Do you have any other more expressive name we could use?
I think it makes more sense to rip out these tiny functions and consolidate them like this: +static inline bool iplb_valid(IplParameterBlock *iplb) { - return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN && - iplb->pbt == S390_IPL_TYPE_FCP; + switch (iplb->pbt) { + case S390_IPL_TYPE_FCP: + return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_FCP_LEN && + iplb->pbt == S390_IPL_TYPE_FCP); + case S390_IPL_TYPE_CCW: + return (be32_to_cpu(iplb->len) >= S390_IPLB_MIN_CCW_LEN && + iplb->pbt == S390_IPL_TYPE_CCW); + case S390_IPL_TYPE_PV: + if(be32_to_cpu(iplb->len) < S390_IPLB_MIN_PV_LEN || + iplb->pbt != S390_IPL_TYPE_PV) { + return false; + } + return s390_ipl_pv_check_components(iplb); + default: + return false; + } } The component check is still a separate function right above this one in ipl.h > >> >>> +{ >>> + int i; >>> + IPLBlockPV *ipib_pv = &iplb->pv; >> >> nit: place "int i;" down here > > Ack > >> >>> + >>> + if (ipib_pv->num_comp == 0) { >>> + return -EINVAL; >>> + } >>> + >>> + for (i = 0; i < ipib_pv->num_comp; i++) { >>> + /* Addr must be 4k aligned */ >>> + if (ipib_pv->components[i].addr & ~TARGET_PAGE_MASK) { >>> + return -EINVAL; >>> + } >>> + >>> + /* Tweak prefix is monotonously increasing with each component */ >> >> should that be "monotonically increasing" ? > > Ooooooh, yeah... > >> >>> + if (i < ipib_pv->num_comp - 1 && >>> + ipib_pv->components[i].tweak_pref > >>> + ipib_pv->components[i + 1].tweak_pref) { >> >> and I assume "==" is valid then. > > Nope, it should be >= in this check > >> >>> + return -EINVAL; >>> + } >>> + } >>> + return 0; >>> +} >>> + >>> void s390_ipl_update_diag308(IplParameterBlock *iplb) >>> { >>> S390IPLState *ipl = get_ipl_device(); >>> >>> - ipl->iplb = *iplb; >>> - ipl->iplb_valid = true; >>> + 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_secure(void) >> >> Why suddenly the "secure" ? s390_ipl_get_iplb_pv? > > Remnants of former times > >> >>> +{ >>> + S390IPLState *ipl = get_ipl_device(); >>> + >>> + if (!ipl->iplb_valid_pv) { >>> + return NULL; >>> + } >>> + return &ipl->iplb_pv; >>> +} >>> + >>> IplParameterBlock *s390_ipl_get_iplb(void) >>> { >>> S390IPLState *ipl = get_ipl_device(); >>> @@ -561,7 +601,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) { >> >> What about a switch-case now instead? >> >>> /* use CPU 0 for full resets */ >>> ipl->reset_cpu_index = 0; >>> } else { >>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h >>> index d4813105db..04be63cee1 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; >> >> Do we need the packed here? All members are naturally aligned. > > No, I'll remove them > >> >>> +typedef struct IPLBlockPVComp IPLBlockPVComp; >>> + >>> +struct IPLBlockPV { >>> + uint8_t reserved[87]; >>> + uint8_t version; >>> + uint32_t reserved70; >>> + uint32_t num_comp; >>> + uint64_t pv_header_addr; >>> + uint64_t pv_header_len; >>> + struct IPLBlockPVComp components[]; >>> +} QEMU_PACKED; >> >> Dito. >> >> [...] >> >>> uint64_t compat_bios_start_addr; >>> bool enforce_bios; >>> bool iplb_valid; >>> + bool iplb_valid_pv; >> >> I'd name this "iplb_pv_valid" to match "iplb_pv". > > I like matching prefixes :) > >> >>> bool netboot; >>> /* reset related properties don't have to be migrated or reset */ >>> enum s390_reset reset_type; >>> @@ -161,9 +185,11 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, >>> "alignment of iplb wrong"); >>> >>> #define S390_IPL_TYPE_FCP 0x00 >>> #define S390_IPL_TYPE_CCW 0x02 >>> +#define S390_IPL_TYPE_PV 0x05 >>> #define S390_IPL_TYPE_QEMU_SCSI 0xff >>> >>> #define S390_IPLB_HEADER_LEN 8 >>> +#define S390_IPLB_MIN_PV_LEN 148 >>> #define S390_IPLB_MIN_CCW_LEN 200 >>> #define S390_IPLB_MIN_FCP_LEN 384 >>> #define S390_IPLB_MIN_QEMU_SCSI_LEN 200 >>> @@ -185,4 +211,10 @@ static inline bool iplb_valid_fcp(IplParameterBlock >>> *iplb) >>> iplb->pbt == S390_IPL_TYPE_FCP; >>> } >>> >>> +static inline bool iplb_valid_pv(IplParameterBlock *iplb) >>> +{ >>> + return be32_to_cpu(iplb->len) >= S390_IPLB_MIN_PV_LEN && >>> + iplb->pbt == S390_IPL_TYPE_PV; >>> +} >>> + >>> #endif >>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c >>> index b5aec06d6b..945b263f0a 100644 >>> --- a/target/s390x/diag.c >>> +++ b/target/s390x/diag.c >>> @@ -52,6 +52,7 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, >>> uint64_t r3) >>> #define DIAG_308_RC_OK 0x0001 >>> #define DIAG_308_RC_NO_CONF 0x0102 >>> #define DIAG_308_RC_INVALID 0x0402 >>> +#define DIAG_308_RC_NO_PV_CONF 0x0902 >>> >>> #define DIAG308_RESET_MOD_CLR 0 >>> #define DIAG308_RESET_LOAD_NORM 1 >>> @@ -59,6 +60,9 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, >>> uint64_t r3) >>> #define DIAG308_LOAD_NORMAL_DUMP 4 >>> #define DIAG308_SET 5 >>> #define DIAG308_STORE 6 >>> +#define DIAG308_PV_SET 8 >>> +#define DIAG308_PV_STORE 9 >>> +#define DIAG308_PV_START 10 >>> >>> static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t >>> addr, >>> uintptr_t ra, bool write) >>> @@ -105,6 +109,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, >>> uint64_t r3, uintptr_t ra) >>> s390_ipl_reset_request(cs, S390_RESET_REIPL); >>> break; >>> case DIAG308_SET: >>> + case DIAG308_PV_SET: >>> if (diag308_parm_check(env, r1, addr, ra, false)) { >>> return; >>> } >>> @@ -117,7 +122,8 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, >>> uint64_t r3, uintptr_t ra) >>> >>> cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len)); >>> >>> - if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb)) { >>> + if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb) && >>> + !(iplb_valid_pv(iplb) && !s390_ipl_pv_check_components(iplb))) >>> { >> >> I really think we should make this s390_ipl_pv_valid(), we're mixing >> functions that return true on success with functions that return 0 on >> success. Also, can't we simply move that check into iplb_valid_pv(iplb) >> to make this here easier to read? > > Yes, let me figure something out > >> >>> env->regs[r1 + 1] = DIAG_308_RC_INVALID; >>> goto out; >>> } >>> @@ -128,17 +134,31 @@ out: >>> g_free(iplb); >>> return; >>> case DIAG308_STORE: >>> + case DIAG308_PV_STORE: >>> if (diag308_parm_check(env, r1, addr, ra, true)) { >>> return; >>> } >>> - iplb = s390_ipl_get_iplb(); >>> + if (subcode == DIAG308_PV_STORE) { >>> + iplb = s390_ipl_get_iplb_secure(); >>> + } else { >>> + iplb = s390_ipl_get_iplb(); >>> + } >>> if (iplb) { >>> cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len)); >>> env->regs[r1 + 1] = DIAG_308_RC_OK; >>> } else { >>> env->regs[r1 + 1] = DIAG_308_RC_NO_CONF; >>> } >>> - return; >>> + break; >>> + case DIAG308_PV_START: >>> + iplb = s390_ipl_get_iplb_secure(); >>> + if (!iplb || !iplb_valid_pv(iplb)) { >> >> Why do we need another iplb_valid_pv() check? I thought we would verify >> this when setting and marking valid. > > Good question, I'll look into it and give this patch a dust off > >> >>> + env->regs[r1 + 1] = DIAG_308_RC_NO_PV_CONF; >>> + return; >>> + } >>> + >> >> > >
signature.asc
Description: OpenPGP digital signature