* [PATCH v5 01/18] s390x: Use constant for ESA PSW address
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 13:46 ` Christian Borntraeger
2020-02-26 14:27 ` David Hildenbrand
2020-02-26 12:20 ` [PATCH v5 02/18] Sync pv Janosch Frank
` (18 subsequent siblings)
19 siblings, 2 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Lets make it a bit more clear that we're extracting the 31 bit address
from the short psw.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
hw/s390x/ipl.c | 2 +-
target/s390x/cpu.c | 4 ++--
target/s390x/cpu.h | 1 +
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 7773499d7f..42e21e7a6a 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -179,7 +179,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
/* if not Linux load the address of the (short) IPL PSW */
ipl_psw = rom_ptr(4, 4);
if (ipl_psw) {
- pentry = be32_to_cpu(*ipl_psw) & 0x7fffffffUL;
+ pentry = be32_to_cpu(*ipl_psw) & PSW_MASK_ESA_ADDR;
} else {
error_setg(&err, "Could not get IPL PSW");
goto error;
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 8da1905485..43360912a0 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -78,13 +78,13 @@ static void s390_cpu_load_normal(CPUState *s)
S390CPU *cpu = S390_CPU(s);
uint64_t spsw = ldq_phys(s->as, 0);
- cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
+ cpu->env.psw.mask = spsw & PSW_MASK_ESA_MASK;
/*
* Invert short psw indication, so SIE will report a specification
* exception if it was not set.
*/
cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
- cpu->env.psw.addr = spsw & 0x7fffffffULL;
+ cpu->env.psw.addr = spsw & PSW_MASK_ESA_ADDR;
s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
}
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 8a557fd8d1..74e66fe0c2 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -277,6 +277,7 @@ extern const VMStateDescription vmstate_s390_cpu;
#define PSW_MASK_64 0x0000000100000000ULL
#define PSW_MASK_32 0x0000000080000000ULL
#define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
+#define PSW_MASK_ESA_MASK 0xffffffff80000000ULL
#undef PSW_ASC_PRIMARY
#undef PSW_ASC_ACCREG
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v5 01/18] s390x: Use constant for ESA PSW address
2020-02-26 12:20 ` [PATCH v5 01/18] s390x: Use constant for ESA PSW address Janosch Frank
@ 2020-02-26 13:46 ` Christian Borntraeger
2020-02-26 13:47 ` Janosch Frank
2020-02-26 14:27 ` David Hildenbrand
1 sibling, 1 reply; 62+ messages in thread
From: Christian Borntraeger @ 2020-02-26 13:46 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck, david
On 26.02.20 13:20, Janosch Frank wrote:
> Lets make it a bit more clear that we're extracting the 31 bit address
> from the short psw.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
PSW_MASK_ESA_MASK and PSW_MASK_ESA_ADDR look pretty similar, but I cant find
a good name. We could use ~PSW_MASK_ESA_ADDR as an alternative.
Either way, this makes sense.
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> hw/s390x/ipl.c | 2 +-
> target/s390x/cpu.c | 4 ++--
> target/s390x/cpu.h | 1 +
> 3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 7773499d7f..42e21e7a6a 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -179,7 +179,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
> /* if not Linux load the address of the (short) IPL PSW */
> ipl_psw = rom_ptr(4, 4);
> if (ipl_psw) {
> - pentry = be32_to_cpu(*ipl_psw) & 0x7fffffffUL;
> + pentry = be32_to_cpu(*ipl_psw) & PSW_MASK_ESA_ADDR;
> } else {
> error_setg(&err, "Could not get IPL PSW");
> goto error;
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 8da1905485..43360912a0 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -78,13 +78,13 @@ static void s390_cpu_load_normal(CPUState *s)
> S390CPU *cpu = S390_CPU(s);
> uint64_t spsw = ldq_phys(s->as, 0);
>
> - cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
> + cpu->env.psw.mask = spsw & PSW_MASK_ESA_MASK;
> /*
> * Invert short psw indication, so SIE will report a specification
> * exception if it was not set.
> */
> cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
> - cpu->env.psw.addr = spsw & 0x7fffffffULL;
> + cpu->env.psw.addr = spsw & PSW_MASK_ESA_ADDR;
>
> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> }
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 8a557fd8d1..74e66fe0c2 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -277,6 +277,7 @@ extern const VMStateDescription vmstate_s390_cpu;
> #define PSW_MASK_64 0x0000000100000000ULL
> #define PSW_MASK_32 0x0000000080000000ULL
> #define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
> +#define PSW_MASK_ESA_MASK 0xffffffff80000000ULL
>
> #undef PSW_ASC_PRIMARY
> #undef PSW_ASC_ACCREG
>
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 01/18] s390x: Use constant for ESA PSW address
2020-02-26 13:46 ` Christian Borntraeger
@ 2020-02-26 13:47 ` Janosch Frank
0 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 13:47 UTC (permalink / raw)
To: Christian Borntraeger, qemu-devel; +Cc: qemu-s390x, cohuck, david
[-- Attachment #1.1: Type: text/plain, Size: 2801 bytes --]
On 2/26/20 2:46 PM, Christian Borntraeger wrote:
>
>
> On 26.02.20 13:20, Janosch Frank wrote:
>> Lets make it a bit more clear that we're extracting the 31 bit address
>> from the short psw.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>
>
>
> PSW_MASK_ESA_MASK and PSW_MASK_ESA_ADDR look pretty similar, but I cant find
> a good name. We could use ~PSW_MASK_ESA_ADDR as an alternative.
I'm also not too happy about it, if there's a better suggestion I'd take
it any day.
>
> Either way, this makes sense.
>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Thanks!
>
>> ---
>> hw/s390x/ipl.c | 2 +-
>> target/s390x/cpu.c | 4 ++--
>> target/s390x/cpu.h | 1 +
>> 3 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 7773499d7f..42e21e7a6a 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -179,7 +179,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>> /* if not Linux load the address of the (short) IPL PSW */
>> ipl_psw = rom_ptr(4, 4);
>> if (ipl_psw) {
>> - pentry = be32_to_cpu(*ipl_psw) & 0x7fffffffUL;
>> + pentry = be32_to_cpu(*ipl_psw) & PSW_MASK_ESA_ADDR;
>> } else {
>> error_setg(&err, "Could not get IPL PSW");
>> goto error;
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 8da1905485..43360912a0 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -78,13 +78,13 @@ static void s390_cpu_load_normal(CPUState *s)
>> S390CPU *cpu = S390_CPU(s);
>> uint64_t spsw = ldq_phys(s->as, 0);
>>
>> - cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
>> + cpu->env.psw.mask = spsw & PSW_MASK_ESA_MASK;
>
>
>> /*
>> * Invert short psw indication, so SIE will report a specification
>> * exception if it was not set.
>> */
>> cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
>> - cpu->env.psw.addr = spsw & 0x7fffffffULL;
>> + cpu->env.psw.addr = spsw & PSW_MASK_ESA_ADDR;
>>
>> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>> }
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 8a557fd8d1..74e66fe0c2 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -277,6 +277,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>> #define PSW_MASK_64 0x0000000100000000ULL
>> #define PSW_MASK_32 0x0000000080000000ULL
>> #define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
>> +#define PSW_MASK_ESA_MASK 0xffffffff80000000ULL
>>
>> #undef PSW_ASC_PRIMARY
>> #undef PSW_ASC_ACCREG
>>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 01/18] s390x: Use constant for ESA PSW address
2020-02-26 12:20 ` [PATCH v5 01/18] s390x: Use constant for ESA PSW address Janosch Frank
2020-02-26 13:46 ` Christian Borntraeger
@ 2020-02-26 14:27 ` David Hildenbrand
2020-02-26 17:51 ` Cornelia Huck
1 sibling, 1 reply; 62+ messages in thread
From: David Hildenbrand @ 2020-02-26 14:27 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
On 26.02.20 13:20, Janosch Frank wrote:
> Lets make it a bit more clear that we're extracting the 31 bit address
> from the short psw.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> hw/s390x/ipl.c | 2 +-
> target/s390x/cpu.c | 4 ++--
> target/s390x/cpu.h | 1 +
> 3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 7773499d7f..42e21e7a6a 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -179,7 +179,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
> /* if not Linux load the address of the (short) IPL PSW */
> ipl_psw = rom_ptr(4, 4);
> if (ipl_psw) {
> - pentry = be32_to_cpu(*ipl_psw) & 0x7fffffffUL;
> + pentry = be32_to_cpu(*ipl_psw) & PSW_MASK_ESA_ADDR;
> } else {
> error_setg(&err, "Could not get IPL PSW");
> goto error;
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 8da1905485..43360912a0 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -78,13 +78,13 @@ static void s390_cpu_load_normal(CPUState *s)
> S390CPU *cpu = S390_CPU(s);
> uint64_t spsw = ldq_phys(s->as, 0);
>
> - cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
> + cpu->env.psw.mask = spsw & PSW_MASK_ESA_MASK;
> /*
> * Invert short psw indication, so SIE will report a specification
> * exception if it was not set.
> */
> cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
> - cpu->env.psw.addr = spsw & 0x7fffffffULL;
> + cpu->env.psw.addr = spsw & PSW_MASK_ESA_ADDR;
>
> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> }
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 8a557fd8d1..74e66fe0c2 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -277,6 +277,7 @@ extern const VMStateDescription vmstate_s390_cpu;
> #define PSW_MASK_64 0x0000000100000000ULL
> #define PSW_MASK_32 0x0000000080000000ULL
> #define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
> +#define PSW_MASK_ESA_MASK 0xffffffff80000000ULL
..._MASK_..._MASK
Isn't there a better name for all the bits in the PSW that are not an
address?
PSW_MASK_ESA_BITS
PSW_MASK_ESA_FLAGS
...
>
> #undef PSW_ASC_PRIMARY
> #undef PSW_ASC_ACCREG
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 01/18] s390x: Use constant for ESA PSW address
2020-02-26 14:27 ` David Hildenbrand
@ 2020-02-26 17:51 ` Cornelia Huck
2020-02-26 17:52 ` David Hildenbrand
2020-02-27 7:53 ` Janosch Frank
0 siblings, 2 replies; 62+ messages in thread
From: Cornelia Huck @ 2020-02-26 17:51 UTC (permalink / raw)
To: David Hildenbrand; +Cc: borntraeger, qemu-s390x, Janosch Frank, qemu-devel
On Wed, 26 Feb 2020 15:27:52 +0100
David Hildenbrand <david@redhat.com> wrote:
> On 26.02.20 13:20, Janosch Frank wrote:
> > Lets make it a bit more clear that we're extracting the 31 bit address
s/Lets/Let's/ :)
> > from the short psw.
> >
> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> > ---
> > hw/s390x/ipl.c | 2 +-
> > target/s390x/cpu.c | 4 ++--
> > target/s390x/cpu.h | 1 +
> > 3 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> > index 7773499d7f..42e21e7a6a 100644
> > --- a/hw/s390x/ipl.c
> > +++ b/hw/s390x/ipl.c
> > @@ -179,7 +179,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
> > /* if not Linux load the address of the (short) IPL PSW */
> > ipl_psw = rom_ptr(4, 4);
> > if (ipl_psw) {
> > - pentry = be32_to_cpu(*ipl_psw) & 0x7fffffffUL;
> > + pentry = be32_to_cpu(*ipl_psw) & PSW_MASK_ESA_ADDR;
> > } else {
> > error_setg(&err, "Could not get IPL PSW");
> > goto error;
> > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> > index 8da1905485..43360912a0 100644
> > --- a/target/s390x/cpu.c
> > +++ b/target/s390x/cpu.c
> > @@ -78,13 +78,13 @@ static void s390_cpu_load_normal(CPUState *s)
> > S390CPU *cpu = S390_CPU(s);
> > uint64_t spsw = ldq_phys(s->as, 0);
> >
> > - cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
> > + cpu->env.psw.mask = spsw & PSW_MASK_ESA_MASK;
> > /*
> > * Invert short psw indication, so SIE will report a specification
> > * exception if it was not set.
> > */
> > cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
> > - cpu->env.psw.addr = spsw & 0x7fffffffULL;
> > + cpu->env.psw.addr = spsw & PSW_MASK_ESA_ADDR;
> >
> > s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> > }
> > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > index 8a557fd8d1..74e66fe0c2 100644
> > --- a/target/s390x/cpu.h
> > +++ b/target/s390x/cpu.h
> > @@ -277,6 +277,7 @@ extern const VMStateDescription vmstate_s390_cpu;
> > #define PSW_MASK_64 0x0000000100000000ULL
> > #define PSW_MASK_32 0x0000000080000000ULL
> > #define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
> > +#define PSW_MASK_ESA_MASK 0xffffffff80000000ULL
>
> ..._MASK_..._MASK
>
> Isn't there a better name for all the bits in the PSW that are not an
> address?
>
> PSW_MASK_ESA_BITS
> PSW_MASK_ESA_FLAGS
> ...
Hm, the PoP says that the PSW "includes the instruction address,
condition code, and other control fields"; it also talks about the
'short' PSW as being distinct from the 'ESA' PSW (bit 31 may be 0 or 1
in the short PSW). Maybe
PSW_MASK_SHORT_ADDR
PSW_MASK_SHORT_CTRL
(Or keep _ESA_ if renaming creates too much churn.)
>
> >
> > #undef PSW_ASC_PRIMARY
> > #undef PSW_ASC_ACCREG
> >
>
>
This patch is also independent of the protected virtualization
support... I plan to send a pull request tomorrow, so I can include
this patch, if we agree on a name for the constant :)
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 01/18] s390x: Use constant for ESA PSW address
2020-02-26 17:51 ` Cornelia Huck
@ 2020-02-26 17:52 ` David Hildenbrand
2020-02-27 7:53 ` Janosch Frank
1 sibling, 0 replies; 62+ messages in thread
From: David Hildenbrand @ 2020-02-26 17:52 UTC (permalink / raw)
To: Cornelia Huck; +Cc: borntraeger, qemu-s390x, Janosch Frank, qemu-devel
On 26.02.20 18:51, Cornelia Huck wrote:
> On Wed, 26 Feb 2020 15:27:52 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
>> On 26.02.20 13:20, Janosch Frank wrote:
>>> Lets make it a bit more clear that we're extracting the 31 bit address
>
> s/Lets/Let's/ :)
>
>>> from the short psw.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>> hw/s390x/ipl.c | 2 +-
>>> target/s390x/cpu.c | 4 ++--
>>> target/s390x/cpu.h | 1 +
>>> 3 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>> index 7773499d7f..42e21e7a6a 100644
>>> --- a/hw/s390x/ipl.c
>>> +++ b/hw/s390x/ipl.c
>>> @@ -179,7 +179,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>> /* if not Linux load the address of the (short) IPL PSW */
>>> ipl_psw = rom_ptr(4, 4);
>>> if (ipl_psw) {
>>> - pentry = be32_to_cpu(*ipl_psw) & 0x7fffffffUL;
>>> + pentry = be32_to_cpu(*ipl_psw) & PSW_MASK_ESA_ADDR;
>>> } else {
>>> error_setg(&err, "Could not get IPL PSW");
>>> goto error;
>>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>>> index 8da1905485..43360912a0 100644
>>> --- a/target/s390x/cpu.c
>>> +++ b/target/s390x/cpu.c
>>> @@ -78,13 +78,13 @@ static void s390_cpu_load_normal(CPUState *s)
>>> S390CPU *cpu = S390_CPU(s);
>>> uint64_t spsw = ldq_phys(s->as, 0);
>>>
>>> - cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
>>> + cpu->env.psw.mask = spsw & PSW_MASK_ESA_MASK;
>>> /*
>>> * Invert short psw indication, so SIE will report a specification
>>> * exception if it was not set.
>>> */
>>> cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
>>> - cpu->env.psw.addr = spsw & 0x7fffffffULL;
>>> + cpu->env.psw.addr = spsw & PSW_MASK_ESA_ADDR;
>>>
>>> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>> }
>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>> index 8a557fd8d1..74e66fe0c2 100644
>>> --- a/target/s390x/cpu.h
>>> +++ b/target/s390x/cpu.h
>>> @@ -277,6 +277,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>>> #define PSW_MASK_64 0x0000000100000000ULL
>>> #define PSW_MASK_32 0x0000000080000000ULL
>>> #define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
>>> +#define PSW_MASK_ESA_MASK 0xffffffff80000000ULL
>>
>> ..._MASK_..._MASK
>>
>> Isn't there a better name for all the bits in the PSW that are not an
>> address?
>>
>> PSW_MASK_ESA_BITS
>> PSW_MASK_ESA_FLAGS
>> ...
>
> Hm, the PoP says that the PSW "includes the instruction address,
> condition code, and other control fields"; it also talks about the
> 'short' PSW as being distinct from the 'ESA' PSW (bit 31 may be 0 or 1
> in the short PSW). Maybe
>
> PSW_MASK_SHORT_ADDR
> PSW_MASK_SHORT_CTRL
+1
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 01/18] s390x: Use constant for ESA PSW address
2020-02-26 17:51 ` Cornelia Huck
2020-02-26 17:52 ` David Hildenbrand
@ 2020-02-27 7:53 ` Janosch Frank
2020-02-27 8:09 ` Janosch Frank
1 sibling, 1 reply; 62+ messages in thread
From: Janosch Frank @ 2020-02-27 7:53 UTC (permalink / raw)
To: Cornelia Huck, David Hildenbrand; +Cc: borntraeger, qemu-s390x, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 3549 bytes --]
On 2/26/20 6:51 PM, Cornelia Huck wrote:
> On Wed, 26 Feb 2020 15:27:52 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
>> On 26.02.20 13:20, Janosch Frank wrote:
>>> Lets make it a bit more clear that we're extracting the 31 bit address
>
> s/Lets/Let's/ :)
Ack
>
>>> from the short psw.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>> hw/s390x/ipl.c | 2 +-
>>> target/s390x/cpu.c | 4 ++--
>>> target/s390x/cpu.h | 1 +
>>> 3 files changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>> index 7773499d7f..42e21e7a6a 100644
>>> --- a/hw/s390x/ipl.c
>>> +++ b/hw/s390x/ipl.c
>>> @@ -179,7 +179,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>> /* if not Linux load the address of the (short) IPL PSW */
>>> ipl_psw = rom_ptr(4, 4);
>>> if (ipl_psw) {
>>> - pentry = be32_to_cpu(*ipl_psw) & 0x7fffffffUL;
>>> + pentry = be32_to_cpu(*ipl_psw) & PSW_MASK_ESA_ADDR;
>>> } else {
>>> error_setg(&err, "Could not get IPL PSW");
>>> goto error;
>>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>>> index 8da1905485..43360912a0 100644
>>> --- a/target/s390x/cpu.c
>>> +++ b/target/s390x/cpu.c
>>> @@ -78,13 +78,13 @@ static void s390_cpu_load_normal(CPUState *s)
>>> S390CPU *cpu = S390_CPU(s);
>>> uint64_t spsw = ldq_phys(s->as, 0);
>>>
>>> - cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
>>> + cpu->env.psw.mask = spsw & PSW_MASK_ESA_MASK;
>>> /*
>>> * Invert short psw indication, so SIE will report a specification
>>> * exception if it was not set.
>>> */
>>> cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
>>> - cpu->env.psw.addr = spsw & 0x7fffffffULL;
>>> + cpu->env.psw.addr = spsw & PSW_MASK_ESA_ADDR;
>>>
>>> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>> }
>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>> index 8a557fd8d1..74e66fe0c2 100644
>>> --- a/target/s390x/cpu.h
>>> +++ b/target/s390x/cpu.h
>>> @@ -277,6 +277,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>>> #define PSW_MASK_64 0x0000000100000000ULL
>>> #define PSW_MASK_32 0x0000000080000000ULL
>>> #define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
>>> +#define PSW_MASK_ESA_MASK 0xffffffff80000000ULL
>>
>> ..._MASK_..._MASK
>>
>> Isn't there a better name for all the bits in the PSW that are not an
>> address?
>>
>> PSW_MASK_ESA_BITS
>> PSW_MASK_ESA_FLAGS
>> ...
>
> Hm, the PoP says that the PSW "includes the instruction address,
> condition code, and other control fields"; it also talks about the
> 'short' PSW as being distinct from the 'ESA' PSW (bit 31 may be 0 or 1
> in the short PSW). Maybe
>
> PSW_MASK_SHORT_ADDR
> PSW_MASK_SHORT_CTRL
Sure, why not
>
> (Or keep _ESA_ if renaming creates too much churn.)
>
>>
>>>
>>> #undef PSW_ASC_PRIMARY
>>> #undef PSW_ASC_ACCREG
>>>
>>
>>
>
> This patch is also independent of the protected virtualization
> support... I plan to send a pull request tomorrow, so I can include
> this patch, if we agree on a name for the constant :)
Well, you would also need to rename all users of PSW_MASK_ESA_ADDR
Let me split that up into two patches, the rename for the ADDR and this
one. I'll send it out once I'm more or less awake.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 01/18] s390x: Use constant for ESA PSW address
2020-02-27 7:53 ` Janosch Frank
@ 2020-02-27 8:09 ` Janosch Frank
2020-02-27 9:06 ` Cornelia Huck
0 siblings, 1 reply; 62+ messages in thread
From: Janosch Frank @ 2020-02-27 8:09 UTC (permalink / raw)
To: Cornelia Huck, David Hildenbrand; +Cc: borntraeger, qemu-s390x, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 3913 bytes --]
On 2/27/20 8:53 AM, Janosch Frank wrote:
> On 2/26/20 6:51 PM, Cornelia Huck wrote:
>> On Wed, 26 Feb 2020 15:27:52 +0100
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 26.02.20 13:20, Janosch Frank wrote:
>>>> Lets make it a bit more clear that we're extracting the 31 bit address
>>
>> s/Lets/Let's/ :)
>
> Ack
>
>>
>>>> from the short psw.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>> hw/s390x/ipl.c | 2 +-
>>>> target/s390x/cpu.c | 4 ++--
>>>> target/s390x/cpu.h | 1 +
>>>> 3 files changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>>> index 7773499d7f..42e21e7a6a 100644
>>>> --- a/hw/s390x/ipl.c
>>>> +++ b/hw/s390x/ipl.c
>>>> @@ -179,7 +179,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
>>>> /* if not Linux load the address of the (short) IPL PSW */
>>>> ipl_psw = rom_ptr(4, 4);
>>>> if (ipl_psw) {
>>>> - pentry = be32_to_cpu(*ipl_psw) & 0x7fffffffUL;
>>>> + pentry = be32_to_cpu(*ipl_psw) & PSW_MASK_ESA_ADDR;
>>>> } else {
>>>> error_setg(&err, "Could not get IPL PSW");
>>>> goto error;
>>>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>>>> index 8da1905485..43360912a0 100644
>>>> --- a/target/s390x/cpu.c
>>>> +++ b/target/s390x/cpu.c
>>>> @@ -78,13 +78,13 @@ static void s390_cpu_load_normal(CPUState *s)
>>>> S390CPU *cpu = S390_CPU(s);
>>>> uint64_t spsw = ldq_phys(s->as, 0);
>>>>
>>>> - cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
>>>> + cpu->env.psw.mask = spsw & PSW_MASK_ESA_MASK;
>>>> /*
>>>> * Invert short psw indication, so SIE will report a specification
>>>> * exception if it was not set.
>>>> */
>>>> cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
>>>> - cpu->env.psw.addr = spsw & 0x7fffffffULL;
>>>> + cpu->env.psw.addr = spsw & PSW_MASK_ESA_ADDR;
>>>>
>>>> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>>> }
>>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>>> index 8a557fd8d1..74e66fe0c2 100644
>>>> --- a/target/s390x/cpu.h
>>>> +++ b/target/s390x/cpu.h
>>>> @@ -277,6 +277,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>>>> #define PSW_MASK_64 0x0000000100000000ULL
>>>> #define PSW_MASK_32 0x0000000080000000ULL
>>>> #define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
>>>> +#define PSW_MASK_ESA_MASK 0xffffffff80000000ULL
>>>
>>> ..._MASK_..._MASK
>>>
>>> Isn't there a better name for all the bits in the PSW that are not an
>>> address?
>>>
>>> PSW_MASK_ESA_BITS
>>> PSW_MASK_ESA_FLAGS
>>> ...
>>
>> Hm, the PoP says that the PSW "includes the instruction address,
>> condition code, and other control fields"; it also talks about the
>> 'short' PSW as being distinct from the 'ESA' PSW (bit 31 may be 0 or 1
>> in the short PSW). Maybe
>>
>> PSW_MASK_SHORT_ADDR
>> PSW_MASK_SHORT_CTRL
>
> Sure, why not
>
>>
>> (Or keep _ESA_ if renaming creates too much churn.)
>>
>>>
>>>>
>>>> #undef PSW_ASC_PRIMARY
>>>> #undef PSW_ASC_ACCREG
>>>>
>>>
>>>
>>
>> This patch is also independent of the protected virtualization
>> support... I plan to send a pull request tomorrow, so I can include
>> this patch, if we agree on a name for the constant :)
>
> Well, you would also need to rename all users of PSW_MASK_ESA_ADDR
> Let me split that up into two patches, the rename for the ADDR and this
> one. I'll send it out once I'm more or less awake.
Seems like the ADDR constant has never been used anyway...
Ok, I renounce everything I said before, if you want to fix this up
yourself that would be wonderful, if not I'd be happy to provide you
with a patch.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 01/18] s390x: Use constant for ESA PSW address
2020-02-27 8:09 ` Janosch Frank
@ 2020-02-27 9:06 ` Cornelia Huck
2020-02-27 9:23 ` [PATCH v6] s390x: Rename and use constants for short PSW address and mask Janosch Frank
0 siblings, 1 reply; 62+ messages in thread
From: Cornelia Huck @ 2020-02-27 9:06 UTC (permalink / raw)
To: Janosch Frank; +Cc: borntraeger, qemu-s390x, qemu-devel, David Hildenbrand
[-- Attachment #1: Type: text/plain, Size: 4296 bytes --]
On Thu, 27 Feb 2020 09:09:47 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:
> On 2/27/20 8:53 AM, Janosch Frank wrote:
> > On 2/26/20 6:51 PM, Cornelia Huck wrote:
> >> On Wed, 26 Feb 2020 15:27:52 +0100
> >> David Hildenbrand <david@redhat.com> wrote:
> >>
> >>> On 26.02.20 13:20, Janosch Frank wrote:
> >>>> Lets make it a bit more clear that we're extracting the 31 bit address
> >>
> >> s/Lets/Let's/ :)
> >
> > Ack
> >
> >>
> >>>> from the short psw.
> >>>>
> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >>>> ---
> >>>> hw/s390x/ipl.c | 2 +-
> >>>> target/s390x/cpu.c | 4 ++--
> >>>> target/s390x/cpu.h | 1 +
> >>>> 3 files changed, 4 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> >>>> index 7773499d7f..42e21e7a6a 100644
> >>>> --- a/hw/s390x/ipl.c
> >>>> +++ b/hw/s390x/ipl.c
> >>>> @@ -179,7 +179,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
> >>>> /* if not Linux load the address of the (short) IPL PSW */
> >>>> ipl_psw = rom_ptr(4, 4);
> >>>> if (ipl_psw) {
> >>>> - pentry = be32_to_cpu(*ipl_psw) & 0x7fffffffUL;
> >>>> + pentry = be32_to_cpu(*ipl_psw) & PSW_MASK_ESA_ADDR;
> >>>> } else {
> >>>> error_setg(&err, "Could not get IPL PSW");
> >>>> goto error;
> >>>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> >>>> index 8da1905485..43360912a0 100644
> >>>> --- a/target/s390x/cpu.c
> >>>> +++ b/target/s390x/cpu.c
> >>>> @@ -78,13 +78,13 @@ static void s390_cpu_load_normal(CPUState *s)
> >>>> S390CPU *cpu = S390_CPU(s);
> >>>> uint64_t spsw = ldq_phys(s->as, 0);
> >>>>
> >>>> - cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
> >>>> + cpu->env.psw.mask = spsw & PSW_MASK_ESA_MASK;
> >>>> /*
> >>>> * Invert short psw indication, so SIE will report a specification
> >>>> * exception if it was not set.
> >>>> */
> >>>> cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
> >>>> - cpu->env.psw.addr = spsw & 0x7fffffffULL;
> >>>> + cpu->env.psw.addr = spsw & PSW_MASK_ESA_ADDR;
> >>>>
> >>>> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> >>>> }
> >>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> >>>> index 8a557fd8d1..74e66fe0c2 100644
> >>>> --- a/target/s390x/cpu.h
> >>>> +++ b/target/s390x/cpu.h
> >>>> @@ -277,6 +277,7 @@ extern const VMStateDescription vmstate_s390_cpu;
> >>>> #define PSW_MASK_64 0x0000000100000000ULL
> >>>> #define PSW_MASK_32 0x0000000080000000ULL
> >>>> #define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
> >>>> +#define PSW_MASK_ESA_MASK 0xffffffff80000000ULL
> >>>
> >>> ..._MASK_..._MASK
> >>>
> >>> Isn't there a better name for all the bits in the PSW that are not an
> >>> address?
> >>>
> >>> PSW_MASK_ESA_BITS
> >>> PSW_MASK_ESA_FLAGS
> >>> ...
> >>
> >> Hm, the PoP says that the PSW "includes the instruction address,
> >> condition code, and other control fields"; it also talks about the
> >> 'short' PSW as being distinct from the 'ESA' PSW (bit 31 may be 0 or 1
> >> in the short PSW). Maybe
> >>
> >> PSW_MASK_SHORT_ADDR
> >> PSW_MASK_SHORT_CTRL
> >
> > Sure, why not
> >
> >>
> >> (Or keep _ESA_ if renaming creates too much churn.)
> >>
> >>>
> >>>>
> >>>> #undef PSW_ASC_PRIMARY
> >>>> #undef PSW_ASC_ACCREG
> >>>>
> >>>
> >>>
> >>
> >> This patch is also independent of the protected virtualization
> >> support... I plan to send a pull request tomorrow, so I can include
> >> this patch, if we agree on a name for the constant :)
> >
> > Well, you would also need to rename all users of PSW_MASK_ESA_ADDR
> > Let me split that up into two patches, the rename for the ADDR and this
> > one. I'll send it out once I'm more or less awake.
>
> Seems like the ADDR constant has never been used anyway...
> Ok, I renounce everything I said before, if you want to fix this up
> yourself that would be wonderful, if not I'd be happy to provide you
> with a patch.
A quick respin of this patch would be easiest for me.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread* [PATCH v6] s390x: Rename and use constants for short PSW address and mask
2020-02-27 9:06 ` Cornelia Huck
@ 2020-02-27 9:23 ` Janosch Frank
2020-02-27 9:27 ` David Hildenbrand
2020-02-27 10:36 ` Cornelia Huck
0 siblings, 2 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-27 9:23 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Let's rename PSW_MASK_ESA_ADDR to PSW_MASK_SHORT_ADDR because we're
not working with a ESA PSW which would not support the extended
addressing bit. Also let's actually use it.
Additionally we introduce PSW_MASK_SHORT_CTRL and use it throughout
the codebase.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
hw/s390x/ipl.c | 2 +-
target/s390x/cpu.c | 4 ++--
target/s390x/cpu.h | 3 ++-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 7773499d7f..e36f770a72 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -179,7 +179,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
/* if not Linux load the address of the (short) IPL PSW */
ipl_psw = rom_ptr(4, 4);
if (ipl_psw) {
- pentry = be32_to_cpu(*ipl_psw) & 0x7fffffffUL;
+ pentry = be32_to_cpu(*ipl_psw) & PSW_MASK_SHORT_ADDR;
} else {
error_setg(&err, "Could not get IPL PSW");
goto error;
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 8da1905485..3dd396e870 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -78,13 +78,13 @@ static void s390_cpu_load_normal(CPUState *s)
S390CPU *cpu = S390_CPU(s);
uint64_t spsw = ldq_phys(s->as, 0);
- cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
+ cpu->env.psw.mask = spsw & PSW_MASK_SHORT_CTRL;
/*
* Invert short psw indication, so SIE will report a specification
* exception if it was not set.
*/
cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
- cpu->env.psw.addr = spsw & 0x7fffffffULL;
+ cpu->env.psw.addr = spsw & PSW_MASK_SHORT_ADDR;
s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
}
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 8a557fd8d1..1d17709d6e 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -276,7 +276,8 @@ extern const VMStateDescription vmstate_s390_cpu;
#define PSW_MASK_RI 0x0000008000000000ULL
#define PSW_MASK_64 0x0000000100000000ULL
#define PSW_MASK_32 0x0000000080000000ULL
-#define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
+#define PSW_MASK_SHORT_ADDR 0x000000007fffffffULL
+#define PSW_MASK_SHORT_CTRL 0xffffffff80000000ULL
#undef PSW_ASC_PRIMARY
#undef PSW_ASC_ACCREG
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v6] s390x: Rename and use constants for short PSW address and mask
2020-02-27 9:23 ` [PATCH v6] s390x: Rename and use constants for short PSW address and mask Janosch Frank
@ 2020-02-27 9:27 ` David Hildenbrand
2020-02-27 10:36 ` Cornelia Huck
1 sibling, 0 replies; 62+ messages in thread
From: David Hildenbrand @ 2020-02-27 9:27 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
On 27.02.20 10:23, Janosch Frank wrote:
> Let's rename PSW_MASK_ESA_ADDR to PSW_MASK_SHORT_ADDR because we're
> not working with a ESA PSW which would not support the extended
> addressing bit. Also let's actually use it.
>
> Additionally we introduce PSW_MASK_SHORT_CTRL and use it throughout
> the codebase.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> hw/s390x/ipl.c | 2 +-
> target/s390x/cpu.c | 4 ++--
> target/s390x/cpu.h | 3 ++-
> 3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 7773499d7f..e36f770a72 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -179,7 +179,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
> /* if not Linux load the address of the (short) IPL PSW */
> ipl_psw = rom_ptr(4, 4);
> if (ipl_psw) {
> - pentry = be32_to_cpu(*ipl_psw) & 0x7fffffffUL;
> + pentry = be32_to_cpu(*ipl_psw) & PSW_MASK_SHORT_ADDR;
> } else {
> error_setg(&err, "Could not get IPL PSW");
> goto error;
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 8da1905485..3dd396e870 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -78,13 +78,13 @@ static void s390_cpu_load_normal(CPUState *s)
> S390CPU *cpu = S390_CPU(s);
> uint64_t spsw = ldq_phys(s->as, 0);
>
> - cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
> + cpu->env.psw.mask = spsw & PSW_MASK_SHORT_CTRL;
> /*
> * Invert short psw indication, so SIE will report a specification
> * exception if it was not set.
> */
> cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
> - cpu->env.psw.addr = spsw & 0x7fffffffULL;
> + cpu->env.psw.addr = spsw & PSW_MASK_SHORT_ADDR;
>
> s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> }
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 8a557fd8d1..1d17709d6e 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -276,7 +276,8 @@ extern const VMStateDescription vmstate_s390_cpu;
> #define PSW_MASK_RI 0x0000008000000000ULL
> #define PSW_MASK_64 0x0000000100000000ULL
> #define PSW_MASK_32 0x0000000080000000ULL
> -#define PSW_MASK_ESA_ADDR 0x000000007fffffffULL
> +#define PSW_MASK_SHORT_ADDR 0x000000007fffffffULL
> +#define PSW_MASK_SHORT_CTRL 0xffffffff80000000ULL
>
> #undef PSW_ASC_PRIMARY
> #undef PSW_ASC_ACCREG
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v6] s390x: Rename and use constants for short PSW address and mask
2020-02-27 9:23 ` [PATCH v6] s390x: Rename and use constants for short PSW address and mask Janosch Frank
2020-02-27 9:27 ` David Hildenbrand
@ 2020-02-27 10:36 ` Cornelia Huck
1 sibling, 0 replies; 62+ messages in thread
From: Cornelia Huck @ 2020-02-27 10:36 UTC (permalink / raw)
To: Janosch Frank; +Cc: borntraeger, qemu-s390x, qemu-devel, david
On Thu, 27 Feb 2020 04:23:41 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:
> Let's rename PSW_MASK_ESA_ADDR to PSW_MASK_SHORT_ADDR because we're
> not working with a ESA PSW which would not support the extended
> addressing bit. Also let's actually use it.
>
> Additionally we introduce PSW_MASK_SHORT_CTRL and use it throughout
> the codebase.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> hw/s390x/ipl.c | 2 +-
> target/s390x/cpu.c | 4 ++--
> target/s390x/cpu.h | 3 ++-
> 3 files changed, 5 insertions(+), 4 deletions(-)
Thanks, applied.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v5 02/18] Sync pv
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
2020-02-26 12:20 ` [PATCH v5 01/18] s390x: Use constant for ESA PSW address Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 03/18] s390x: protvirt: Add diag308 subcodes 8 - 10 Janosch Frank
` (17 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
| 43 +++++++++++++++++++++++++++++++++++++--
1 file changed, 41 insertions(+), 2 deletions(-)
--git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 265099100e..e36f761194 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -474,8 +474,11 @@ struct kvm_s390_mem_op {
__u32 size; /* amount of bytes */
__u32 op; /* type of operation */
__u64 buf; /* buffer in userspace */
- __u8 ar; /* the access register number */
- __u8 reserved[31]; /* should be set to 0 */
+ union {
+ __u8 ar; /* the access register number */
+ __u32 sida_offset; /* offset into the sida */
+ __u8 reserved[32]; /* should be set to 0 */
+ };
};
/* types for kvm_s390_mem_op->op */
#define KVM_S390_MEMOP_LOGICAL_READ 0
@@ -1010,6 +1013,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_ARM_NISV_TO_USER 177
#define KVM_CAP_ARM_INJECT_EXT_DABT 178
#define KVM_CAP_S390_VCPU_RESETS 179
+#define KVM_CAP_S390_PROTECTED 180
#ifdef KVM_CAP_IRQ_ROUTING
@@ -1478,6 +1482,41 @@ struct kvm_enc_region {
#define KVM_S390_NORMAL_RESET _IO(KVMIO, 0xc3)
#define KVM_S390_CLEAR_RESET _IO(KVMIO, 0xc4)
+struct kvm_s390_pv_sec_parm {
+ __u64 origin;
+ __u64 length;
+};
+
+struct kvm_s390_pv_unp {
+ __u64 addr;
+ __u64 size;
+ __u64 tweak;
+};
+
+enum pv_cmd_id {
+ KVM_PV_ENABLE,
+ KVM_PV_DISABLE,
+ KVM_PV_VM_SET_SEC_PARMS,
+ KVM_PV_VM_UNPACK,
+ KVM_PV_VM_VERIFY,
+ KVM_PV_VM_PREP_RESET,
+ KVM_PV_VM_UNSHARE_ALL,
+ KVM_PV_VCPU_CREATE,
+ KVM_PV_VCPU_DESTROY,
+};
+
+struct kvm_pv_cmd {
+ __u32 cmd; /* Command to be executed */
+ __u16 rc; /* Ultravisor return code */
+ __u16 rrc; /* Ultravisor return reason code */
+ __u64 data; /* Data or address */
+ __u32 flags; /* flags for future extensions. Must be 0 for now */
+ __u32 reserved[3];
+};
+
+/* Available with KVM_CAP_S390_PROTECTED */
+#define KVM_S390_PV_COMMAND _IOWR(KVMIO, 0xc5, struct kvm_pv_cmd)
+
/* Secure Encrypted Virtualization command */
enum sev_cmd_id {
/* Guest initialization commands */
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 03/18] s390x: protvirt: Add diag308 subcodes 8 - 10
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
2020-02-26 12:20 ` [PATCH v5 01/18] s390x: Use constant for ESA PSW address Janosch Frank
2020-02-26 12:20 ` [PATCH v5 02/18] Sync pv Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 04/18] s390x: protvirt: Support unpack facility Janosch Frank
` (16 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
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 <frankja@linux.ibm.com>
---
hw/s390x/ipl.c | 47 ++++++++++++++++++++++++++++++++++++++++++---
hw/s390x/ipl.h | 31 ++++++++++++++++++++++++++++++
target/s390x/diag.c | 27 +++++++++++++++++++++++---
3 files changed, 99 insertions(+), 6 deletions(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 42e21e7a6a..a017b3edda 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)
+{
+ int i;
+ IPLBlockPV *ipib_pv = &iplb->pv;
+
+ 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 */
+ if (i < ipib_pv->num_comp - 1 &&
+ ipib_pv->components[i].tweak_pref >
+ ipib_pv->components[i + 1].tweak_pref) {
+ 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)
+{
+ 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) {
/* 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..3c4bb66eda 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -15,6 +15,23 @@
#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 reserved[87];
+ uint8_t version;
+ uint32_t num_comp;
+ uint64_t pv_header_addr;
+ uint64_t pv_header_len;
+ struct IPLBlockPVComp components[];
+} QEMU_PACKED;
+typedef struct IPLBlockPV IPLBlockPV;
+
struct IplBlockCcw {
uint8_t reserved0[85];
uint8_t ssid;
@@ -71,6 +88,7 @@ union IplParameterBlock {
union {
IplBlockCcw ccw;
IplBlockFcp fcp;
+ IPLBlockPV pv;
IplBlockQemuScsi scsi;
};
} QEMU_PACKED;
@@ -84,9 +102,11 @@ union IplParameterBlock {
typedef union IplParameterBlock IplParameterBlock;
int s390_ipl_set_loadparm(uint8_t *loadparm);
+int s390_ipl_pv_check_components(IplParameterBlock *iplb);
void s390_ipl_update_diag308(IplParameterBlock *iplb);
void s390_ipl_prepare_cpu(S390CPU *cpu);
IplParameterBlock *s390_ipl_get_iplb(void);
+IplParameterBlock *s390_ipl_get_iplb_secure(void);
enum s390_reset {
/* default is a reset not triggered by a CPU e.g. issued by QMP */
@@ -94,6 +114,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 +154,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 +162,7 @@ struct S390IPLState {
uint64_t compat_bios_start_addr;
bool enforce_bios;
bool iplb_valid;
+ bool iplb_valid_pv;
bool netboot;
/* reset related properties don't have to be migrated or reset */
enum s390_reset reset_type;
@@ -161,9 +184,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 +210,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..d6ceb1f75d 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -52,6 +52,8 @@ 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 0x0a02
+#define DIAG_308_RC_INVAL_FOR_PV 0x0b02
#define DIAG308_RESET_MOD_CLR 0
#define DIAG308_RESET_LOAD_NORM 1
@@ -59,6 +61,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 +110,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 +123,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))) {
env->regs[r1 + 1] = DIAG_308_RC_INVALID;
goto out;
}
@@ -128,17 +135,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)) {
+ env->regs[r1 + 1] = DIAG_308_RC_NO_PV_CONF;
+ return;
+ }
+
+ s390_ipl_reset_request(cs, S390_RESET_PV);
+ break;
default:
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
break;
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 04/18] s390x: protvirt: Support unpack facility
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (2 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 03/18] s390x: protvirt: Add diag308 subcodes 8 - 10 Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 05/18] s390x: protvirt: Add migration blocker Janosch Frank
` (15 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
When a guest has saved a ipib of type 5 and calls diagnose308 with
subcode 10, we have to setup the protected processing environment via
Ultravisor calls. The calls are done by KVM and are exposed via an
API.
The following steps are necessary:
1. Enable protected mode for the VM (register it and its cpus with the Ultravisor)
2. Forward the secure header to the Ultravisor (has all information on
how to decrypt the image and VM information)
3. Protect image pages from the host and decrypt them
4. Verify the image integrity
Only after step 4 a protected VM is allowed to run.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> [Changes
to machine]
---
hw/s390x/Makefile.objs | 1 +
hw/s390x/ipl.c | 33 +++++++++
hw/s390x/ipl.h | 2 +
hw/s390x/pv.c | 106 ++++++++++++++++++++++++++++
hw/s390x/pv.h | 34 +++++++++
hw/s390x/s390-virtio-ccw.c | 87 +++++++++++++++++++++++
include/hw/s390x/s390-virtio-ccw.h | 1 +
target/s390x/cpu.c | 4 ++
target/s390x/cpu.h | 1 +
target/s390x/cpu_features_def.inc.h | 1 +
10 files changed, 270 insertions(+)
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 a017b3edda..53876637ee 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_START 0x010000UL
#define LINUX_MAGIC_ADDR 0x010008UL
@@ -676,6 +677,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 = &ipl->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 = &ipl->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 3c4bb66eda..92467738a7 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -104,6 +104,8 @@ typedef union IplParameterBlock IplParameterBlock;
int s390_ipl_set_loadparm(uint8_t *loadparm);
int s390_ipl_pv_check_components(IplParameterBlock *iplb);
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_secure(void);
diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
new file mode 100644
index 0000000000..50b68b6c34
--- /dev/null
+++ b/hw/s390x/pv.c
@@ -0,0 +1,106 @@
+/*
+ * Secure execution functions
+ *
+ * Copyright IBM Corp. 2020
+ * Author(s):
+ * Janosch Frank <frankja@linux.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+#include "qemu/osdep.h"
+#include <sys/ioctl.h>
+
+#include <linux/kvm.h>
+
+#include "qemu/error-report.h"
+#include "sysemu/kvm.h"
+#include "pv.h"
+
+const char *cmd_names[] = {
+ "VM_ENABLE",
+ "VM_DISABLE",
+ "VM_SET_SEC_PARAMS",
+ "VM_UNPACK",
+ "VM_VERIFY",
+ "VM_PREP_RESET",
+ "VM_UNSHARE_ALL",
+ NULL
+};
+
+static int s390_pv_cmd(uint32_t cmd, void *data)
+{
+ int rc;
+ struct kvm_pv_cmd pv_cmd = {
+ .cmd = cmd,
+ .data = (uint64_t)data,
+ };
+
+ rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, &pv_cmd);
+ if (rc) {
+ error_report("KVM PV command %d (%s) failed: header rc %x rrc %x "
+ "IOCTL rc: %d", cmd, cmd_names[cmd], pv_cmd.rc, pv_cmd.rrc,
+ rc);
+ }
+ return rc;
+}
+
+static void s390_pv_cmd_exit(uint32_t cmd, void *data)
+{
+ int rc;
+
+ rc = s390_pv_cmd(cmd, data);
+ if (rc) {
+ exit(1);
+ }
+}
+
+int s390_pv_vm_enable(void)
+{
+ return s390_pv_cmd(KVM_PV_ENABLE, NULL);
+}
+
+void s390_pv_vm_disable(void)
+{
+ s390_pv_cmd_exit(KVM_PV_DISABLE, NULL);
+}
+
+int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
+{
+ struct kvm_s390_pv_sec_parm args = {
+ .origin = origin,
+ .length = length,
+ };
+
+ return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
+}
+
+/*
+ * Called for each component in the SE type IPL parameter block 0.
+ */
+int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
+{
+ struct kvm_s390_pv_unp args = {
+ .addr = addr,
+ .size = size,
+ .tweak = tweak,
+ };
+
+ return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
+}
+
+void s390_pv_perf_clear_reset(void)
+{
+ s390_pv_cmd_exit(KVM_PV_VM_PREP_RESET, NULL);
+}
+
+int s390_pv_verify(void)
+{
+ return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
+}
+
+void s390_pv_unshare(void)
+{
+ s390_pv_cmd_exit(KVM_PV_VM_UNSHARE_ALL, NULL);
+}
diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
new file mode 100644
index 0000000000..936262889f
--- /dev/null
+++ b/hw/s390x/pv.h
@@ -0,0 +1,34 @@
+/*
+ * Protected Virtualization header
+ *
+ * Copyright IBM Corp. 2020
+ * Author(s):
+ * Janosch Frank <frankja@linux.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef HW_S390_PV_H
+#define HW_S390_PV_H
+
+#ifdef CONFIG_KVM
+int s390_pv_vm_enable(void);
+void s390_pv_vm_disable(void);
+int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
+int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
+void s390_pv_perf_clear_reset(void);
+int s390_pv_verify(void);
+void s390_pv_unshare(void);
+#else
+int s390_pv_vm_enable(void) { return 0; }
+void s390_pv_vm_disable(void) {}
+int s390_pv_set_sec_parms(uint64_t origin, uint64_t length) { return 0; }
+int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { return 0; }
+void s390_pv_perf_clear_reset(void) {}
+int s390_pv_verify(void) { return 0; }
+void s390_pv_unshare(void) {}
+#endif
+
+#endif /* HW_S390_PV_H */
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e759eb5f83..e506dd65ed 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -41,6 +41,7 @@
#include "hw/qdev-properties.h"
#include "hw/s390x/tod.h"
#include "sysemu/sysemu.h"
+#include "hw/s390x/pv.h"
S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
{
@@ -240,9 +241,11 @@ static void s390_create_sclpconsole(const char *type, Chardev *chardev)
static void ccw_init(MachineState *machine)
{
int ret;
+ S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
VirtualCssBus *css_bus;
DeviceState *dev;
+ ms->pv = false;
s390_sclp_init();
/* init memory + setup max page size. Required for the CPU model */
s390_memory_init(machine->ram_size);
@@ -318,10 +321,72 @@ static inline void s390_do_cpu_ipl(CPUState *cs, run_on_cpu_data arg)
s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
}
+static void s390_machine_unprotect(S390CcwMachineState *ms)
+{
+ CPUState *t;
+
+ s390_pv_vm_disable();
+ CPU_FOREACH(t) {
+ S390_CPU(t)->env.pv = false;
+ }
+ ms->pv = false;
+}
+
+static int s390_machine_protect(S390CcwMachineState *ms)
+{
+ CPUState *t;
+ int rc;
+
+ /* Create SE VM */
+ rc = s390_pv_vm_enable();
+ if (rc) {
+ return rc;
+ }
+
+ CPU_FOREACH(t) {
+ S390_CPU(t)->env.pv = true;
+ }
+ ms->pv = true;
+
+ /* Set SE header and unpack */
+ rc = s390_ipl_prepare_pv_header();
+ if (rc) {
+ goto out_err;
+ }
+
+ /* Decrypt image */
+ rc = s390_ipl_pv_unpack();
+ if (rc) {
+ goto out_err;
+ }
+
+ /* Verify integrity */
+ rc = s390_pv_verify();
+ if (rc) {
+ goto out_err;
+ }
+ return rc;
+
+out_err:
+ s390_machine_unprotect(ms);
+ return rc;
+}
+
+static void s390_machine_inject_pv_error(CPUState *cs)
+{
+ int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4;
+ CPUS390XState *env = &S390_CPU(cs)->env;
+
+ /* Report that we are unable to enter protected mode */
+ env->regs[r1 + 1] = 0xa02;
+}
+
static void s390_machine_reset(MachineState *machine)
{
enum s390_reset reset_type;
CPUState *cs, *t;
+ S390CPU *cpu;
+ S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
/* get the reset parameters, reset them once done */
s390_ipl_get_reset_request(&cs, &reset_type);
@@ -329,6 +394,8 @@ static void s390_machine_reset(MachineState *machine)
/* all CPUs are paused and synchronized at this point */
s390_cmma_reset();
+ cpu = S390_CPU(cs);
+
switch (reset_type) {
case S390_RESET_EXTERNAL:
case S390_RESET_REIPL:
@@ -355,6 +422,26 @@ static void s390_machine_reset(MachineState *machine)
}
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);
break;
default:
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 8aa27199c9..cd1dccc6e3 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -28,6 +28,7 @@ typedef struct S390CcwMachineState {
/*< public >*/
bool aes_key_wrap;
bool dea_key_wrap;
+ bool pv;
uint8_t loadparm[8];
} S390CcwMachineState;
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 43360912a0..c4d937e35a 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -37,6 +37,8 @@
#include "sysemu/hw_accel.h"
#include "hw/qdev-properties.h"
#ifndef CONFIG_USER_ONLY
+#include "hw/s390x/s390-virtio-ccw.h"
+#include "hw/s390x/pv.h"
#include "hw/boards.h"
#include "sysemu/arch_init.h"
#include "sysemu/sysemu.h"
@@ -191,6 +193,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
#if !defined(CONFIG_USER_ONLY)
MachineState *ms = MACHINE(qdev_get_machine());
+ S390CcwMachineState *ccw = S390_CCW_MACHINE(ms);
unsigned int max_cpus = ms->smp.max_cpus;
if (cpu->env.core_id >= max_cpus) {
error_setg(&err, "Unable to add CPU with core-id: %" PRIu32
@@ -205,6 +208,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
goto out;
}
+ cpu->env.pv = ccw->pv;
/* sync cs->cpu_index and env->core_id. The latter is needed for TCG. */
cs->cpu_index = cpu->env.core_id;
#endif
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 74e66fe0c2..acff5c9a65 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -114,6 +114,7 @@ struct CPUS390XState {
/* Fields up to this point are cleared by a CPU reset */
struct {} end_reset_fields;
+ bool pv; /* protected virtualization */
#if !defined(CONFIG_USER_ONLY)
uint32_t core_id; /* PoP "CPU address", same as cpu_index */
diff --git a/target/s390x/cpu_features_def.inc.h b/target/s390x/cpu_features_def.inc.h
index 31dff0d84e..60db28351d 100644
--- a/target/s390x/cpu_features_def.inc.h
+++ b/target/s390x/cpu_features_def.inc.h
@@ -107,6 +107,7 @@ DEF_FEAT(DEFLATE_BASE, "deflate-base", STFL, 151, "Deflate-conversion facility (
DEF_FEAT(VECTOR_PACKED_DECIMAL_ENH, "vxpdeh", STFL, 152, "Vector-Packed-Decimal-Enhancement Facility")
DEF_FEAT(MSA_EXT_9, "msa9-base", STFL, 155, "Message-security-assist-extension-9 facility (excluding subfunctions)")
DEF_FEAT(ETOKEN, "etoken", STFL, 156, "Etoken facility")
+DEF_FEAT(UNPACK, "unpack", STFL, 161, "Unpack facility")
/* Features exposed via SCLP SCCB Byte 80 - 98 (bit numbers relative to byte-80) */
DEF_FEAT(SIE_GSLS, "gsls", SCLP_CONF_CHAR, 40, "SIE: Guest-storage-limit-suppression facility")
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 05/18] s390x: protvirt: Add migration blocker
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (3 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 04/18] s390x: protvirt: Support unpack facility Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 14:05 ` Christian Borntraeger
2020-02-26 12:20 ` [PATCH v5 06/18] s390x: protvirt: Handle diag 308 subcodes 0,1,3,4 Janosch Frank
` (14 subsequent siblings)
19 siblings, 1 reply; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Migration is not yet supported.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e506dd65ed..79f472c309 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -42,6 +42,9 @@
#include "hw/s390x/tod.h"
#include "sysemu/sysemu.h"
#include "hw/s390x/pv.h"
+#include "migration/blocker.h"
+
+static Error *pv_mig_blocker;
S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
{
@@ -325,18 +328,30 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
{
CPUState *t;
- s390_pv_vm_disable();
- CPU_FOREACH(t) {
- S390_CPU(t)->env.pv = false;
+ if (ms->pv) {
+ s390_pv_vm_disable();
+ CPU_FOREACH(t) {
+ S390_CPU(t)->env.pv = false;
+ }
+ ms->pv = false;
}
- ms->pv = false;
}
static int s390_machine_protect(S390CcwMachineState *ms)
{
+ static Error *local_err;
CPUState *t;
int rc;
+ if (!pv_mig_blocker) {
+ error_setg(&pv_mig_blocker,
+ "protected VMs are currently not migrateable.");
+ }
+ migrate_add_blocker(pv_mig_blocker, &local_err);
+ if (local_err) {
+ goto out_err;
+ }
+
/* Create SE VM */
rc = s390_pv_vm_enable();
if (rc) {
@@ -438,11 +453,12 @@ static void s390_machine_reset(MachineState *machine)
if (s390_machine_protect(ms)) {
s390_machine_inject_pv_error(cs);
- s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
- return;
+ goto pv_err;
}
run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
+pv_err:
+ s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
break;
default:
g_assert_not_reached();
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v5 05/18] s390x: protvirt: Add migration blocker
2020-02-26 12:20 ` [PATCH v5 05/18] s390x: protvirt: Add migration blocker Janosch Frank
@ 2020-02-26 14:05 ` Christian Borntraeger
2020-02-26 14:08 ` Janosch Frank
0 siblings, 1 reply; 62+ messages in thread
From: Christian Borntraeger @ 2020-02-26 14:05 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck, david
On 26.02.20 13:20, Janosch Frank wrote:
> Migration is not yet supported.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++------
> 1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e506dd65ed..79f472c309 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -42,6 +42,9 @@
> #include "hw/s390x/tod.h"
> #include "sysemu/sysemu.h"
> #include "hw/s390x/pv.h"
> +#include "migration/blocker.h"
> +
> +static Error *pv_mig_blocker;
>
> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> {
> @@ -325,18 +328,30 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
> {
> CPUState *t;
>
> - s390_pv_vm_disable();
> - CPU_FOREACH(t) {
> - S390_CPU(t)->env.pv = false;
> + if (ms->pv) {
> + s390_pv_vm_disable();
> + CPU_FOREACH(t) {
> + S390_CPU(t)->env.pv = false;
> + }
> + ms->pv = false;
> }
> - ms->pv = false;
> }
Shouldnt that hunk go into the previous patch?
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 05/18] s390x: protvirt: Add migration blocker
2020-02-26 14:05 ` Christian Borntraeger
@ 2020-02-26 14:08 ` Janosch Frank
0 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 14:08 UTC (permalink / raw)
To: Christian Borntraeger, qemu-devel; +Cc: qemu-s390x, cohuck, david
[-- Attachment #1.1: Type: text/plain, Size: 1309 bytes --]
On 2/26/20 3:05 PM, Christian Borntraeger wrote:
>
>
> On 26.02.20 13:20, Janosch Frank wrote:
>> Migration is not yet supported.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> hw/s390x/s390-virtio-ccw.c | 28 ++++++++++++++++++++++------
>> 1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index e506dd65ed..79f472c309 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -42,6 +42,9 @@
>> #include "hw/s390x/tod.h"
>> #include "sysemu/sysemu.h"
>> #include "hw/s390x/pv.h"
>> +#include "migration/blocker.h"
>> +
>> +static Error *pv_mig_blocker;
>>
>> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>> {
>> @@ -325,18 +328,30 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
>> {
>> CPUState *t;
>>
>> - s390_pv_vm_disable();
>> - CPU_FOREACH(t) {
>> - S390_CPU(t)->env.pv = false;
>> + if (ms->pv) {
>> + s390_pv_vm_disable();
>> + CPU_FOREACH(t) {
>> + S390_CPU(t)->env.pv = false;
>> + }
>> + ms->pv = false;
>> }
>> - ms->pv = false;
>> }
>
> Shouldnt that hunk go into the previous patch?
>
>
Yes, something went wrong
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v5 06/18] s390x: protvirt: Handle diag 308 subcodes 0,1,3,4
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (4 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 05/18] s390x: protvirt: Add migration blocker Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 15:00 ` Christian Borntraeger
2020-02-26 12:20 ` [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode Janosch Frank
` (13 subsequent siblings)
19 siblings, 1 reply; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
As we now have access to the protection state of the cpus, we can
implement special handling of diag 308 subcodes for cpus in the
protected state.
For subcodes 0 and 1 we need to unshare all pages before continuing,
so the guest doesn't accidentally expose data when dumping.
For subcode 3/4 we tear down the protected VM and reboot into
unprotected mode. We do not provide a secure reboot.
Before we can do the unshare calls, we need to mark all cpus as
stopped.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
hw/s390x/s390-virtio-ccw.c | 37 ++++++++++++++++++++++++++++++++++---
target/s390x/diag.c | 4 ++++
2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 79f472c309..9983165b05 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -335,6 +335,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
}
ms->pv = false;
}
+ migrate_del_blocker(pv_mig_blocker);
}
static int s390_machine_protect(S390CcwMachineState *ms)
@@ -396,12 +397,27 @@ static void s390_machine_inject_pv_error(CPUState *cs)
env->regs[r1 + 1] = 0xa02;
}
+static void s390_pv_prepare_reset(CPUS390XState *env)
+{
+ CPUState *cs;
+
+ if (!env->pv) {
+ return;
+ }
+ CPU_FOREACH(cs) {
+ s390_cpu_set_state(S390_CPU_STATE_STOPPED, S390_CPU(cs));
+ }
+ s390_pv_unshare();
+ s390_pv_perf_clear_reset();
+}
+
static void s390_machine_reset(MachineState *machine)
{
enum s390_reset reset_type;
CPUState *cs, *t;
S390CPU *cpu;
S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
+ CPUS390XState *env;
/* get the reset parameters, reset them once done */
s390_ipl_get_reset_request(&cs, &reset_type);
@@ -410,10 +426,15 @@ static void s390_machine_reset(MachineState *machine)
s390_cmma_reset();
cpu = S390_CPU(cs);
+ env = &cpu->env;
switch (reset_type) {
case S390_RESET_EXTERNAL:
case S390_RESET_REIPL:
+ if (ms->pv) {
+ s390_machine_unprotect(ms);
+ }
+
qemu_devices_reset();
s390_crypto_reset();
@@ -421,21 +442,31 @@ 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;
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index d6ceb1f75d..840335d40a 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -68,6 +68,10 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
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;
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v5 06/18] s390x: protvirt: Handle diag 308 subcodes 0,1,3,4
2020-02-26 12:20 ` [PATCH v5 06/18] s390x: protvirt: Handle diag 308 subcodes 0,1,3,4 Janosch Frank
@ 2020-02-26 15:00 ` Christian Borntraeger
2020-02-26 15:11 ` Janosch Frank
0 siblings, 1 reply; 62+ messages in thread
From: Christian Borntraeger @ 2020-02-26 15:00 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck, david
On 26.02.20 13:20, Janosch Frank wrote:
> As we now have access to the protection state of the cpus, we can
> implement special handling of diag 308 subcodes for cpus in the
> protected state.
>
> For subcodes 0 and 1 we need to unshare all pages before continuing,
> so the guest doesn't accidentally expose data when dumping.
>
> For subcode 3/4 we tear down the protected VM and reboot into
> unprotected mode. We do not provide a secure reboot.
>
> Before we can do the unshare calls, we need to mark all cpus as
> stopped.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 37 ++++++++++++++++++++++++++++++++++---
> target/s390x/diag.c | 4 ++++
> 2 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 79f472c309..9983165b05 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -335,6 +335,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
> }
> ms->pv = false;
> }
> + migrate_del_blocker(pv_mig_blocker);
> }
>
and that part into patch 5?
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 06/18] s390x: protvirt: Handle diag 308 subcodes 0,1,3,4
2020-02-26 15:00 ` Christian Borntraeger
@ 2020-02-26 15:11 ` Janosch Frank
0 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 15:11 UTC (permalink / raw)
To: Christian Borntraeger, qemu-devel; +Cc: qemu-s390x, cohuck, david
[-- Attachment #1.1: Type: text/plain, Size: 1323 bytes --]
On 2/26/20 4:00 PM, Christian Borntraeger wrote:
>
>
> On 26.02.20 13:20, Janosch Frank wrote:
>> As we now have access to the protection state of the cpus, we can
>> implement special handling of diag 308 subcodes for cpus in the
>> protected state.
>>
>> For subcodes 0 and 1 we need to unshare all pages before continuing,
>> so the guest doesn't accidentally expose data when dumping.
>>
>> For subcode 3/4 we tear down the protected VM and reboot into
>> unprotected mode. We do not provide a secure reboot.
>>
>> Before we can do the unshare calls, we need to mark all cpus as
>> stopped.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> hw/s390x/s390-virtio-ccw.c | 37 ++++++++++++++++++++++++++++++++++---
>> target/s390x/diag.c | 4 ++++
>> 2 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 79f472c309..9983165b05 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -335,6 +335,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
>> }
>> ms->pv = false;
>> }
>> + migrate_del_blocker(pv_mig_blocker);
>> }
>>
>
> and that part into patch 5?
>
>
Already fixed in the branch :-)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (5 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 06/18] s390x: protvirt: Handle diag 308 subcodes 0,1,3,4 Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 14:59 ` David Hildenbrand
2020-02-26 12:20 ` [PATCH v5 08/18] s390x: protvirt: KVM intercept changes Janosch Frank
` (12 subsequent siblings)
19 siblings, 1 reply; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Ballooning in protected VMs can only be done when the guest shares the
pages it gives to the host. Hence, until we have a solution for this
in the guest kernel, we inhibit ballooning when switching into
protected mode and reverse that once we move out of it.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
hw/s390x/s390-virtio-ccw.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 9983165b05..0f4455d1df 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -41,6 +41,7 @@
#include "hw/qdev-properties.h"
#include "hw/s390x/tod.h"
#include "sysemu/sysemu.h"
+#include "sysemu/balloon.h"
#include "hw/s390x/pv.h"
#include "migration/blocker.h"
@@ -336,6 +337,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
ms->pv = false;
}
migrate_del_blocker(pv_mig_blocker);
+ qemu_balloon_inhibit(false);
}
static int s390_machine_protect(S390CcwMachineState *ms)
@@ -344,6 +346,7 @@ static int s390_machine_protect(S390CcwMachineState *ms)
CPUState *t;
int rc;
+ qemu_balloon_inhibit(true);
if (!pv_mig_blocker) {
error_setg(&pv_mig_blocker,
"protected VMs are currently not migrateable.");
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-26 12:20 ` [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode Janosch Frank
@ 2020-02-26 14:59 ` David Hildenbrand
2020-02-26 15:06 ` Christian Borntraeger
2020-02-26 15:11 ` Janosch Frank
0 siblings, 2 replies; 62+ messages in thread
From: David Hildenbrand @ 2020-02-26 14:59 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
On 26.02.20 13:20, Janosch Frank wrote:
> Ballooning in protected VMs can only be done when the guest shares the
> pages it gives to the host. Hence, until we have a solution for this
> in the guest kernel, we inhibit ballooning when switching into
> protected mode and reverse that once we move out of it.
I don't understand what you mean here, sorry. zapping a page will mean
that a fresh one will be faulted in when accessed. And AFAIK, that means
it will be encrypted again when needed.
Is that more like the UV will detect this as an integrity issue and
crash the VM?
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 9983165b05..0f4455d1df 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -41,6 +41,7 @@
> #include "hw/qdev-properties.h"
> #include "hw/s390x/tod.h"
> #include "sysemu/sysemu.h"
> +#include "sysemu/balloon.h"
> #include "hw/s390x/pv.h"
> #include "migration/blocker.h"
>
> @@ -336,6 +337,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
> ms->pv = false;
> }
> migrate_del_blocker(pv_mig_blocker);
> + qemu_balloon_inhibit(false);
> }
>
> static int s390_machine_protect(S390CcwMachineState *ms)
> @@ -344,6 +346,7 @@ static int s390_machine_protect(S390CcwMachineState *ms)
> CPUState *t;
> int rc;
>
> + qemu_balloon_inhibit(true);
> if (!pv_mig_blocker) {
> error_setg(&pv_mig_blocker,
> "protected VMs are currently not migrateable.");
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-26 14:59 ` David Hildenbrand
@ 2020-02-26 15:06 ` Christian Borntraeger
2020-02-26 15:16 ` David Hildenbrand
2020-02-26 15:11 ` Janosch Frank
1 sibling, 1 reply; 62+ messages in thread
From: Christian Borntraeger @ 2020-02-26 15:06 UTC (permalink / raw)
To: David Hildenbrand, Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck
On 26.02.20 15:59, David Hildenbrand wrote:
> On 26.02.20 13:20, Janosch Frank wrote:
>> Ballooning in protected VMs can only be done when the guest shares the
>> pages it gives to the host. Hence, until we have a solution for this
>> in the guest kernel, we inhibit ballooning when switching into
>> protected mode and reverse that once we move out of it.
>
> I don't understand what you mean here, sorry. zapping a page will mean
> that a fresh one will be faulted in when accessed. And AFAIK, that means
> it will be encrypted again when needed.
>
> Is that more like the UV will detect this as an integrity issue and
> crash the VM?
yes, the UV will detect a fresh page as an integrity issue.
Only if the page was defined to be shared by the guest, we would avoid the
integrity check.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-26 15:06 ` Christian Borntraeger
@ 2020-02-26 15:16 ` David Hildenbrand
2020-02-26 15:30 ` Janosch Frank
0 siblings, 1 reply; 62+ messages in thread
From: David Hildenbrand @ 2020-02-26 15:16 UTC (permalink / raw)
To: Christian Borntraeger, Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck
On 26.02.20 16:06, Christian Borntraeger wrote:
>
>
> On 26.02.20 15:59, David Hildenbrand wrote:
>> On 26.02.20 13:20, Janosch Frank wrote:
>>> Ballooning in protected VMs can only be done when the guest shares the
>>> pages it gives to the host. Hence, until we have a solution for this
>>> in the guest kernel, we inhibit ballooning when switching into
>>> protected mode and reverse that once we move out of it.
>>
>> I don't understand what you mean here, sorry. zapping a page will mean
>> that a fresh one will be faulted in when accessed. And AFAIK, that means
>> it will be encrypted again when needed.
>>
>> Is that more like the UV will detect this as an integrity issue and
>> crash the VM?
>
> yes, the UV will detect a fresh page as an integrity issue.
> Only if the page was defined to be shared by the guest, we would avoid the
> integrity check.
>
Please make that clearer in the patch description. With that
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-26 15:16 ` David Hildenbrand
@ 2020-02-26 15:30 ` Janosch Frank
2020-02-26 15:31 ` David Hildenbrand
2020-02-26 18:24 ` Cornelia Huck
0 siblings, 2 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 15:30 UTC (permalink / raw)
To: David Hildenbrand, Christian Borntraeger, qemu-devel; +Cc: qemu-s390x, cohuck
[-- Attachment #1.1: Type: text/plain, Size: 1560 bytes --]
On 2/26/20 4:16 PM, David Hildenbrand wrote:
> On 26.02.20 16:06, Christian Borntraeger wrote:
>>
>>
>> On 26.02.20 15:59, David Hildenbrand wrote:
>>> On 26.02.20 13:20, Janosch Frank wrote:
>>>> Ballooning in protected VMs can only be done when the guest shares the
>>>> pages it gives to the host. Hence, until we have a solution for this
>>>> in the guest kernel, we inhibit ballooning when switching into
>>>> protected mode and reverse that once we move out of it.
>>>
>>> I don't understand what you mean here, sorry. zapping a page will mean
>>> that a fresh one will be faulted in when accessed. And AFAIK, that means
>>> it will be encrypted again when needed.
>>>
>>> Is that more like the UV will detect this as an integrity issue and
>>> crash the VM?
>>
>> yes, the UV will detect a fresh page as an integrity issue.
>> Only if the page was defined to be shared by the guest, we would avoid the
>> integrity check.
>>
>
> Please make that clearer in the patch description. With that
>
> Reviewed-by: David Hildenbrand <david@redhat.com>
>
How about:
s390x: protvirt: Inhibit balloon when switching to protected mode
Ballooning in protected VMs can only be done when the guest shares the
pages it gives to the host. If pages are not shared, the integrity
checks will fail once those pages have been altered and are given back
to the guest.
Hence, until we have a solution for this in the guest kernel, we
inhibit ballooning when switching into protected mode and reverse that
once we move out of it.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-26 15:30 ` Janosch Frank
@ 2020-02-26 15:31 ` David Hildenbrand
2020-02-26 18:24 ` Cornelia Huck
1 sibling, 0 replies; 62+ messages in thread
From: David Hildenbrand @ 2020-02-26 15:31 UTC (permalink / raw)
To: Janosch Frank, Christian Borntraeger, qemu-devel; +Cc: qemu-s390x, cohuck
On 26.02.20 16:30, Janosch Frank wrote:
> On 2/26/20 4:16 PM, David Hildenbrand wrote:
>> On 26.02.20 16:06, Christian Borntraeger wrote:
>>>
>>>
>>> On 26.02.20 15:59, David Hildenbrand wrote:
>>>> On 26.02.20 13:20, Janosch Frank wrote:
>>>>> Ballooning in protected VMs can only be done when the guest shares the
>>>>> pages it gives to the host. Hence, until we have a solution for this
>>>>> in the guest kernel, we inhibit ballooning when switching into
>>>>> protected mode and reverse that once we move out of it.
>>>>
>>>> I don't understand what you mean here, sorry. zapping a page will mean
>>>> that a fresh one will be faulted in when accessed. And AFAIK, that means
>>>> it will be encrypted again when needed.
>>>>
>>>> Is that more like the UV will detect this as an integrity issue and
>>>> crash the VM?
>>>
>>> yes, the UV will detect a fresh page as an integrity issue.
>>> Only if the page was defined to be shared by the guest, we would avoid the
>>> integrity check.
>>>
>>
>> Please make that clearer in the patch description. With that
>>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>
>
> How about:
> s390x: protvirt: Inhibit balloon when switching to protected mode
>
> Ballooning in protected VMs can only be done when the guest shares the
> pages it gives to the host. If pages are not shared, the integrity
> checks will fail once those pages have been altered and are given back
> to the guest.
>
> Hence, until we have a solution for this in the guest kernel, we
> inhibit ballooning when switching into protected mode and reverse that
> once we move out of it.
>
Yep, sounds good!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-26 15:30 ` Janosch Frank
2020-02-26 15:31 ` David Hildenbrand
@ 2020-02-26 18:24 ` Cornelia Huck
2020-03-03 12:42 ` Christian Borntraeger
1 sibling, 1 reply; 62+ messages in thread
From: Cornelia Huck @ 2020-02-26 18:24 UTC (permalink / raw)
To: Janosch Frank
Cc: Christian Borntraeger, qemu-s390x, qemu-devel, David Hildenbrand
[-- Attachment #1: Type: text/plain, Size: 2645 bytes --]
On Wed, 26 Feb 2020 16:30:32 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:
> On 2/26/20 4:16 PM, David Hildenbrand wrote:
> > On 26.02.20 16:06, Christian Borntraeger wrote:
> >>
> >>
> >> On 26.02.20 15:59, David Hildenbrand wrote:
> >>> On 26.02.20 13:20, Janosch Frank wrote:
> >>>> Ballooning in protected VMs can only be done when the guest shares the
> >>>> pages it gives to the host. Hence, until we have a solution for this
> >>>> in the guest kernel, we inhibit ballooning when switching into
> >>>> protected mode and reverse that once we move out of it.
> >>>
> >>> I don't understand what you mean here, sorry. zapping a page will mean
> >>> that a fresh one will be faulted in when accessed. And AFAIK, that means
> >>> it will be encrypted again when needed.
> >>>
> >>> Is that more like the UV will detect this as an integrity issue and
> >>> crash the VM?
> >>
> >> yes, the UV will detect a fresh page as an integrity issue.
> >> Only if the page was defined to be shared by the guest, we would avoid the
> >> integrity check.
> >>
> >
> > Please make that clearer in the patch description. With that
> >
> > Reviewed-by: David Hildenbrand <david@redhat.com>
> >
>
> How about:
> s390x: protvirt: Inhibit balloon when switching to protected mode
>
> Ballooning in protected VMs can only be done when the guest shares the
> pages it gives to the host. If pages are not shared, the integrity
> checks will fail once those pages have been altered and are given back
> to the guest.
This makes sense to me...
>
> Hence, until we have a solution for this in the guest kernel, we
> inhibit ballooning when switching into protected mode and reverse that
> once we move out of it.
...however, I'm scratching my head here.
If we have a future guest that knows how to handle this, how do we
know? We know that the current Linux driver clears
VIRTIO_F_IOMMU_PLATFORM during feature negotiation, and presumably a
guest that knows how to handle this will not do that. But it still
won't work as expected, as we inhibit ballooning...
So, either
- we don't inhibit ballooning now; any guest that clears the (required)
virtio feature bit won't be able to drive the virtio-balloon device
anyway, but a future guest that negotiates the bit would work; or
- we inhibit ballooning now; no guest can therefore use ballooning,
regardless what they are doing or not doing (this includes guests
that negotiate the feature bit, but fail to handle pages properly).
Or is there some other reason why we need to inhibit ballooning for
protected vms?
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-26 18:24 ` Cornelia Huck
@ 2020-03-03 12:42 ` Christian Borntraeger
0 siblings, 0 replies; 62+ messages in thread
From: Christian Borntraeger @ 2020-03-03 12:42 UTC (permalink / raw)
To: Cornelia Huck, Janosch Frank
Cc: Halil Pasic, qemu-s390x, qemu-devel, David Hildenbrand
On 26.02.20 19:24, Cornelia Huck wrote:
> On Wed, 26 Feb 2020 16:30:32 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> On 2/26/20 4:16 PM, David Hildenbrand wrote:
>>> On 26.02.20 16:06, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 26.02.20 15:59, David Hildenbrand wrote:
>>>>> On 26.02.20 13:20, Janosch Frank wrote:
>>>>>> Ballooning in protected VMs can only be done when the guest shares the
>>>>>> pages it gives to the host. Hence, until we have a solution for this
>>>>>> in the guest kernel, we inhibit ballooning when switching into
>>>>>> protected mode and reverse that once we move out of it.
>>>>>
>>>>> I don't understand what you mean here, sorry. zapping a page will mean
>>>>> that a fresh one will be faulted in when accessed. And AFAIK, that means
>>>>> it will be encrypted again when needed.
>>>>>
>>>>> Is that more like the UV will detect this as an integrity issue and
>>>>> crash the VM?
>>>>
>>>> yes, the UV will detect a fresh page as an integrity issue.
>>>> Only if the page was defined to be shared by the guest, we would avoid the
>>>> integrity check.
>>>>
>>>
>>> Please make that clearer in the patch description. With that
>>>
>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>
>>
>> How about:
>> s390x: protvirt: Inhibit balloon when switching to protected mode
>>
>> Ballooning in protected VMs can only be done when the guest shares the
>> pages it gives to the host. If pages are not shared, the integrity
>> checks will fail once those pages have been altered and are given back
>> to the guest.
>
> This makes sense to me...
>
>>
>> Hence, until we have a solution for this in the guest kernel, we
>> inhibit ballooning when switching into protected mode and reverse that
>> once we move out of it.
>
> ...however, I'm scratching my head here.
>
> If we have a future guest that knows how to handle this, how do we
> know? We know that the current Linux driver clears
> VIRTIO_F_IOMMU_PLATFORM during feature negotiation, and presumably a
> guest that knows how to handle this will not do that. But it still
> won't work as expected, as we inhibit ballooning...
>
> So, either
> - we don't inhibit ballooning now; any guest that clears the (required)
> virtio feature bit won't be able to drive the virtio-balloon device
> anyway, but a future guest that negotiates the bit would work; or
> - we inhibit ballooning now; no guest can therefore use ballooning,
> regardless what they are doing or not doing (this includes guests
> that negotiate the feature bit, but fail to handle pages properly).
>
> Or is there some other reason why we need to inhibit ballooning for
> protected vms?
So here is my proposal.
1. we block ballooning now in QEMU (take this patch now)
2. Later Halil will provide a change to virtio that removes the blocker and adds
VIRTIO_F_IOMMU_PLATFORM automatically by QEMU when doing the protvirt switch. This
is ok as the guest balloon driver will reject to work with the IOMMU change
(see
https://lore.kernel.org/qemu-devel/20200227132402.67a38047.pasic@linux.ibm.com/)
3. later we can fix the guest balloon driver to do the right thing for this
case (e.g. do the make shared call)
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-26 14:59 ` David Hildenbrand
2020-02-26 15:06 ` Christian Borntraeger
@ 2020-02-26 15:11 ` Janosch Frank
2020-02-26 15:13 ` Christian Borntraeger
2020-02-27 12:24 ` Halil Pasic
1 sibling, 2 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 15:11 UTC (permalink / raw)
To: David Hildenbrand, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
[-- Attachment #1.1: Type: text/plain, Size: 2192 bytes --]
On 2/26/20 3:59 PM, David Hildenbrand wrote:
> On 26.02.20 13:20, Janosch Frank wrote:
>> Ballooning in protected VMs can only be done when the guest shares the
>> pages it gives to the host. Hence, until we have a solution for this
>> in the guest kernel, we inhibit ballooning when switching into
>> protected mode and reverse that once we move out of it.
>
> I don't understand what you mean here, sorry. zapping a page will mean
> that a fresh one will be faulted in when accessed. And AFAIK, that means
> it will be encrypted again when needed.
Yes, as soon as the host alters non-shared memory we'll run into
integrity issues.
I've been talking to Halil after I sent this out and it looks like we'll
rather try to automatically enable the IOMMU for all devices when
switching into protected mode. He said that if the IOMMU is set the
balloon code will do an early exit on feature negotiation.
>
> Is that more like the UV will detect this as an integrity issue and
> crash the VM?
>
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> hw/s390x/s390-virtio-ccw.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 9983165b05..0f4455d1df 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -41,6 +41,7 @@
>> #include "hw/qdev-properties.h"
>> #include "hw/s390x/tod.h"
>> #include "sysemu/sysemu.h"
>> +#include "sysemu/balloon.h"
>> #include "hw/s390x/pv.h"
>> #include "migration/blocker.h"
>>
>> @@ -336,6 +337,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
>> ms->pv = false;
>> }
>> migrate_del_blocker(pv_mig_blocker);
>> + qemu_balloon_inhibit(false);
>> }
>>
>> static int s390_machine_protect(S390CcwMachineState *ms)
>> @@ -344,6 +346,7 @@ static int s390_machine_protect(S390CcwMachineState *ms)
>> CPUState *t;
>> int rc;
>>
>> + qemu_balloon_inhibit(true);
>> if (!pv_mig_blocker) {
>> error_setg(&pv_mig_blocker,
>> "protected VMs are currently not migrateable.");
>>
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-26 15:11 ` Janosch Frank
@ 2020-02-26 15:13 ` Christian Borntraeger
2020-02-26 15:15 ` David Hildenbrand
2020-02-27 12:24 ` Halil Pasic
1 sibling, 1 reply; 62+ messages in thread
From: Christian Borntraeger @ 2020-02-26 15:13 UTC (permalink / raw)
To: Janosch Frank, David Hildenbrand, qemu-devel; +Cc: qemu-s390x, cohuck
On 26.02.20 16:11, Janosch Frank wrote:
> On 2/26/20 3:59 PM, David Hildenbrand wrote:
>> On 26.02.20 13:20, Janosch Frank wrote:
>>> Ballooning in protected VMs can only be done when the guest shares the
>>> pages it gives to the host. Hence, until we have a solution for this
>>> in the guest kernel, we inhibit ballooning when switching into
>>> protected mode and reverse that once we move out of it.
>>
>> I don't understand what you mean here, sorry. zapping a page will mean
>> that a fresh one will be faulted in when accessed. And AFAIK, that means
>> it will be encrypted again when needed.
>
> Yes, as soon as the host alters non-shared memory we'll run into
> integrity issues.
>
>
> I've been talking to Halil after I sent this out and it looks like we'll
> rather try to automatically enable the IOMMU for all devices when
> switching into protected mode. He said that if the IOMMU is set the
> balloon code will do an early exit on feature negotiation.
I think we should fence the balloon here nevertheless, so the patch in
itself is probably fine.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-26 15:13 ` Christian Borntraeger
@ 2020-02-26 15:15 ` David Hildenbrand
0 siblings, 0 replies; 62+ messages in thread
From: David Hildenbrand @ 2020-02-26 15:15 UTC (permalink / raw)
To: Christian Borntraeger, Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck
On 26.02.20 16:13, Christian Borntraeger wrote:
>
>
> On 26.02.20 16:11, Janosch Frank wrote:
>> On 2/26/20 3:59 PM, David Hildenbrand wrote:
>>> On 26.02.20 13:20, Janosch Frank wrote:
>>>> Ballooning in protected VMs can only be done when the guest shares the
>>>> pages it gives to the host. Hence, until we have a solution for this
>>>> in the guest kernel, we inhibit ballooning when switching into
>>>> protected mode and reverse that once we move out of it.
>>>
>>> I don't understand what you mean here, sorry. zapping a page will mean
>>> that a fresh one will be faulted in when accessed. And AFAIK, that means
>>> it will be encrypted again when needed.
>>
>> Yes, as soon as the host alters non-shared memory we'll run into
>> integrity issues.
>>
>>
>> I've been talking to Halil after I sent this out and it looks like we'll
>> rather try to automatically enable the IOMMU for all devices when
>> switching into protected mode. He said that if the IOMMU is set the
>> balloon code will do an early exit on feature negotiation.
>
> I think we should fence the balloon here nevertheless, so the patch in
> itself is probably fine.
+1, this is a global "don't use ram_block_discard" trigger.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-26 15:11 ` Janosch Frank
2020-02-26 15:13 ` Christian Borntraeger
@ 2020-02-27 12:24 ` Halil Pasic
2020-03-19 13:42 ` Halil Pasic
2020-03-19 13:54 ` David Hildenbrand
1 sibling, 2 replies; 62+ messages in thread
From: Halil Pasic @ 2020-02-27 12:24 UTC (permalink / raw)
To: Janosch Frank
Cc: borntraeger, qemu-s390x, cohuck, qemu-devel, David Hildenbrand
[-- Attachment #1: Type: text/plain, Size: 5678 bytes --]
On Wed, 26 Feb 2020 16:11:03 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:
> On 2/26/20 3:59 PM, David Hildenbrand wrote:
> > On 26.02.20 13:20, Janosch Frank wrote:
> >> Ballooning in protected VMs can only be done when the guest shares the
> >> pages it gives to the host. Hence, until we have a solution for this
> >> in the guest kernel, we inhibit ballooning when switching into
> >> protected mode and reverse that once we move out of it.
> >
> > I don't understand what you mean here, sorry. zapping a page will mean
> > that a fresh one will be faulted in when accessed. And AFAIK, that means
> > it will be encrypted again when needed.
>
> Yes, as soon as the host alters non-shared memory we'll run into
> integrity issues.
>
>
> I've been talking to Halil after I sent this out and it looks like we'll
> rather try to automatically enable the IOMMU for all devices when
> switching into protected mode. He said that if the IOMMU is set the
> balloon code will do an early exit on feature negotiation.
>
I have a discussion starter RFC for you.
--------------------------8<----------------------------------------------
From: Halil Pasic <pasic@linux.ibm.com>
Date: Wed, 26 Feb 2020 16:48:21 +0100
Subject: [RFC PATCH 1/1] virtio-ccw: auto-manage VIRTIO_F_IOMMU_PLATFORM
The virtio specification tells that the device is to present
VIRTIO_F_ACCESS_PLATFORM (a.k.a. VIRTIO_F_IOMMU_PLATFORM) when the
device "can only access certain memory addresses with said access
specified and/or granted by the platform". This is the case for a
protected VM, as the device can access only memory addresses that are in
pages that are currently shared (only the guest can share/unsare its
page).
No VM however starts out as a protected VM, but some VMs may be
converted to protected VMs if the guest decides so.
Making the end user explicitly manage the VIRTIO_F_ACCESS_PLATFORM via
the property iommu_on is a minor disaster. If the correctness of the
paravirtualized virtio devices depends (and thus in a sense the
correctness of the hypervisor), then the hypervisor should have the last
word about whether VIRTIO_F_ACCESS_PLATFORM is to be presented or not.
Let's manage the VIRTIO_F_ACCESS_PLATFORM virtio feature automatically
for virtio-ccw devices, so that we set it before we start the protected
configuration, and unset it when our VM is not protected.
Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
---
NOTES:
* I wanted to have a discussion starter fast, there are multiple open
questions.
* Doing more than one system_resets() is hackish. We
should look into this.
* The user interface implications of this patch are also an ugly can of
worms. Needs to be discussed.
* We should consider keeping the original value if !pv and restoring it
on pv --> !pv, instead of forcing to unset when leaving pv, and actually
not forcing unset when !pv.
* It might make sense to do something like this in virtio core, but I
decided start the discussion with a ccw-only change.
* Maybe we need a machine option that enables this sort of behavior,
especially if we decide not to do the conserving/restoring.
* Lightly tested.
---
hw/s390x/s390-virtio-ccw.c | 4 ++--
hw/s390x/virtio-ccw.c | 13 +++++++++++++
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 0f4455d1df..996124f152 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -337,7 +337,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
ms->pv = false;
}
migrate_del_blocker(pv_mig_blocker);
- qemu_balloon_inhibit(false);
+ subsystem_reset();
}
static int s390_machine_protect(S390CcwMachineState *ms)
@@ -346,7 +346,6 @@ static int s390_machine_protect(S390CcwMachineState *ms)
CPUState *t;
int rc;
- qemu_balloon_inhibit(true);
if (!pv_mig_blocker) {
error_setg(&pv_mig_blocker,
"protected VMs are currently not migrateable.");
@@ -384,6 +383,7 @@ static int s390_machine_protect(S390CcwMachineState *ms)
if (rc) {
goto out_err;
}
+ subsystem_reset();
return rc;
out_err:
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 13f57e7b67..48bb54f68e 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -869,12 +869,24 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
}
}
+static inline void virtio_ccw_pv_enforce_features(VirtIODevice *vdev)
+{
+ S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
+
+ if (ms->pv) {
+ virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
+ } else {
+ virtio_clear_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
+ }
+}
+
static void virtio_ccw_reset(DeviceState *d)
{
VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
+ virtio_ccw_pv_enforce_features(vdev);
virtio_ccw_reset_virtio(dev, vdev);
if (vdc->parent_reset) {
vdc->parent_reset(d);
@@ -1103,6 +1115,7 @@ static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
if (dev->max_rev >= 1) {
virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
}
+ virtio_ccw_pv_enforce_features(vdev);
}
/* This is called by virtio-bus just after the device is plugged. */
base-commit: 8665f2475f5999d4c9f33598f1360f0b0797c489
--
2.17.1
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-27 12:24 ` Halil Pasic
@ 2020-03-19 13:42 ` Halil Pasic
2020-03-19 13:54 ` David Hildenbrand
1 sibling, 0 replies; 62+ messages in thread
From: Halil Pasic @ 2020-03-19 13:42 UTC (permalink / raw)
To: Janosch Frank
Cc: borntraeger, qemu-s390x, cohuck, qemu-devel, David Hildenbrand
[-- Attachment #1: Type: text/plain, Size: 6076 bytes --]
On Thu, 27 Feb 2020 13:24:02 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:
> On Wed, 26 Feb 2020 16:11:03 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
>
> > On 2/26/20 3:59 PM, David Hildenbrand wrote:
> > > On 26.02.20 13:20, Janosch Frank wrote:
> > >> Ballooning in protected VMs can only be done when the guest shares the
> > >> pages it gives to the host. Hence, until we have a solution for this
> > >> in the guest kernel, we inhibit ballooning when switching into
> > >> protected mode and reverse that once we move out of it.
> > >
> > > I don't understand what you mean here, sorry. zapping a page will mean
> > > that a fresh one will be faulted in when accessed. And AFAIK, that means
> > > it will be encrypted again when needed.
> >
> > Yes, as soon as the host alters non-shared memory we'll run into
> > integrity issues.
> >
> >
> > I've been talking to Halil after I sent this out and it looks like we'll
> > rather try to automatically enable the IOMMU for all devices when
> > switching into protected mode. He said that if the IOMMU is set the
> > balloon code will do an early exit on feature negotiation.
> >
>
> I have a discussion starter RFC for you.
>
> --------------------------8<----------------------------------------------
> From: Halil Pasic <pasic@linux.ibm.com>
> Date: Wed, 26 Feb 2020 16:48:21 +0100
> Subject: [RFC PATCH 1/1] virtio-ccw: auto-manage VIRTIO_F_IOMMU_PLATFORM
>
> The virtio specification tells that the device is to present
> VIRTIO_F_ACCESS_PLATFORM (a.k.a. VIRTIO_F_IOMMU_PLATFORM) when the
> device "can only access certain memory addresses with said access
> specified and/or granted by the platform". This is the case for a
> protected VM, as the device can access only memory addresses that are in
> pages that are currently shared (only the guest can share/unsare its
> page).
>
> No VM however starts out as a protected VM, but some VMs may be
> converted to protected VMs if the guest decides so.
>
> Making the end user explicitly manage the VIRTIO_F_ACCESS_PLATFORM via
> the property iommu_on is a minor disaster. If the correctness of the
> paravirtualized virtio devices depends (and thus in a sense the
> correctness of the hypervisor), then the hypervisor should have the last
> word about whether VIRTIO_F_ACCESS_PLATFORM is to be presented or not.
>
> Let's manage the VIRTIO_F_ACCESS_PLATFORM virtio feature automatically
> for virtio-ccw devices, so that we set it before we start the protected
> configuration, and unset it when our VM is not protected.
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
> NOTES:
> * I wanted to have a discussion starter fast, there are multiple open
> questions.
>
> * Doing more than one system_resets() is hackish. We
> should look into this.
>
> * The user interface implications of this patch are also an ugly can of
> worms. Needs to be discussed.
>
> * We should consider keeping the original value if !pv and restoring it
> on pv --> !pv, instead of forcing to unset when leaving pv, and actually
> not forcing unset when !pv.
>
> * It might make sense to do something like this in virtio core, but I
> decided start the discussion with a ccw-only change.
>
> * Maybe we need a machine option that enables this sort of behavior,
> especially if we decide not to do the conserving/restoring.
>
> * Lightly tested.
ping
Any interest in this?
> ---
> hw/s390x/s390-virtio-ccw.c | 4 ++--
> hw/s390x/virtio-ccw.c | 13 +++++++++++++
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 0f4455d1df..996124f152 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -337,7 +337,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
> ms->pv = false;
> }
> migrate_del_blocker(pv_mig_blocker);
> - qemu_balloon_inhibit(false);
> + subsystem_reset();
> }
>
> static int s390_machine_protect(S390CcwMachineState *ms)
> @@ -346,7 +346,6 @@ static int s390_machine_protect(S390CcwMachineState *ms)
> CPUState *t;
> int rc;
>
> - qemu_balloon_inhibit(true);
> if (!pv_mig_blocker) {
> error_setg(&pv_mig_blocker,
> "protected VMs are currently not migrateable.");
> @@ -384,6 +383,7 @@ static int s390_machine_protect(S390CcwMachineState *ms)
> if (rc) {
> goto out_err;
> }
> + subsystem_reset();
> return rc;
>
> out_err:
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 13f57e7b67..48bb54f68e 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -869,12 +869,24 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
> }
> }
>
> +static inline void virtio_ccw_pv_enforce_features(VirtIODevice *vdev)
> +{
> + S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
> +
> + if (ms->pv) {
> + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> + } else {
> + virtio_clear_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> + }
> +}
> +
> static void virtio_ccw_reset(DeviceState *d)
> {
> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
>
> + virtio_ccw_pv_enforce_features(vdev);
> virtio_ccw_reset_virtio(dev, vdev);
> if (vdc->parent_reset) {
> vdc->parent_reset(d);
> @@ -1103,6 +1115,7 @@ static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> if (dev->max_rev >= 1) {
> virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> }
> + virtio_ccw_pv_enforce_features(vdev);
> }
>
> /* This is called by virtio-bus just after the device is plugged. */
>
> base-commit: 8665f2475f5999d4c9f33598f1360f0b0797c489
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-02-27 12:24 ` Halil Pasic
2020-03-19 13:42 ` Halil Pasic
@ 2020-03-19 13:54 ` David Hildenbrand
2020-03-19 15:40 ` Halil Pasic
2020-03-19 17:45 ` Michael S. Tsirkin
1 sibling, 2 replies; 62+ messages in thread
From: David Hildenbrand @ 2020-03-19 13:54 UTC (permalink / raw)
To: Halil Pasic, Janosch Frank
Cc: borntraeger, qemu-s390x, cohuck, qemu-devel, Michael S. Tsirkin
On 27.02.20 13:24, Halil Pasic wrote:
> On Wed, 26 Feb 2020 16:11:03 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> On 2/26/20 3:59 PM, David Hildenbrand wrote:
>>> On 26.02.20 13:20, Janosch Frank wrote:
>>>> Ballooning in protected VMs can only be done when the guest shares the
>>>> pages it gives to the host. Hence, until we have a solution for this
>>>> in the guest kernel, we inhibit ballooning when switching into
>>>> protected mode and reverse that once we move out of it.
>>>
>>> I don't understand what you mean here, sorry. zapping a page will mean
>>> that a fresh one will be faulted in when accessed. And AFAIK, that means
>>> it will be encrypted again when needed.
>>
>> Yes, as soon as the host alters non-shared memory we'll run into
>> integrity issues.
>>
>>
>> I've been talking to Halil after I sent this out and it looks like we'll
>> rather try to automatically enable the IOMMU for all devices when
>> switching into protected mode. He said that if the IOMMU is set the
>> balloon code will do an early exit on feature negotiation.
>>
>
> I have a discussion starter RFC for you.
>
> --------------------------8<----------------------------------------------
> From: Halil Pasic <pasic@linux.ibm.com>
> Date: Wed, 26 Feb 2020 16:48:21 +0100
> Subject: [RFC PATCH 1/1] virtio-ccw: auto-manage VIRTIO_F_IOMMU_PLATFORM
>
> The virtio specification tells that the device is to present
> VIRTIO_F_ACCESS_PLATFORM (a.k.a. VIRTIO_F_IOMMU_PLATFORM) when the
> device "can only access certain memory addresses with said access
> specified and/or granted by the platform". This is the case for a
> protected VM, as the device can access only memory addresses that are in
> pages that are currently shared (only the guest can share/unsare its
> page).
>
> No VM however starts out as a protected VM, but some VMs may be
> converted to protected VMs if the guest decides so.
>
> Making the end user explicitly manage the VIRTIO_F_ACCESS_PLATFORM via
> the property iommu_on is a minor disaster. If the correctness of the
> paravirtualized virtio devices depends (and thus in a sense the
> correctness of the hypervisor), then the hypervisor should have the last
> word about whether VIRTIO_F_ACCESS_PLATFORM is to be presented or not.
>
> Let's manage the VIRTIO_F_ACCESS_PLATFORM virtio feature automatically
> for virtio-ccw devices, so that we set it before we start the protected
> configuration, and unset it when our VM is not protected.
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
> NOTES:
> * I wanted to have a discussion starter fast, there are multiple open
> questions.
>
> * Doing more than one system_resets() is hackish. We
> should look into this.
>
> * The user interface implications of this patch are also an ugly can of
> worms. Needs to be discussed.
>
> * We should consider keeping the original value if !pv and restoring it
> on pv --> !pv, instead of forcing to unset when leaving pv, and actually
> not forcing unset when !pv.
>
> * It might make sense to do something like this in virtio core, but I
> decided start the discussion with a ccw-only change.
>
> * Maybe we need a machine option that enables this sort of behavior,
> especially if we decide not to do the conserving/restoring.
>
> * Lightly tested.
> ---
> hw/s390x/s390-virtio-ccw.c | 4 ++--
> hw/s390x/virtio-ccw.c | 13 +++++++++++++
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 0f4455d1df..996124f152 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -337,7 +337,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
> ms->pv = false;
> }
> migrate_del_blocker(pv_mig_blocker);
> - qemu_balloon_inhibit(false);
> + subsystem_reset();
> }
>
> static int s390_machine_protect(S390CcwMachineState *ms)
> @@ -346,7 +346,6 @@ static int s390_machine_protect(S390CcwMachineState *ms)
> CPUState *t;
> int rc;
>
> - qemu_balloon_inhibit(true);
> if (!pv_mig_blocker) {
> error_setg(&pv_mig_blocker,
> "protected VMs are currently not migrateable.");
> @@ -384,6 +383,7 @@ static int s390_machine_protect(S390CcwMachineState *ms)
> if (rc) {
> goto out_err;
> }
> + subsystem_reset();
> return rc;
>
> out_err:
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 13f57e7b67..48bb54f68e 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -869,12 +869,24 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
> }
> }
>
> +static inline void virtio_ccw_pv_enforce_features(VirtIODevice *vdev)
> +{
> + S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
> +
> + if (ms->pv) {
> + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> + } else {
> + virtio_clear_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> + }
> +}
> +
> static void virtio_ccw_reset(DeviceState *d)
> {
> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
>
> + virtio_ccw_pv_enforce_features(vdev);
> virtio_ccw_reset_virtio(dev, vdev);
> if (vdc->parent_reset) {
> vdc->parent_reset(d);
> @@ -1103,6 +1115,7 @@ static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> if (dev->max_rev >= 1) {
> virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> }
> + virtio_ccw_pv_enforce_features(vdev);
> }
>
> /* This is called by virtio-bus just after the device is plugged. */
>
> base-commit: 8665f2475f5999d4c9f33598f1360f0b0797c489
>
I asked this question already to Michael (cc) via a different channel,
but hare is it again:
Why does the balloon driver not support VIRTIO_F_IOMMU_PLATFORM? It is
absolutely not clear to me. The introducing commit mentioned that it
"bypasses DMA". I fail to see that.
At least the communication via the SG mechanism should work perfectly
fine with an IOMMU enabled. So I assume it boils down to the pages that
we inflate/deflate not being referenced via IOVA?
I don't think they have to be IOVA addresses. We're neither reading nor
writing these pages. We really speak about "physical memory in the
system" when ballooning. Everything else doesn't really make sense.
There is no need to map/unmap pages we inflate/deflate AFAIKs.
IMHO, we should not try to piggy-back on VIRTIO_F_IOMMU_PLATFORM here,
but instead explicitly disable it either in the hypervisor or the guest.
I hope someone can clarify what the real issue with an IOMMU and
ballooning is, because I'll be having the same "issue" with virtio-mem.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-03-19 13:54 ` David Hildenbrand
@ 2020-03-19 15:40 ` Halil Pasic
2020-03-19 17:31 ` David Hildenbrand
2020-03-19 17:45 ` Michael S. Tsirkin
1 sibling, 1 reply; 62+ messages in thread
From: Halil Pasic @ 2020-03-19 15:40 UTC (permalink / raw)
To: David Hildenbrand
Cc: Janosch Frank, Michael S. Tsirkin, cohuck, qemu-devel,
borntraeger, qemu-s390x
On Thu, 19 Mar 2020 14:54:11 +0100
David Hildenbrand <david@redhat.com> wrote:
> On 27.02.20 13:24, Halil Pasic wrote:
> > On Wed, 26 Feb 2020 16:11:03 +0100
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >
> >> On 2/26/20 3:59 PM, David Hildenbrand wrote:
> >>> On 26.02.20 13:20, Janosch Frank wrote:
> >>>> Ballooning in protected VMs can only be done when the guest shares the
> >>>> pages it gives to the host. Hence, until we have a solution for this
> >>>> in the guest kernel, we inhibit ballooning when switching into
> >>>> protected mode and reverse that once we move out of it.
> >>>
> >>> I don't understand what you mean here, sorry. zapping a page will mean
> >>> that a fresh one will be faulted in when accessed. And AFAIK, that means
> >>> it will be encrypted again when needed.
> >>
> >> Yes, as soon as the host alters non-shared memory we'll run into
> >> integrity issues.
> >>
> >>
> >> I've been talking to Halil after I sent this out and it looks like we'll
> >> rather try to automatically enable the IOMMU for all devices when
> >> switching into protected mode. He said that if the IOMMU is set the
> >> balloon code will do an early exit on feature negotiation.
> >>
> >
> > I have a discussion starter RFC for you.
> >
> > --------------------------8<----------------------------------------------
> > From: Halil Pasic <pasic@linux.ibm.com>
> > Date: Wed, 26 Feb 2020 16:48:21 +0100
> > Subject: [RFC PATCH 1/1] virtio-ccw: auto-manage VIRTIO_F_IOMMU_PLATFORM
> >
> > The virtio specification tells that the device is to present
> > VIRTIO_F_ACCESS_PLATFORM (a.k.a. VIRTIO_F_IOMMU_PLATFORM) when the
> > device "can only access certain memory addresses with said access
> > specified and/or granted by the platform". This is the case for a
> > protected VM, as the device can access only memory addresses that are in
> > pages that are currently shared (only the guest can share/unsare its
> > page).
> >
> > No VM however starts out as a protected VM, but some VMs may be
> > converted to protected VMs if the guest decides so.
> >
> > Making the end user explicitly manage the VIRTIO_F_ACCESS_PLATFORM via
> > the property iommu_on is a minor disaster. If the correctness of the
> > paravirtualized virtio devices depends (and thus in a sense the
> > correctness of the hypervisor), then the hypervisor should have the last
> > word about whether VIRTIO_F_ACCESS_PLATFORM is to be presented or not.
> >
> > Let's manage the VIRTIO_F_ACCESS_PLATFORM virtio feature automatically
> > for virtio-ccw devices, so that we set it before we start the protected
> > configuration, and unset it when our VM is not protected.
> >
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > ---
> > NOTES:
> > * I wanted to have a discussion starter fast, there are multiple open
> > questions.
> >
> > * Doing more than one system_resets() is hackish. We
> > should look into this.
> >
> > * The user interface implications of this patch are also an ugly can of
> > worms. Needs to be discussed.
> >
> > * We should consider keeping the original value if !pv and restoring it
> > on pv --> !pv, instead of forcing to unset when leaving pv, and actually
> > not forcing unset when !pv.
> >
> > * It might make sense to do something like this in virtio core, but I
> > decided start the discussion with a ccw-only change.
> >
> > * Maybe we need a machine option that enables this sort of behavior,
> > especially if we decide not to do the conserving/restoring.
> >
> > * Lightly tested.
> > ---
> > hw/s390x/s390-virtio-ccw.c | 4 ++--
> > hw/s390x/virtio-ccw.c | 13 +++++++++++++
> > 2 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index 0f4455d1df..996124f152 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -337,7 +337,7 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
> > ms->pv = false;
> > }
> > migrate_del_blocker(pv_mig_blocker);
> > - qemu_balloon_inhibit(false);
> > + subsystem_reset();
> > }
> >
> > static int s390_machine_protect(S390CcwMachineState *ms)
> > @@ -346,7 +346,6 @@ static int s390_machine_protect(S390CcwMachineState *ms)
> > CPUState *t;
> > int rc;
> >
> > - qemu_balloon_inhibit(true);
> > if (!pv_mig_blocker) {
> > error_setg(&pv_mig_blocker,
> > "protected VMs are currently not migrateable.");
> > @@ -384,6 +383,7 @@ static int s390_machine_protect(S390CcwMachineState *ms)
> > if (rc) {
> > goto out_err;
> > }
> > + subsystem_reset();
> > return rc;
> >
> > out_err:
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index 13f57e7b67..48bb54f68e 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -869,12 +869,24 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
> > }
> > }
> >
> > +static inline void virtio_ccw_pv_enforce_features(VirtIODevice *vdev)
> > +{
> > + S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
> > +
> > + if (ms->pv) {
> > + virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> > + } else {
> > + virtio_clear_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> > + }
> > +}
> > +
> > static void virtio_ccw_reset(DeviceState *d)
> > {
> > VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> > VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
> > VirtIOCCWDeviceClass *vdc = VIRTIO_CCW_DEVICE_GET_CLASS(dev);
> >
> > + virtio_ccw_pv_enforce_features(vdev);
> > virtio_ccw_reset_virtio(dev, vdev);
> > if (vdc->parent_reset) {
> > vdc->parent_reset(d);
> > @@ -1103,6 +1115,7 @@ static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> > if (dev->max_rev >= 1) {
> > virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> > }
> > + virtio_ccw_pv_enforce_features(vdev);
> > }
> >
> > /* This is called by virtio-bus just after the device is plugged. */
> >
> > base-commit: 8665f2475f5999d4c9f33598f1360f0b0797c489
> >
>
> I asked this question already to Michael (cc) via a different channel,
> but hare is it again:
>
> Why does the balloon driver not support VIRTIO_F_IOMMU_PLATFORM? It is
> absolutely not clear to me. The introducing commit mentioned that it
> "bypasses DMA". I fail to see that.
>
> At least the communication via the SG mechanism should work perfectly
> fine with an IOMMU enabled. So I assume it boils down to the pages that
> we inflate/deflate not being referenced via IOVA?
AFAIU the IOVA/GPA stuff is not the problem here. You have said it
yourself, the SG mechanism would work for balloon out of the box, as it
does for the other virtio devices.
But VIRTIO_F_ACCESS_PLATFORM (aka VIRTIO_F_IOMMU_PLATFORM) not presented
means according to Michael that the device has full access to the entire
guest RAM. If VIRTIO_F_ACCESS_PLATFORM is negotiated this may or may not
be the case.
The actual problem is that the pages denoted by the buffer transmitted
via the virtqueue are normally not shared pages. I.e. the hypervisor
can not reuse them (what is the point of balloon inflate). To make this
work, the guest would need to share the pages before saying 'host these
are in my balloon, so you can use them'. This is a piece of logic we
need only if the host/the device does not have full access to the
guest RAM. That is in my opinion why the balloon driver fences
VIRTIO_F_ACCESS_PLATFORM.
Does that make sense?
>
> I don't think they have to be IOVA addresses. We're neither reading nor
> writing these pages. We really speak about "physical memory in the
> system" when ballooning. Everything else doesn't really make sense.
> There is no need to map/unmap pages we inflate/deflate AFAIKs.
>
> IMHO, we should not try to piggy-back on VIRTIO_F_IOMMU_PLATFORM here,
> but instead explicitly disable it either in the hypervisor or the guest.
>
We need a feature bit here. We can say fencing VIRTIO_F_ACCESS_PLATFORM
was a bug, fix that bug, and then invent another 'the guest RAM is
somehow different' feature bit specific to the balloon, and then create
arch hooks in the driver that get active if this feature is negotiated.
I assumed the fact that the balloon driver fences
VIRTIO_F_ACCESS_PLATFORM is not a bug.
> I hope someone can clarify what the real issue with an IOMMU and
> ballooning is, because I'll be having the same "issue" with
virtio-mem.
>
The issue is not with the IOMMU, the issue is with restricted access
to guest RAM. The definition of VIRTIO_F_ACCESS_PLATFORM is such that we
pretty much know what's up when VIRTIO_F_ACCESS_PLATFORM is not
presented, but VIRTIO_F_ACCESS_PLATFORM presented can mean a couple of
things.
Regards,
Halil
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-03-19 15:40 ` Halil Pasic
@ 2020-03-19 17:31 ` David Hildenbrand
2020-03-20 18:43 ` Halil Pasic
0 siblings, 1 reply; 62+ messages in thread
From: David Hildenbrand @ 2020-03-19 17:31 UTC (permalink / raw)
To: Halil Pasic
Cc: Janosch Frank, Michael S. Tsirkin, cohuck, qemu-devel,
borntraeger, qemu-s390x
[...]
>>
>> I asked this question already to Michael (cc) via a different channel,
>> but hare is it again:
>>
>> Why does the balloon driver not support VIRTIO_F_IOMMU_PLATFORM? It is
>> absolutely not clear to me. The introducing commit mentioned that it
>> "bypasses DMA". I fail to see that.
>>
>> At least the communication via the SG mechanism should work perfectly
>> fine with an IOMMU enabled. So I assume it boils down to the pages that
>> we inflate/deflate not being referenced via IOVA?
>
> AFAIU the IOVA/GPA stuff is not the problem here. You have said it
> yourself, the SG mechanism would work for balloon out of the box, as it
> does for the other virtio devices.
>
> But VIRTIO_F_ACCESS_PLATFORM (aka VIRTIO_F_IOMMU_PLATFORM) not presented
> means according to Michael that the device has full access to the entire
> guest RAM. If VIRTIO_F_ACCESS_PLATFORM is negotiated this may or may not
> be the case.
So you say
"The virtio specification tells that the device is to present
VIRTIO_F_ACCESS_PLATFORM (a.k.a. VIRTIO_F_IOMMU_PLATFORM) when the
device "can only access certain memory addresses with said access
specified and/or granted by the platform"."
So, AFAIU, *any* virtio device (hypervisor side) has to present this
flag when PV is enabled. In that regard, your patch makes perfect sense
(although I am not sure it's a good idea to overwrite these feature bits
- maybe they should be activated on the cmdline permanently instead when
PV is to be used? (or enable )).
>
> The actual problem is that the pages denoted by the buffer transmitted
> via the virtqueue are normally not shared pages. I.e. the hypervisor
> can not reuse them (what is the point of balloon inflate). To make this
> work, the guest would need to share the pages before saying 'host these
> are in my balloon, so you can use them'. This is a piece of logic we
What exactly would have to be done in the hypervisor to support it?
Assume we have to trigger sharing/unsharing - this sounds like a very
architecture specific thing? Or is this e.g., doing a map/unmap
operation like mapping/unmapping the SG?
Right now it sounds to me "we have to do $ARCHSPECIFIC when
inflating/deflating in the guest", which feels wrong.
> need only if the host/the device does not have full access to the
> guest RAM. That is in my opinion why the balloon driver fences
> VIRTIO_F_ACCESS_PLATFORM.> Does that make sense?
Yeah, I understood the "device has to set VIRTIO_F_ACCESS_PLATFORM"
part. Struggling with the "what can the guest driver actually do" part.
>
>>
>> I don't think they have to be IOVA addresses. We're neither reading nor
>> writing these pages. We really speak about "physical memory in the
>> system" when ballooning. Everything else doesn't really make sense.
>> There is no need to map/unmap pages we inflate/deflate AFAIKs.
>>
>> IMHO, we should not try to piggy-back on VIRTIO_F_IOMMU_PLATFORM here,
>> but instead explicitly disable it either in the hypervisor or the guest.
>>
>
> We need a feature bit here. We can say fencing VIRTIO_F_ACCESS_PLATFORM
> was a bug, fix that bug, and then invent another 'the guest RAM is
> somehow different' feature bit specific to the balloon, and then create
> arch hooks in the driver that get active if this feature is negotiated.
>
> I assumed the fact that the balloon driver fences
> VIRTIO_F_ACCESS_PLATFORM is not a bug.
>
>> I hope someone can clarify what the real issue with an IOMMU and
>> ballooning is, because I'll be having the same "issue" with
> virtio-mem.
>>
>
> The issue is not with the IOMMU, the issue is with restricted access
> to guest RAM. The definition of VIRTIO_F_ACCESS_PLATFORM is such that we
> pretty much know what's up when VIRTIO_F_ACCESS_PLATFORM is not
> presented, but VIRTIO_F_ACCESS_PLATFORM presented can mean a couple of
> things.
Understood.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-03-19 17:31 ` David Hildenbrand
@ 2020-03-20 18:43 ` Halil Pasic
2020-03-24 21:07 ` Brijesh Singh
2020-03-27 10:50 ` David Hildenbrand
0 siblings, 2 replies; 62+ messages in thread
From: Halil Pasic @ 2020-03-20 18:43 UTC (permalink / raw)
To: David Hildenbrand
Cc: Tom Lendacky, Brijesh Singh, Janosch Frank, Michael S. Tsirkin,
cohuck, qemu-devel, borntraeger, qemu-s390x
On Thu, 19 Mar 2020 18:31:11 +0100
David Hildenbrand <david@redhat.com> wrote:
> [...]
>
> >>
> >> I asked this question already to Michael (cc) via a different
> >> channel, but hare is it again:
> >>
> >> Why does the balloon driver not support VIRTIO_F_IOMMU_PLATFORM? It
> >> is absolutely not clear to me. The introducing commit mentioned
> >> that it "bypasses DMA". I fail to see that.
> >>
> >> At least the communication via the SG mechanism should work
> >> perfectly fine with an IOMMU enabled. So I assume it boils down to
> >> the pages that we inflate/deflate not being referenced via IOVA?
> >
> > AFAIU the IOVA/GPA stuff is not the problem here. You have said it
> > yourself, the SG mechanism would work for balloon out of the box, as
> > it does for the other virtio devices.
> >
> > But VIRTIO_F_ACCESS_PLATFORM (aka VIRTIO_F_IOMMU_PLATFORM) not
> > presented means according to Michael that the device has full access
> > to the entire guest RAM. If VIRTIO_F_ACCESS_PLATFORM is negotiated
> > this may or may not be the case.
>
> So you say
>
> "The virtio specification tells that the device is to present
> VIRTIO_F_ACCESS_PLATFORM (a.k.a. VIRTIO_F_IOMMU_PLATFORM) when the
> device "can only access certain memory addresses with said access
> specified and/or granted by the platform"."
>
> So, AFAIU, *any* virtio device (hypervisor side) has to present this
> flag when PV is enabled.
Yes, and balloon says bye bye when running in PV mode is only a secondary
objective. I've compiled some references:
"To summarize, the necessary conditions for a hack along these lines
(using DMA API without VIRTIO_F_ACCESS_PLATFORM) are that we detect that:
- secure guest mode is enabled - so we know that since we don't share
most memory regular virtio code won't
work, even though the buggy hypervisor didn't set VIRTIO_F_ACCESS_PLATFORM"
(Michael Tsirkin, https://lkml.org/lkml/2020/2/20/1021)
I.e.: PV but !VIRTIO_F_ACCESS_PLATFORM \implies bugy hypervisor
"If VIRTIO_F_ACCESS_PLATFORM is set then things just work. If
VIRTIO_F_ACCESS_PLATFORM is clear device is supposed to have access to
all of memory. You can argue in various ways but it's easier to just
declare a behaviour that violates this a bug."
(Michael Tsirkin, https://lkml.org/lkml/2020/2/21/1626)
This one is about all memory guest, and not just the buffers transfered
via the virtqueue, which surprised me a bit at the beginning. But balloon
actually needs this.
"A device SHOULD offer VIRTIO_F_ACCESS_PLATFORM if its access to memory
is through bus addresses distinct from and translated by the platform to
physical addresses used by the driver, and/or if it can only access
certain memory addresses with said access specified and/or granted by
the platform. A device MAY fail to operate further if
VIRTIO_F_ACCESS_PLATFORM is not accepted. "
(https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-4120002)
> In that regard, your patch makes perfect sense
> (although I am not sure it's a good idea to overwrite these feature
> bits
> - maybe they should be activated on the cmdline permanently instead
> when PV is to be used? (or enable )).
I didn't understand the last part. I believe conserving the user
specified value when not running in PV mode is better than the hard
overwrite I did here. I wanted a discussion starter.
I think the other option (with respect to let QEMU manage this for user,
i.e. what I try to do here) is to fence the conversion if virtio devices
that do not offer VIRTIO_F_ACCESS_PLATFORM are attached; and disallow
hotplug of such devices at some point during the conversion.
I believe that alternative is even uglier.
IMHO we don't want the end user to fiddle with iommu_platform, because
all the 'benefit' he gets from that is possibility to make a mistake.
For example, I got an internal bug report saying virtio is broken with
PV, which boiled down to an overlooked auto generated NIC, which of
course had iommu_platform (VIRTIO_F_ACCESS_PLATFORM) not set.
>
> >
> > The actual problem is that the pages denoted by the buffer
> > transmitted via the virtqueue are normally not shared pages. I.e.
> > the hypervisor can not reuse them (what is the point of balloon
> > inflate). To make this work, the guest would need to share the pages
> > before saying 'host these are in my balloon, so you can use them'.
> > This is a piece of logic we
>
> What exactly would have to be done in the hypervisor to support it?
AFAIK nothing. The guest needs to share the pages, and everything works.
Janosch, can you help me with this one?
>
> Assume we have to trigger sharing/unsharing - this sounds like a very
> architecture specific thing?
It is, but any guest having sovereignty about its memory may need
something similar.
> Or is this e.g., doing a map/unmap
> operation like mapping/unmapping the SG?
No this is something different. We need stronger guarantees than the
streaming portion of the DMA API provides. And what we actually want
is not DMA but something very different.
>
> Right now it sounds to me "we have to do $ARCHSPECIFIC when
> inflating/deflating in the guest", which feels wrong.
>
It is wrong in a sense. Drivers are mostly supposed to be portable. But
balloon is not a run of the mill device. I don't see any other way to
make this work.
> > need only if the host/the device does not have full access to the
> > guest RAM. That is in my opinion why the balloon driver fences
> > VIRTIO_F_ACCESS_PLATFORM.> Does that make sense?
>
> Yeah, I understood the "device has to set VIRTIO_F_ACCESS_PLATFORM"
> part. Struggling with the "what can the guest driver actually do" part.
>
Let me try to reword this. The point of PV is that the guest has
exclusive access to his pages unless the guest decides to share some
of the using a dedicated ultravisor call.
The point of the memballoon is, as far as I understand, to effectively
dynamically manage the guests memory size within given boundaries, and
without requiring memory hotplug. The basic idea is that the pages in
the balloon belong to the host. The host attempting to re-use a
non-shared page of a guest leads to problems. AFAIR the main problem
was that shall we ever want to deflate such a page (make it again
available for guest use) we would need to do an import, and that can
only work if we have the exact same content as when it was exported.
Otherwise integrity check fails as if we had a malicious hypervisor,
that is trying to inject stuff into the guest.
I'm sure Janosch can provide a better explanation.
I really don't see another way, how memory ballooning could work with
something like PV, without the balloon driver relinquishing the guests
ownership of the pages that are going to leave the guest via the balloon.
On that note ccing the AMD SEV people. Balloon is at this point
dysfunctional for them as well. @Tom: Right? If yes what problems need to
be solved so virtio-balloon can work under SEV?
Regards,
Halil
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-03-20 18:43 ` Halil Pasic
@ 2020-03-24 21:07 ` Brijesh Singh
2020-03-27 10:50 ` David Hildenbrand
1 sibling, 0 replies; 62+ messages in thread
From: Brijesh Singh @ 2020-03-24 21:07 UTC (permalink / raw)
To: Halil Pasic, David Hildenbrand
Cc: Tom Lendacky, brijesh.singh, Janosch Frank, Michael S. Tsirkin,
cohuck, qemu-devel, borntraeger, qemu-s390x
On 3/20/20 1:43 PM, Halil Pasic wrote:
> On Thu, 19 Mar 2020 18:31:11 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
>> [...]
>>
>>>> I asked this question already to Michael (cc) via a different
>>>> channel, but hare is it again:
>>>>
>>>> Why does the balloon driver not support VIRTIO_F_IOMMU_PLATFORM? It
>>>> is absolutely not clear to me. The introducing commit mentioned
>>>> that it "bypasses DMA". I fail to see that.
>>>>
>>>> At least the communication via the SG mechanism should work
>>>> perfectly fine with an IOMMU enabled. So I assume it boils down to
>>>> the pages that we inflate/deflate not being referenced via IOVA?
>>> AFAIU the IOVA/GPA stuff is not the problem here. You have said it
>>> yourself, the SG mechanism would work for balloon out of the box, as
>>> it does for the other virtio devices.
>>>
>>> But VIRTIO_F_ACCESS_PLATFORM (aka VIRTIO_F_IOMMU_PLATFORM) not
>>> presented means according to Michael that the device has full access
>>> to the entire guest RAM. If VIRTIO_F_ACCESS_PLATFORM is negotiated
>>> this may or may not be the case.
>> So you say
>>
>> "The virtio specification tells that the device is to present
>> VIRTIO_F_ACCESS_PLATFORM (a.k.a. VIRTIO_F_IOMMU_PLATFORM) when the
>> device "can only access certain memory addresses with said access
>> specified and/or granted by the platform"."
>>
>> So, AFAIU, *any* virtio device (hypervisor side) has to present this
>> flag when PV is enabled.
> Yes, and balloon says bye bye when running in PV mode is only a secondary
> objective. I've compiled some references:
>
> "To summarize, the necessary conditions for a hack along these lines
> (using DMA API without VIRTIO_F_ACCESS_PLATFORM) are that we detect that:
>
> - secure guest mode is enabled - so we know that since we don't share
> most memory regular virtio code won't
> work, even though the buggy hypervisor didn't set VIRTIO_F_ACCESS_PLATFORM"
> (Michael Tsirkin, https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2020%2F2%2F20%2F1021&data=02%7C01%7Cbrijesh.singh%40amd.com%7C52b79b5c9e894dd968c508d7ccfe9479%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203266090844487&sdata=aNS%2FW2nL27mPSl1Xz3iXUY31qtrzmVHYhzVHEILAaQQ%3D&reserved=0)
> I.e.: PV but !VIRTIO_F_ACCESS_PLATFORM \implies bugy hypervisor
>
>
> "If VIRTIO_F_ACCESS_PLATFORM is set then things just work. If
> VIRTIO_F_ACCESS_PLATFORM is clear device is supposed to have access to
> all of memory. You can argue in various ways but it's easier to just
> declare a behaviour that violates this a bug."
> (Michael Tsirkin, https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2020%2F2%2F21%2F1626&data=02%7C01%7Cbrijesh.singh%40amd.com%7C52b79b5c9e894dd968c508d7ccfe9479%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203266090854439&sdata=d3knybBUZ5NL0Lv1C2JS040A3toiCxXVYLkBlzXSrqc%3D&reserved=0)
> This one is about all memory guest, and not just the buffers transfered
> via the virtqueue, which surprised me a bit at the beginning. But balloon
> actually needs this.
>
> "A device SHOULD offer VIRTIO_F_ACCESS_PLATFORM if its access to memory
> is through bus addresses distinct from and translated by the platform to
> physical addresses used by the driver, and/or if it can only access
> certain memory addresses with said access specified and/or granted by
> the platform. A device MAY fail to operate further if
> VIRTIO_F_ACCESS_PLATFORM is not accepted. "
> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.oasis-open.org%2Fvirtio%2Fvirtio%2Fv1.1%2Fcs01%2Fvirtio-v1.1-cs01.html%23x1-4120002&data=02%7C01%7Cbrijesh.singh%40amd.com%7C52b79b5c9e894dd968c508d7ccfe9479%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637203266090854439&sdata=RBx8cBr8I%2FWFChtVFTjBygRiHIXMmsjT8W%2BwLaTNQ24%3D&reserved=0)
>
>
>> In that regard, your patch makes perfect sense
>> (although I am not sure it's a good idea to overwrite these feature
>> bits
>> - maybe they should be activated on the cmdline permanently instead
>> when PV is to be used? (or enable )).
> I didn't understand the last part. I believe conserving the user
> specified value when not running in PV mode is better than the hard
> overwrite I did here. I wanted a discussion starter.
>
> I think the other option (with respect to let QEMU manage this for user,
> i.e. what I try to do here) is to fence the conversion if virtio devices
> that do not offer VIRTIO_F_ACCESS_PLATFORM are attached; and disallow
> hotplug of such devices at some point during the conversion.
>
> I believe that alternative is even uglier.
>
> IMHO we don't want the end user to fiddle with iommu_platform, because
> all the 'benefit' he gets from that is possibility to make a mistake.
> For example, I got an internal bug report saying virtio is broken with
> PV, which boiled down to an overlooked auto generated NIC, which of
> course had iommu_platform (VIRTIO_F_ACCESS_PLATFORM) not set.
>
>>> The actual problem is that the pages denoted by the buffer
>>> transmitted via the virtqueue are normally not shared pages. I.e.
>>> the hypervisor can not reuse them (what is the point of balloon
>>> inflate). To make this work, the guest would need to share the pages
>>> before saying 'host these are in my balloon, so you can use them'.
>>> This is a piece of logic we
>> What exactly would have to be done in the hypervisor to support it?
> AFAIK nothing. The guest needs to share the pages, and everything works.
> Janosch, can you help me with this one?
>
>> Assume we have to trigger sharing/unsharing - this sounds like a very
>> architecture specific thing?
> It is, but any guest having sovereignty about its memory may need
> something similar.
>
>> Or is this e.g., doing a map/unmap
>> operation like mapping/unmapping the SG?
> No this is something different. We need stronger guarantees than the
> streaming portion of the DMA API provides. And what we actually want
> is not DMA but something very different.
>
>> Right now it sounds to me "we have to do $ARCHSPECIFIC when
>> inflating/deflating in the guest", which feels wrong.
>>
> It is wrong in a sense. Drivers are mostly supposed to be portable. But
> balloon is not a run of the mill device. I don't see any other way to
> make this work.
>
>>> need only if the host/the device does not have full access to the
>>> guest RAM. That is in my opinion why the balloon driver fences
>>> VIRTIO_F_ACCESS_PLATFORM.> Does that make sense?
>> Yeah, I understood the "device has to set VIRTIO_F_ACCESS_PLATFORM"
>> part. Struggling with the "what can the guest driver actually do" part.
>>
> Let me try to reword this. The point of PV is that the guest has
> exclusive access to his pages unless the guest decides to share some
> of the using a dedicated ultravisor call.
>
> The point of the memballoon is, as far as I understand, to effectively
> dynamically manage the guests memory size within given boundaries, and
> without requiring memory hotplug. The basic idea is that the pages in
> the balloon belong to the host. The host attempting to re-use a
> non-shared page of a guest leads to problems. AFAIR the main problem
> was that shall we ever want to deflate such a page (make it again
> available for guest use) we would need to do an import, and that can
> only work if we have the exact same content as when it was exported.
> Otherwise integrity check fails as if we had a malicious hypervisor,
> that is trying to inject stuff into the guest.
>
> I'm sure Janosch can provide a better explanation.
>
> I really don't see another way, how memory ballooning could work with
> something like PV, without the balloon driver relinquishing the guests
> ownership of the pages that are going to leave the guest via the balloon.
>
> On that note ccing the AMD SEV people. Balloon is at this point
> dysfunctional for them as well. @Tom: Right? If yes what problems need to
> be solved so virtio-balloon can work under SEV?
Yes, Balloon does not work for SEV as well. I did not investigated in
the great detail on what it will take to make it work. At that time the
main issue was that the balloon virtio device did not had
VIRTIO_F_ACCESS_PLATFORM flag set. So, we were failing to create a
shared virtio ring. The current SEV does not have integrity protection
support so we should be okay to release the buffer from guest and import
the buffer into the guest. We probably need to deal with some cache
flushes because cache may still contain the data with different C-bit
etc. AMD has recently published the SNP architecture, which supports the
integrity protection and it appears that this architecture provides some
instruction which guest can used by the guest to validate and invalidate
the pages as it leaves and enter the guest memory space. Overall, i
think its doable but devil in the detail.
> Regards,
> Halil
>
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-03-20 18:43 ` Halil Pasic
2020-03-24 21:07 ` Brijesh Singh
@ 2020-03-27 10:50 ` David Hildenbrand
1 sibling, 0 replies; 62+ messages in thread
From: David Hildenbrand @ 2020-03-27 10:50 UTC (permalink / raw)
To: Halil Pasic
Cc: Tom Lendacky, Brijesh Singh, Janosch Frank, Michael S. Tsirkin,
cohuck, qemu-devel, borntraeger, qemu-s390x
>> So, AFAIU, *any* virtio device (hypervisor side) has to present this
>> flag when PV is enabled.
>
> Yes, and balloon says bye bye when running in PV mode is only a secondary
> objective. I've compiled some references:
Thanks!
>
> "To summarize, the necessary conditions for a hack along these lines
> (using DMA API without VIRTIO_F_ACCESS_PLATFORM) are that we detect that:
>
> - secure guest mode is enabled - so we know that since we don't share
> most memory regular virtio code won't
> work, even though the buggy hypervisor didn't set VIRTIO_F_ACCESS_PLATFORM"
> (Michael Tsirkin, https://lkml.org/lkml/2020/2/20/1021)
> I.e.: PV but !VIRTIO_F_ACCESS_PLATFORM \implies bugy hypervisor
>
>
> "If VIRTIO_F_ACCESS_PLATFORM is set then things just work. If
> VIRTIO_F_ACCESS_PLATFORM is clear device is supposed to have access to
> all of memory. You can argue in various ways but it's easier to just
> declare a behaviour that violates this a bug."
> (Michael Tsirkin, https://lkml.org/lkml/2020/2/21/1626)
> This one is about all memory guest, and not just the buffers transfered
> via the virtqueue, which surprised me a bit at the beginning. But balloon
> actually needs this.
>
> "A device SHOULD offer VIRTIO_F_ACCESS_PLATFORM if its access to memory
> is through bus addresses distinct from and translated by the platform to
> physical addresses used by the driver, and/or if it can only access
> certain memory addresses with said access specified and/or granted by
> the platform. A device MAY fail to operate further if
> VIRTIO_F_ACCESS_PLATFORM is not accepted. "
> (https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-4120002)
>
>
>> In that regard, your patch makes perfect sense
>> (although I am not sure it's a good idea to overwrite these feature
>> bits
>> - maybe they should be activated on the cmdline permanently instead
>> when PV is to be used? (or enable )).
>
> I didn't understand the last part. I believe conserving the user
> specified value when not running in PV mode is better than the hard
> overwrite I did here. I wanted a discussion starter.
>
> I think the other option (with respect to let QEMU manage this for user,
> i.e. what I try to do here) is to fence the conversion if virtio devices
> that do not offer VIRTIO_F_ACCESS_PLATFORM are attached; and disallow
> hotplug of such devices at some point during the conversion.
>
> I believe that alternative is even uglier.
>
> IMHO we don't want the end user to fiddle with iommu_platform, because
> all the 'benefit' he gets from that is possibility to make a mistake.
> For example, I got an internal bug report saying virtio is broken with
> PV, which boiled down to an overlooked auto generated NIC, which of
> course had iommu_platform (VIRTIO_F_ACCESS_PLATFORM) not set.
>
>>
>>>
>>> The actual problem is that the pages denoted by the buffer
>>> transmitted via the virtqueue are normally not shared pages. I.e.
>>> the hypervisor can not reuse them (what is the point of balloon
>>> inflate). To make this work, the guest would need to share the pages
>>> before saying 'host these are in my balloon, so you can use them'.
>>> This is a piece of logic we
>>
>> What exactly would have to be done in the hypervisor to support it?
>
> AFAIK nothing. The guest needs to share the pages, and everything works.
> Janosch, can you help me with this one?
>
See below, making this work on the hypervisor side would be much cleaner
IMHO, but most probably not possible due to guest integrity.
FWIW, "Free page reporting" will (never) work with PV, where there is
basically no manual "deflation" step anymore.
>>
>> Assume we have to trigger sharing/unsharing - this sounds like a very
>> architecture specific thing?
>
> It is, but any guest having sovereignty about its memory may need
> something similar.
>
>> Or is this e.g., doing a map/unmap
>> operation like mapping/unmapping the SG?
>
> No this is something different. We need stronger guarantees than the
> streaming portion of the DMA API provides. And what we actually want
> is not DMA but something very different.
Right, that's what I was expecting ...
>
>>
>> Right now it sounds to me "we have to do $ARCHSPECIFIC when
>> inflating/deflating in the guest", which feels wrong.
>>
>
> It is wrong in a sense. Drivers are mostly supposed to be portable. But
> balloon is not a run of the mill device. I don't see any other way to
> make this work.
Well, it is mostly architecture independent until now ...
>
>>> need only if the host/the device does not have full access to the
>>> guest RAM. That is in my opinion why the balloon driver fences
>>> VIRTIO_F_ACCESS_PLATFORM.> Does that make sense?
>>
>> Yeah, I understood the "device has to set VIRTIO_F_ACCESS_PLATFORM"
>> part. Struggling with the "what can the guest driver actually do" part.
>>
>
> Let me try to reword this. The point of PV is that the guest has
> exclusive access to his pages unless the guest decides to share some
> of the using a dedicated ultravisor call.
>
> The point of the memballoon is, as far as I understand, to effectively
> dynamically manage the guests memory size within given boundaries, and
> without requiring memory hotplug. The basic idea is that the pages in
> the balloon belong to the host. The host attempting to re-use a
> non-shared page of a guest leads to problems. AFAIR the main problem
> was that shall we ever want to deflate such a page (make it again
> available for guest use) we would need to do an import, and that can
> only work if we have the exact same content as when it was exported.
> Otherwise integrity check fails as if we had a malicious hypervisor,
> that is trying to inject stuff into the guest.
>
> I'm sure Janosch can provide a better explanation.
>
> I really don't see another way, how memory ballooning could work with
> something like PV, without the balloon driver relinquishing the guests
> ownership of the pages that are going to leave the guest via the balloon.>
> On that note ccing the AMD SEV people. Balloon is at this point
> dysfunctional for them as well. @Tom: Right? If yes what problems need to
> be solved so virtio-balloon can work under SEV?
SEV even pins all guest memory, so it's useless and would be even
dangerous to use.
Some thoughts:
1. I would really prefer if there is a way to zero-out+share a page and
zero-out+unshare a page triggered by the hypervisor. Then only the
hypervisor has to do "the right thing" when
inflating/deflating/rebooting etc. I know we can "unshare all" via the
UV - we e.g., have to do that on reboots. But I assume this might mess
with "guest integrity" (being able to zero out random guest pages
technically) and is therefore not possible.
2. Have some other way to communicate "careful, ballooning won't work".
E.g., the guest detecting *itself* that it is running inside a PV
environment and not loading virtio-balloon until it properly
shares/unshares. Again, piggy-backing on IOMMU/VIRTIO_F_ACCESS_PLATFORM
somehow feels wrong.
E.g., once you would support inflation/deflation in virtio-balloon, free
page reporting could not be supported. So it's more than just a single
arch specific inflation/deflation callback.
And virtio-mem [1] will have similar issues once we want to use that on
s390x. But there, an arch-specific share/unshare callback should be
sufficient most probably. Still, there would have to be a way to block
it on s390x PV until implemented. Ideally it will be the same as for
virtio-balloon.
Again, being able to do that in the hypervisor instead of in the guest
would be much cleaner.
[1] https://lkml.kernel.org/r/20200311171422.10484-1-david@redhat.com
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-03-19 13:54 ` David Hildenbrand
2020-03-19 15:40 ` Halil Pasic
@ 2020-03-19 17:45 ` Michael S. Tsirkin
2020-03-20 9:36 ` David Hildenbrand
1 sibling, 1 reply; 62+ messages in thread
From: Michael S. Tsirkin @ 2020-03-19 17:45 UTC (permalink / raw)
To: David Hildenbrand
Cc: Janosch Frank, cohuck, qemu-devel, Halil Pasic, borntraeger, qemu-s390x
On Thu, Mar 19, 2020 at 02:54:11PM +0100, David Hildenbrand wrote:
> Why does the balloon driver not support VIRTIO_F_IOMMU_PLATFORM? It is
> absolutely not clear to me. The introducing commit mentioned that it
> "bypasses DMA". I fail to see that.
Well sure one can put the balloon behind an IOMMU. If will shuffle PFN
lists through a shared page. Problem is, you can't run an untrusted
driver with it since if you do it can corrupt guest memory.
And VIRTIO_F_IOMMU_PLATFORM so far meant that you can run
a userspace driver.
Maybe we need a separate feature bit for this kind of thing where you
assume the driver is trusted? Such a bit - unlike
VIRTIO_F_IOMMU_PLATFORM - would allow legacy guests ...
--
MST
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-03-19 17:45 ` Michael S. Tsirkin
@ 2020-03-20 9:36 ` David Hildenbrand
2020-03-29 14:48 ` Michael S. Tsirkin
0 siblings, 1 reply; 62+ messages in thread
From: David Hildenbrand @ 2020-03-20 9:36 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Janosch Frank, cohuck, qemu-devel, Halil Pasic, borntraeger, qemu-s390x
On 19.03.20 18:45, Michael S. Tsirkin wrote:
> On Thu, Mar 19, 2020 at 02:54:11PM +0100, David Hildenbrand wrote:
>> Why does the balloon driver not support VIRTIO_F_IOMMU_PLATFORM? It is
>> absolutely not clear to me. The introducing commit mentioned that it
>> "bypasses DMA". I fail to see that.
>
> Well sure one can put the balloon behind an IOMMU. If will shuffle PFN
> lists through a shared page. Problem is, you can't run an untrusted
> driver with it since if you do it can corrupt guest memory.
> And VIRTIO_F_IOMMU_PLATFORM so far meant that you can run
> a userspace driver.
Just to clarify: Is it sufficient to clear VIRTIO_F_IOMMU_PLATFORM in
the *guest kernel driver* to prohibit *guest userspace drivers*?
I would have thought we would have to disallow on the hypervisor/device
side. (no expert on user space drivers, especially how they
detect/enable/access virtio devices)
>
> Maybe we need a separate feature bit for this kind of thing where you
> assume the driver is trusted? Such a bit - unlike
> VIRTIO_F_IOMMU_PLATFORM - would allow legacy guests ...
Let's take virtio-mem as an example. You cannot zap memory outside of
the scope of a virtio-mem device. So I assume having a user space driver
would be ok (although most probably of limited use :) )?
Still, for virtio-mem, special s390x handling, similar to virtio-balloon
- (un)sharing of pages - would have to be performed.
So some feature bits to cleanly separate the different limitations would
be great. At least in regard to s390x, I guess we don't have to worry
too much about legacy guests.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-03-20 9:36 ` David Hildenbrand
@ 2020-03-29 14:48 ` Michael S. Tsirkin
2020-03-31 8:59 ` David Hildenbrand
0 siblings, 1 reply; 62+ messages in thread
From: Michael S. Tsirkin @ 2020-03-29 14:48 UTC (permalink / raw)
To: David Hildenbrand
Cc: Janosch Frank, cohuck, qemu-devel, Halil Pasic, borntraeger, qemu-s390x
On Fri, Mar 20, 2020 at 10:36:53AM +0100, David Hildenbrand wrote:
> On 19.03.20 18:45, Michael S. Tsirkin wrote:
> > On Thu, Mar 19, 2020 at 02:54:11PM +0100, David Hildenbrand wrote:
> >> Why does the balloon driver not support VIRTIO_F_IOMMU_PLATFORM? It is
> >> absolutely not clear to me. The introducing commit mentioned that it
> >> "bypasses DMA". I fail to see that.
> >
> > Well sure one can put the balloon behind an IOMMU. If will shuffle PFN
> > lists through a shared page. Problem is, you can't run an untrusted
> > driver with it since if you do it can corrupt guest memory.
> > And VIRTIO_F_IOMMU_PLATFORM so far meant that you can run
> > a userspace driver.
>
> Just to clarify: Is it sufficient to clear VIRTIO_F_IOMMU_PLATFORM in
> the *guest kernel driver* to prohibit *guest userspace drivers*?
No it isn't sufficient.
> I would have thought we would have to disallow on the hypervisor/device
> side. (no expert on user space drivers, especially how they
> detect/enable/access virtio devices)
QEMU does exactly this:
static int virtio_validate_features(VirtIODevice *vdev)
{
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&
!virtio_vdev_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
return -EFAULT;
}
...
}
> >
> > Maybe we need a separate feature bit for this kind of thing where you
> > assume the driver is trusted? Such a bit - unlike
> > VIRTIO_F_IOMMU_PLATFORM - would allow legacy guests ...
>
> Let's take virtio-mem as an example. You cannot zap memory outside of
> the scope of a virtio-mem device. So I assume having a user space driver
> would be ok (although most probably of limited use :) )?
>
> Still, for virtio-mem, special s390x handling, similar to virtio-balloon
> - (un)sharing of pages - would have to be performed.
>
> So some feature bits to cleanly separate the different limitations would
> be great. At least in regard to s390x, I guess we don't have to worry
> too much about legacy guests.
So if you have the cycles to think through and document how balloon
interacts with different access limitations, that would be great!
> --
> Thanks,
>
> David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode
2020-03-29 14:48 ` Michael S. Tsirkin
@ 2020-03-31 8:59 ` David Hildenbrand
0 siblings, 0 replies; 62+ messages in thread
From: David Hildenbrand @ 2020-03-31 8:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Janosch Frank, cohuck, qemu-devel, Halil Pasic, borntraeger, qemu-s390x
>> I would have thought we would have to disallow on the hypervisor/device
>> side. (no expert on user space drivers, especially how they
>> detect/enable/access virtio devices)
>
> QEMU does exactly this:
>
> static int virtio_validate_features(VirtIODevice *vdev)
> {
> VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>
> if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) &&
> !virtio_vdev_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
> return -EFAULT;
> }
> ...
> }
Okay, that makes sense. Thanks!
>
>
>>>
>>> Maybe we need a separate feature bit for this kind of thing where you
>>> assume the driver is trusted? Such a bit - unlike
>>> VIRTIO_F_IOMMU_PLATFORM - would allow legacy guests ...
>>
>> Let's take virtio-mem as an example. You cannot zap memory outside of
>> the scope of a virtio-mem device. So I assume having a user space driver
>> would be ok (although most probably of limited use :) )?
>>
>> Still, for virtio-mem, special s390x handling, similar to virtio-balloon
>> - (un)sharing of pages - would have to be performed.
>>
>> So some feature bits to cleanly separate the different limitations would
>> be great. At least in regard to s390x, I guess we don't have to worry
>> too much about legacy guests.
>
> So if you have the cycles to think through and document how balloon
> interacts with different access limitations, that would be great!
I'll add it to my ever-growing todo list. Would be great if Halil could
help out thinking how to express the semantics so we can handle PV
properly (both, virtio-balloon, but also virtio-mem).
Cheers!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v5 08/18] s390x: protvirt: KVM intercept changes
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (6 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 07/18] s390x: protvirt: Inhibit balloon when switching to protected mode Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 09/18] s390x: Add SIDA memory ops Janosch Frank
` (11 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Secure guests no longer intercept with code 4 for an instruction
interception. Instead they have codes 104 and 108 for secure
instruction interception and secure instruction notification
respectively.
The 104 mirrors the 4 interception.
The 108 is a notification interception to let KVM and QEMU know that
something changed and we need to update tracking information or
perform specific tasks. It's currently taken for the following
instructions:
* stpx (To inform about the changed prefix location)
* sclp (On incorrect SCCB values, so we can inject a IRQ)
* sigp (All but "stop and store status")
* diag308 (Subcodes 0/1)
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
target/s390x/kvm.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 1d6fd6a27b..eec0b92479 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -115,6 +115,8 @@
#define ICPT_CPU_STOP 0x28
#define ICPT_OPEREXC 0x2c
#define ICPT_IO 0x40
+#define ICPT_PV_INSTR 0x68
+#define ICPT_PV_INSTR_NOTIFICATION 0x6c
#define NR_LOCAL_IRQS 32
/*
@@ -1693,6 +1695,8 @@ static int handle_intercept(S390CPU *cpu)
(long)cs->kvm_run->psw_addr);
switch (icpt_code) {
case ICPT_INSTRUCTION:
+ case ICPT_PV_INSTR:
+ case ICPT_PV_INSTR_NOTIFICATION:
r = handle_instruction(cpu, run);
break;
case ICPT_PROGRAM:
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 09/18] s390x: Add SIDA memory ops
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (7 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 08/18] s390x: protvirt: KVM intercept changes Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 10/18] s390x: protvirt: Move STSI data over SIDAD Janosch Frank
` (10 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Protected guests save the instruction control blocks in the SIDA
instead of QEMU/KVM directly accessing the guest's memory.
Let's introduce new functions to access the SIDA.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
| 2 ++
target/s390x/cpu.h | 7 ++++++-
target/s390x/kvm.c | 25 +++++++++++++++++++++++++
target/s390x/kvm_s390x.h | 2 ++
target/s390x/mmu_helper.c | 14 ++++++++++++++
5 files changed, 49 insertions(+), 1 deletion(-)
--git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index e36f761194..c30344ab00 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -483,6 +483,8 @@ struct kvm_s390_mem_op {
/* types for kvm_s390_mem_op->op */
#define KVM_S390_MEMOP_LOGICAL_READ 0
#define KVM_S390_MEMOP_LOGICAL_WRITE 1
+#define KVM_S390_MEMOP_SIDA_READ 2
+#define KVM_S390_MEMOP_SIDA_WRITE 3
/* flags for kvm_s390_mem_op->flags */
#define KVM_S390_MEMOP_F_CHECK_ONLY (1ULL << 0)
#define KVM_S390_MEMOP_F_INJECT_EXCEPTION (1ULL << 1)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index acff5c9a65..c9bbb51214 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -824,7 +824,12 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
#define s390_cpu_virt_mem_check_write(cpu, laddr, ar, len) \
s390_cpu_virt_mem_rw(cpu, laddr, ar, NULL, len, true)
void s390_cpu_virt_mem_handle_exc(S390CPU *cpu, uintptr_t ra);
-
+int s390_cpu_pv_mem_rw(S390CPU *cpu, unsigned int offset, void *hostbuf,
+ int len, bool is_write);
+#define s390_cpu_pv_mem_read(cpu, offset, dest, len) \
+ s390_cpu_pv_mem_rw(cpu, offset, dest, len, false)
+#define s390_cpu_pv_mem_write(cpu, offset, dest, len) \
+ s390_cpu_pv_mem_rw(cpu, offset, dest, len, true)
/* sigp.c */
int s390_cpu_restart(S390CPU *cpu);
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index eec0b92479..cdcd538b4f 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -154,6 +154,7 @@ static int cap_ri;
static int cap_gs;
static int cap_hpage_1m;
static int cap_vcpu_resets;
+static int cap_protected;
static int active_cmma;
@@ -346,6 +347,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
+ cap_protected = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
|| !kvm_check_extension(s, KVM_CAP_S390_COW)) {
@@ -846,6 +848,29 @@ int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf,
return ret;
}
+int kvm_s390_mem_op_pv(S390CPU *cpu, uint64_t offset, void *hostbuf,
+ int len, bool is_write)
+{
+ struct kvm_s390_mem_op mem_op = {
+ .sida_offset = offset,
+ .size = len,
+ .op = is_write ? KVM_S390_MEMOP_SIDA_WRITE
+ : KVM_S390_MEMOP_SIDA_READ,
+ .buf = (uint64_t)hostbuf,
+ };
+ int ret;
+
+ if (!cap_mem_op || !cap_protected) {
+ return -ENOSYS;
+ }
+
+ ret = kvm_vcpu_ioctl(CPU(cpu), KVM_S390_MEM_OP, &mem_op);
+ if (ret < 0) {
+ error_report("KVM_S390_MEM_OP failed: %s", strerror(-ret));
+ }
+ return ret;
+}
+
/*
* Legacy layout for s390:
* Older S390 KVM requires the topmost vma of the RAM to be
diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
index 0b21789796..9c38f6ccce 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -19,6 +19,8 @@ void kvm_s390_vcpu_interrupt(S390CPU *cpu, struct kvm_s390_irq *irq);
void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code);
int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf,
int len, bool is_write);
+int kvm_s390_mem_op_pv(S390CPU *cpu, vaddr addr, void *hostbuf, int len,
+ bool is_write);
void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t code);
int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state);
void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu);
diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index c9f3f34750..ec8befbdc8 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -474,6 +474,20 @@ static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages,
return 0;
}
+int s390_cpu_pv_mem_rw(S390CPU *cpu, unsigned int offset, void *hostbuf,
+ int len, bool is_write)
+{
+ int ret;
+
+ if (kvm_enabled()) {
+ ret = kvm_s390_mem_op_pv(cpu, offset, hostbuf, len, is_write);
+ } else {
+ /* Protected Virtualization is a KVM/Hardware only feature */
+ g_assert_not_reached();
+ }
+ return ret;
+}
+
/**
* s390_cpu_virt_mem_rw:
* @laddr: the logical start address
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 10/18] s390x: protvirt: Move STSI data over SIDAD
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (8 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 09/18] s390x: Add SIDA memory ops Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 11/18] s390x: protvirt: SCLP interpretation Janosch Frank
` (9 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
For protected guests, we need to put the STSI emulation results into
the SIDA, so SIE will write them into the guest at the next entry.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Acked-by: David Hildenbrand <david@redhat.com>
---
target/s390x/kvm.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index cdcd538b4f..43fc0c088b 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1797,11 +1797,16 @@ static int handle_tsch(S390CPU *cpu)
static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr, uint8_t ar)
{
+ CPUS390XState *env = &cpu->env;
SysIB_322 sysib;
int del;
- if (s390_cpu_virt_mem_read(cpu, addr, ar, &sysib, sizeof(sysib))) {
- return;
+ if (env->pv) {
+ s390_cpu_pv_mem_read(cpu, 0, &sysib, sizeof(sysib));
+ } else {
+ if (s390_cpu_virt_mem_read(cpu, addr, ar, &sysib, sizeof(sysib))) {
+ return;
+ }
}
/* Shift the stack of Extended Names to prepare for our own data */
memmove(&sysib.ext_names[1], &sysib.ext_names[0],
@@ -1840,7 +1845,11 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr, uint8_t ar)
/* Insert UUID */
memcpy(sysib.vm[0].uuid, &qemu_uuid, sizeof(sysib.vm[0].uuid));
- s390_cpu_virt_mem_write(cpu, addr, ar, &sysib, sizeof(sysib));
+ if (env->pv) {
+ s390_cpu_pv_mem_write(cpu, 0, &sysib, sizeof(sysib));
+ } else {
+ s390_cpu_virt_mem_write(cpu, addr, ar, &sysib, sizeof(sysib));
+ }
}
static int handle_stsi(S390CPU *cpu)
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 11/18] s390x: protvirt: SCLP interpretation
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (9 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 10/18] s390x: protvirt: Move STSI data over SIDAD Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 12/18] s390x: protvirt: Set guest IPL PSW Janosch Frank
` (8 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
SCLP for a protected guest is done over the SIDAD, so we need to use
the s390_cpu_virt_mem_* functions to access the SIDAD instead of guest
memory when reading/writing SCBs.
To not confuse the sclp emulation, we set 0x4000 as the SCCB address,
since the function that injects the sclp external interrupt would
reject a zero sccb address.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
hw/s390x/sclp.c | 17 +++++++++++++++++
include/hw/s390x/sclp.h | 2 ++
target/s390x/kvm.c | 5 +++++
3 files changed, 24 insertions(+)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index af0bfbc2ec..5136f5fcbe 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -193,6 +193,23 @@ static void sclp_execute(SCLPDevice *sclp, SCCB *sccb, uint32_t code)
}
}
+#define SCLP_PV_DUMMY_ADDR 0x4000
+int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
+ uint32_t code)
+{
+ SCLPDevice *sclp = get_sclp_device();
+ SCLPDeviceClass *sclp_c = SCLP_GET_CLASS(sclp);
+ SCCB work_sccb;
+ hwaddr sccb_len = sizeof(SCCB);
+
+ s390_cpu_pv_mem_read(env_archcpu(env), 0, &work_sccb, sccb_len);
+ sclp_c->execute(sclp, &work_sccb, code);
+ s390_cpu_pv_mem_write(env_archcpu(env), 0, &work_sccb,
+ be16_to_cpu(work_sccb.h.length));
+ sclp_c->service_interrupt(sclp, SCLP_PV_DUMMY_ADDR);
+ return 0;
+}
+
int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code)
{
SCLPDevice *sclp = get_sclp_device();
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index c54413b78c..c0a3faa37d 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -217,5 +217,7 @@ void s390_sclp_init(void);
void sclp_service_interrupt(uint32_t sccb);
void raise_irq_cpu_hotplug(void);
int sclp_service_call(CPUS390XState *env, uint64_t sccb, uint32_t code);
+int sclp_service_call_protected(CPUS390XState *env, uint64_t sccb,
+ uint32_t code);
#endif
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 43fc0c088b..a4cbdc5fc6 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1226,6 +1226,11 @@ static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
sccb = env->regs[ipbh0 & 0xf];
code = env->regs[(ipbh0 & 0xf0) >> 4];
+ if (run->s390_sieic.icptcode == ICPT_PV_INSTR) {
+ sclp_service_call_protected(env, sccb, code);
+ return;
+ }
+
r = sclp_service_call(env, sccb, code);
if (r < 0) {
kvm_s390_program_interrupt(cpu, -r);
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 12/18] s390x: protvirt: Set guest IPL PSW
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (10 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 11/18] s390x: protvirt: SCLP interpretation Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 13/18] s390x: protvirt: Move diag 308 data over SIDAD Janosch Frank
` (7 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Handling of CPU reset and setting of the IPL psw from guest storage at
offset 0 is done by a Ultravisor call. Let's only fetch it if
necessary.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
target/s390x/cpu.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index c4d937e35a..cd53e123f3 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -78,16 +78,21 @@ static bool s390_cpu_has_work(CPUState *cs)
static void s390_cpu_load_normal(CPUState *s)
{
S390CPU *cpu = S390_CPU(s);
- uint64_t spsw = ldq_phys(s->as, 0);
-
- cpu->env.psw.mask = spsw & PSW_MASK_ESA_MASK;
- /*
- * Invert short psw indication, so SIE will report a specification
- * exception if it was not set.
- */
- cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
- cpu->env.psw.addr = spsw & PSW_MASK_ESA_ADDR;
+ CPUS390XState *env = &cpu->env;
+ uint64_t spsw;
+ if (!env->pv) {
+ spsw = ldq_phys(s->as, 0);
+ cpu->env.psw.mask = spsw & PSW_MASK_ESA_MASK;
+ /*
+ * Invert short psw indication, so SIE will report a specification
+ * exception if it was not set.
+ */
+ cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
+ cpu->env.psw.addr = spsw & PSW_MASK_ESA_ADDR;
+ } else {
+ s390_cpu_set_state(S390_CPU_STATE_LOAD, cpu);
+ }
s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
}
#endif
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 13/18] s390x: protvirt: Move diag 308 data over SIDAD
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (11 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 12/18] s390x: protvirt: Set guest IPL PSW Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 14/18] s390x: protvirt: Disable address checks for PV guest IO emulation Janosch Frank
` (6 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
For protected guests the IPIB is written/read to/from the satellite
block, so we need those accesses to go through
s390_cpu_pv_mem_read/write().
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
target/s390x/diag.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index 840335d40a..266ac78b83 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -88,6 +88,7 @@ static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
{
CPUState *cs = env_cpu(env);
+ S390CPU *cpu = S390_CPU(cs);
uint64_t addr = env->regs[r1];
uint64_t subcode = env->regs[r3];
IplParameterBlock *iplb;
@@ -119,13 +120,22 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
return;
}
iplb = g_new0(IplParameterBlock, 1);
- cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
+ if (!env->pv) {
+ cpu_physical_memory_read(addr, iplb, sizeof(iplb->len));
+ } else {
+ s390_cpu_pv_mem_read(cpu, 0, iplb, sizeof(iplb->len));
+ }
+
if (!iplb_valid_len(iplb)) {
env->regs[r1 + 1] = DIAG_308_RC_INVALID;
goto out;
}
- cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
+ if (!env->pv) {
+ cpu_physical_memory_read(addr, iplb, be32_to_cpu(iplb->len));
+ } else {
+ s390_cpu_pv_mem_read(cpu, 0, iplb, be32_to_cpu(iplb->len));
+ }
if (!iplb_valid_ccw(iplb) && !iplb_valid_fcp(iplb) &&
!(iplb_valid_pv(iplb) && !s390_ipl_pv_check_components(iplb))) {
@@ -137,7 +147,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
env->regs[r1 + 1] = DIAG_308_RC_OK;
out:
g_free(iplb);
- return;
+ break;
case DIAG308_STORE:
case DIAG308_PV_STORE:
if (diag308_parm_check(env, r1, addr, ra, true)) {
@@ -148,12 +158,18 @@ out:
} 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 {
+ if (!iplb) {
env->regs[r1 + 1] = DIAG_308_RC_NO_CONF;
+ return;
}
+
+ if (!env->pv) {
+ cpu_physical_memory_write(addr, iplb, be32_to_cpu(iplb->len));
+ } else {
+ s390_cpu_pv_mem_write(cpu, 0, iplb, be32_to_cpu(iplb->len));
+ }
+
+ env->regs[r1 + 1] = DIAG_308_RC_OK;
break;
case DIAG308_PV_START:
iplb = s390_ipl_get_iplb_secure();
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 14/18] s390x: protvirt: Disable address checks for PV guest IO emulation
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (12 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 13/18] s390x: protvirt: Move diag 308 data over SIDAD Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 15/18] s390x: protvirt: Move IO control structures over SIDA Janosch Frank
` (5 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
IO instruction data is routed through SIDAD for protected guests, so
adresses do not need to be checked, as this is kernel memory.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
target/s390x/ioinst.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index c437a1d8c6..e4102430aa 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -17,6 +17,16 @@
#include "trace.h"
#include "hw/s390x/s390-pci-bus.h"
+static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
+ uint8_t *ar)
+{
+ if (env->pv) {
+ *ar = 0;
+ return 0;
+ }
+ return decode_basedisp_s(env, ipb, ar);
+}
+
int ioinst_disassemble_sch_ident(uint32_t value, int *m, int *cssid, int *ssid,
int *schid)
{
@@ -114,7 +124,7 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
CPUS390XState *env = &cpu->env;
uint8_t ar;
- addr = decode_basedisp_s(env, ipb, &ar);
+ addr = get_address_from_regs(env, ipb, &ar);
if (addr & 3) {
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
@@ -171,7 +181,7 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
CPUS390XState *env = &cpu->env;
uint8_t ar;
- addr = decode_basedisp_s(env, ipb, &ar);
+ addr = get_address_from_regs(env, ipb, &ar);
if (addr & 3) {
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
@@ -203,7 +213,7 @@ void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
CPUS390XState *env = &cpu->env;
uint8_t ar;
- addr = decode_basedisp_s(env, ipb, &ar);
+ addr = get_address_from_regs(env, ipb, &ar);
if (addr & 3) {
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
@@ -234,7 +244,7 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb,
CPUS390XState *env = &cpu->env;
uint8_t ar;
- addr = decode_basedisp_s(env, ipb, &ar);
+ addr = get_address_from_regs(env, ipb, &ar);
if (addr & 3) {
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
@@ -303,7 +313,7 @@ int ioinst_handle_tsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
return -EIO;
}
trace_ioinst_sch_id("tsch", cssid, ssid, schid);
- addr = decode_basedisp_s(env, ipb, &ar);
+ addr = get_address_from_regs(env, ipb, &ar);
if (addr & 3) {
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return -EIO;
@@ -601,7 +611,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
{
ChscReq *req;
ChscResp *res;
- uint64_t addr;
+ uint64_t addr = 0;
int reg;
uint16_t len;
uint16_t command;
@@ -610,7 +620,9 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
trace_ioinst("chsc");
reg = (ipb >> 20) & 0x00f;
- addr = env->regs[reg];
+ if (!env->pv) {
+ addr = env->regs[reg];
+ }
/* Page boundary? */
if (addr & 0xfff) {
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 15/18] s390x: protvirt: Move IO control structures over SIDA
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (13 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 14/18] s390x: protvirt: Disable address checks for PV guest IO emulation Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 16/18] s390x: protvirt: Handle SIGP store status correctly Janosch Frank
` (4 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
For protected guests, we need to put the IO emulation results into the
SIDA, so SIE will write them into the guest at the next entry.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
target/s390x/ioinst.c | 87 ++++++++++++++++++++++++++++++-------------
1 file changed, 61 insertions(+), 26 deletions(-)
diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
index e4102430aa..330b04d79a 100644
--- a/target/s390x/ioinst.c
+++ b/target/s390x/ioinst.c
@@ -129,9 +129,13 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
}
- if (s390_cpu_virt_mem_read(cpu, addr, ar, &schib, sizeof(schib))) {
- s390_cpu_virt_mem_handle_exc(cpu, ra);
- return;
+ if (env->pv) {
+ s390_cpu_pv_mem_read(cpu, addr, &schib, sizeof(schib));
+ } else {
+ if (s390_cpu_virt_mem_read(cpu, addr, ar, &schib, sizeof(schib))) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ return;
+ }
}
if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid) ||
!ioinst_schib_valid(&schib)) {
@@ -186,9 +190,13 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
s390_program_interrupt(env, PGM_SPECIFICATION, ra);
return;
}
- if (s390_cpu_virt_mem_read(cpu, addr, ar, &orig_orb, sizeof(orb))) {
- s390_cpu_virt_mem_handle_exc(cpu, ra);
- return;
+ if (env->pv) {
+ s390_cpu_pv_mem_read(cpu, addr, &orig_orb, sizeof(orb));
+ } else {
+ if (s390_cpu_virt_mem_read(cpu, addr, ar, &orig_orb, sizeof(orb))) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ return;
+ }
}
copy_orb_from_guest(&orb, &orig_orb);
if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid) ||
@@ -222,14 +230,19 @@ void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
cc = css_do_stcrw(&crw);
/* 0 - crw stored, 1 - zeroes stored */
- if (s390_cpu_virt_mem_write(cpu, addr, ar, &crw, sizeof(crw)) == 0) {
+ if (env->pv) {
+ s390_cpu_pv_mem_write(cpu, addr, &crw, sizeof(crw));
setcc(cpu, cc);
} else {
- if (cc == 0) {
- /* Write failed: requeue CRW since STCRW is suppressing */
- css_undo_stcrw(&crw);
+ if (s390_cpu_virt_mem_write(cpu, addr, ar, &crw, sizeof(crw)) == 0) {
+ setcc(cpu, cc);
+ } else {
+ if (cc == 0) {
+ /* Write failed: requeue CRW since STCRW is suppressing */
+ css_undo_stcrw(&crw);
+ }
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
}
- s390_cpu_virt_mem_handle_exc(cpu, ra);
}
}
@@ -251,6 +264,9 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb,
}
if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
+ if (env->pv) {
+ return;
+ }
/*
* As operand exceptions have a lower priority than access exceptions,
* we check whether the memory area is writeable (injecting the
@@ -283,14 +299,19 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb,
}
}
if (cc != 3) {
- if (s390_cpu_virt_mem_write(cpu, addr, ar, &schib,
- sizeof(schib)) != 0) {
- s390_cpu_virt_mem_handle_exc(cpu, ra);
- return;
+ if (env->pv) {
+ s390_cpu_pv_mem_write(cpu, addr, &schib, sizeof(schib));
+ } else {
+ if (s390_cpu_virt_mem_write(cpu, addr, ar, &schib,
+ sizeof(schib)) != 0) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ return;
+ }
}
} else {
/* Access exceptions have a higher priority than cc3 */
- if (s390_cpu_virt_mem_check_write(cpu, addr, ar, sizeof(schib)) != 0) {
+ if (!env->pv &&
+ s390_cpu_virt_mem_check_write(cpu, addr, ar, sizeof(schib)) != 0) {
s390_cpu_virt_mem_handle_exc(cpu, ra);
return;
}
@@ -327,15 +348,20 @@ int ioinst_handle_tsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra)
}
/* 0 - status pending, 1 - not status pending, 3 - not operational */
if (cc != 3) {
- if (s390_cpu_virt_mem_write(cpu, addr, ar, &irb, irb_len) != 0) {
- s390_cpu_virt_mem_handle_exc(cpu, ra);
- return -EFAULT;
+ if (env->pv) {
+ s390_cpu_pv_mem_write(cpu, addr, &irb, irb_len);
+ } else {
+ if (s390_cpu_virt_mem_write(cpu, addr, ar, &irb, irb_len) != 0) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ return -EFAULT;
+ }
}
css_do_tsch_update_subch(sch);
} else {
irb_len = sizeof(irb) - sizeof(irb.emw);
/* Access exceptions have a higher priority than cc3 */
- if (s390_cpu_virt_mem_check_write(cpu, addr, ar, irb_len) != 0) {
+ if (!env->pv &&
+ s390_cpu_virt_mem_check_write(cpu, addr, ar, irb_len) != 0) {
s390_cpu_virt_mem_handle_exc(cpu, ra);
return -EFAULT;
}
@@ -633,9 +659,13 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
* present CHSC sub-handlers ... if we ever need more, we should take
* care of req->len here first.
*/
- if (s390_cpu_virt_mem_read(cpu, addr, reg, buf, sizeof(ChscReq))) {
- s390_cpu_virt_mem_handle_exc(cpu, ra);
- return;
+ if (env->pv) {
+ s390_cpu_pv_mem_read(cpu, addr, buf, sizeof(ChscReq));
+ } else {
+ if (s390_cpu_virt_mem_read(cpu, addr, reg, buf, sizeof(ChscReq))) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ return;
+ }
}
req = (ChscReq *)buf;
len = be16_to_cpu(req->len);
@@ -666,11 +696,16 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra)
break;
}
- if (!s390_cpu_virt_mem_write(cpu, addr + len, reg, res,
- be16_to_cpu(res->len))) {
+ if (env->pv) {
+ s390_cpu_pv_mem_write(cpu, addr + len, res, be16_to_cpu(res->len));
setcc(cpu, 0); /* Command execution complete */
} else {
- s390_cpu_virt_mem_handle_exc(cpu, ra);
+ if (!s390_cpu_virt_mem_write(cpu, addr + len, reg, res,
+ be16_to_cpu(res->len))) {
+ setcc(cpu, 0); /* Command execution complete */
+ } else {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ }
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 16/18] s390x: protvirt: Handle SIGP store status correctly
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (14 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 15/18] s390x: protvirt: Move IO control structures over SIDA Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 17/18] s390x: Add unpack facility feature to GA1 Janosch Frank
` (3 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Status storing is not done by QEMU anymore, but is handled by SIE.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
target/s390x/helper.c | 4 ++++
target/s390x/sigp.c | 1 +
2 files changed, 5 insertions(+)
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 6808dfda01..ec3185151f 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -246,6 +246,10 @@ int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
hwaddr len = sizeof(*sa);
int i;
+ if (cpu->env.pv) {
+ return 0;
+ }
+
sa = cpu_physical_memory_map(addr, &len, 1);
if (!sa) {
return -EFAULT;
diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index c604f17710..e1c8071464 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -497,6 +497,7 @@ void do_stop_interrupt(CPUS390XState *env)
if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
}
+ /* Storing will occur on next SIE entry for protected VMs */
if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 17/18] s390x: Add unpack facility feature to GA1
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (15 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 16/18] s390x: protvirt: Handle SIGP store status correctly Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 12:20 ` [PATCH v5 18/18] docs: Add protvirt docs Janosch Frank
` (2 subsequent siblings)
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
From: Christian Borntraeger <borntraeger@de.ibm.com>
The unpack facility is an indication that diagnose 308 subcodes 8-10
are available to the guest. That means, that the guest can put itself
into protected mode.
Once it is in protected mode, the hardware stops any attempt of VM
introspection by the hypervisor.
Some features are currently not supported in protected mode:
* Passthrough devices
* Migration
* Huge page backings
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
target/s390x/gen-features.c | 1 +
target/s390x/kvm.c | 5 +++++
2 files changed, 6 insertions(+)
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 6278845b12..8ddeebc544 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -562,6 +562,7 @@ static uint16_t full_GEN15_GA1[] = {
S390_FEAT_GROUP_MSA_EXT_9,
S390_FEAT_GROUP_MSA_EXT_9_PCKMO,
S390_FEAT_ETOKEN,
+ S390_FEAT_UNPACK,
};
/* Default features (in order of release)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index a4cbdc5fc6..bf807793bc 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2396,6 +2396,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
clear_bit(S390_FEAT_BPB, model->features);
}
+ /* we do have the IPL enhancements */
+ if (cap_protected) {
+ set_bit(S390_FEAT_UNPACK, model->features);
+ }
+
/* We emulate a zPCI bus and AEN, therefore we don't need HW support */
set_bit(S390_FEAT_ZPCI, model->features);
set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* [PATCH v5 18/18] docs: Add protvirt docs
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (16 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 17/18] s390x: Add unpack facility feature to GA1 Janosch Frank
@ 2020-02-26 12:20 ` Janosch Frank
2020-02-26 20:09 ` [PATCH v5 00/18] s390x: Protected Virtualization support Cornelia Huck
2020-03-03 15:50 ` [PATCH] pc-bios: s390x: Save iplb location in lowcore Janosch Frank
19 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-26 12:20 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
Lets add some documentation for the Protected VM functionality.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
docs/system/index.rst | 1 +
docs/system/protvirt.rst | 57 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+)
create mode 100644 docs/system/protvirt.rst
diff --git a/docs/system/index.rst b/docs/system/index.rst
index 1a4b2c82ac..d2dc63b973 100644
--- a/docs/system/index.rst
+++ b/docs/system/index.rst
@@ -16,3 +16,4 @@ Contents:
qemu-block-drivers
vfio-ap
+ protvirt
diff --git a/docs/system/protvirt.rst b/docs/system/protvirt.rst
new file mode 100644
index 0000000000..a1902cc47c
--- /dev/null
+++ b/docs/system/protvirt.rst
@@ -0,0 +1,57 @@
+Protected Virtualization on s390x
+=================================
+
+The memory and most of the register contents of Protected Virtual
+Machines (PVMs) are inaccessible to the hypervisor, effectively
+prohibiting VM introspection when the VM is running. At rest, PVMs are
+encrypted and can only be decrypted by the firmware of specific IBM Z
+machines.
+
+
+Prerequisites
+-------------
+
+To run PVMs, you need to have a machine with the Protected
+Virtualization feature, which is indicated by the Ultravisor Call
+facility (stfle bit 158). This is a KVM only feature, therefore you
+need a KVM which is able to support PVMs and activate the Ultravisor
+initialization by setting `prot_virt=1` on the kernel command line.
+
+If those requirements are met, the capability `KVM_CAP_S390_PROTECTED`
+will indicate that KVM can support PVMs on that LPAR.
+
+
+QEMU Settings
+-------------
+
+To indicate to the VM that it can move into protected mode, the
+`Unpack facility` (stfle bit 161) needs to be part of the cpu model of
+the VM.
+
+All I/O devices need to use the IOMMU.
+Passthrough (vfio) devices are currently not supported.
+
+Host huge page backings are not supported. The guest however can use
+huge pages as indicated by its facilities.
+
+
+Boot Process
+------------
+
+A secure guest image can be both booted from disk and using the QEMU
+command line. Booting from disk is done by the unmodified s390-ccw
+BIOS. I.e., the bootmap is interpreted and a number of components is
+read into memory and control is transferred to one of the components
+(zipl stage3), which does some fixups and then transfers control to
+some program residing in guest memory, which is normally the OS
+kernel. The secure image has another component prepended (stage3a)
+which uses the new diag308 subcodes 8 and 10 to trigger the transition
+into secure mode.
+
+Booting from the command line requires that the file passed
+via -kernel has the same memory layout as would result from the disk
+boot. This memory layout includes the encrypted components (kernel,
+initrd, cmdline), the stage3a loader and metadata. In case this boot
+method is used, the command line options -initrd and -cmdline are
+ineffective. The preparation of secure guest image is done by a
+program (name tbd) of the s390-tools package.
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH v5 00/18] s390x: Protected Virtualization support
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (17 preceding siblings ...)
2020-02-26 12:20 ` [PATCH v5 18/18] docs: Add protvirt docs Janosch Frank
@ 2020-02-26 20:09 ` Cornelia Huck
2020-02-27 8:32 ` Janosch Frank
2020-03-03 15:50 ` [PATCH] pc-bios: s390x: Save iplb location in lowcore Janosch Frank
19 siblings, 1 reply; 62+ messages in thread
From: Cornelia Huck @ 2020-02-26 20:09 UTC (permalink / raw)
To: Janosch Frank; +Cc: borntraeger, qemu-s390x, qemu-devel, david
On Wed, 26 Feb 2020 07:20:20 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:
> Most of the QEMU changes for PV are related to the new IPL type with
> subcodes 8 - 10 and the execution of the necessary Ultravisor calls to
> IPL secure guests. Note that we can only boot into secure mode from
> normal mode, i.e. stfle 161 is not active in secure mode.
>
> The other changes related to data gathering for emulation and
> disabling addressing checks in secure mode, as well as CPU resets.
>
> v5:
> * Added balloon inhibition
> * Moved docs into docs/system
> * Some more enable/disable changes
> * Moved enablement/disablement of pv in separate functions
> * Some review fixes
>
> v4:
> * Sync with KVM changes
> * Review changes
>
> V3:
> * Use dedicated functions to access SIDA
> * Smaller cleanups and segfault fixes
> * Error reporting for Ultravisor calls
> * Inject of RC of diag308 subcode 10 fails
>
> V2:
> * Split out cleanups
> * Internal PV state tracking
> * Review feedback
>
>
> Christian Borntraeger (1):
> s390x: Add unpack facility feature to GA1
>
> Janosch Frank (17):
> s390x: Use constant for ESA PSW address
> Sync pv
> s390x: protvirt: Add diag308 subcodes 8 - 10
> s390x: protvirt: Support unpack facility
> s390x: protvirt: Add migration blocker
> s390x: protvirt: Handle diag 308 subcodes 0,1,3,4
> s390x: protvirt: Inhibit balloon when switching to protected mode
> s390x: protvirt: KVM intercept changes
> s390x: Add SIDA memory ops
> s390x: protvirt: Move STSI data over SIDAD
> s390x: protvirt: SCLP interpretation
> s390x: protvirt: Set guest IPL PSW
> s390x: protvirt: Move diag 308 data over SIDAD
> s390x: protvirt: Disable address checks for PV guest IO emulation
> s390x: protvirt: Move IO control structures over SIDA
> s390x: protvirt: Handle SIGP store status correctly
> docs: Add protvirt docs
>
> docs/system/index.rst | 1 +
> docs/system/protvirt.rst | 57 +++++++++++
> hw/s390x/Makefile.objs | 1 +
> hw/s390x/ipl.c | 82 +++++++++++++++-
> hw/s390x/ipl.h | 33 +++++++
> hw/s390x/pv.c | 106 +++++++++++++++++++++
> hw/s390x/pv.h | 34 +++++++
> hw/s390x/s390-virtio-ccw.c | 143 +++++++++++++++++++++++++++-
> hw/s390x/sclp.c | 17 ++++
> include/hw/s390x/s390-virtio-ccw.h | 1 +
> include/hw/s390x/sclp.h | 2 +
> linux-headers/linux/kvm.h | 45 ++++++++-
> target/s390x/cpu.c | 27 ++++--
> target/s390x/cpu.h | 9 +-
> target/s390x/cpu_features_def.inc.h | 1 +
> target/s390x/diag.c | 61 ++++++++++--
> target/s390x/gen-features.c | 1 +
> target/s390x/helper.c | 4 +
> target/s390x/ioinst.c | 113 +++++++++++++++-------
> target/s390x/kvm.c | 54 ++++++++++-
> target/s390x/kvm_s390x.h | 2 +
> target/s390x/mmu_helper.c | 14 +++
> target/s390x/sigp.c | 1 +
> 23 files changed, 744 insertions(+), 65 deletions(-)
> create mode 100644 docs/system/protvirt.rst
> create mode 100644 hw/s390x/pv.c
> create mode 100644 hw/s390x/pv.h
>
I guess this is on top of my published s390-next branch? (At least I
can apply it there...)
When I try to build the result on x86 with clang, I get
In file included from /home/cohuck/git/qemu/hw/s390x/ipl.c:36:
/home/cohuck/git/qemu/hw/s390x/pv.h:25:5: error: no previous prototype for
function 's390_pv_vm_enable' [-Werror,-Wmissing-prototypes]
int s390_pv_vm_enable(void) { return 0; }
^
/home/cohuck/git/qemu/hw/s390x/pv.h:26:6: error: no previous prototype for
function 's390_pv_vm_disable' [-Werror,-Wmissing-prototypes]
void s390_pv_vm_disable(void) {}
^
/home/cohuck/git/qemu/hw/s390x/pv.h:27:5: error: no previous prototype for
function 's390_pv_set_sec_parms' [-Werror,-Wmissing-prototypes]
int s390_pv_set_sec_parms(uint64_t origin, uint64_t length) { return 0; }
^
/home/cohuck/git/qemu/hw/s390x/pv.h:28:5: error: no previous prototype for
function 's390_pv_unpack' [-Werror,-Wmissing-prototypes]
int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { return 0; }
^
/home/cohuck/git/qemu/hw/s390x/pv.h:29:6: error: no previous prototype for
function 's390_pv_perf_clear_reset' [-Werror,-Wmissing-prototypes]
void s390_pv_perf_clear_reset(void) {}
^
/home/cohuck/git/qemu/hw/s390x/pv.h:30:5: error: no previous prototype for
function 's390_pv_verify' [-Werror,-Wmissing-prototypes]
int s390_pv_verify(void) { return 0; }
^
/home/cohuck/git/qemu/hw/s390x/pv.h:31:6: error: no previous prototype for
function 's390_pv_unshare' [-Werror,-Wmissing-prototypes]
void s390_pv_unshare(void) {}
^
and
/home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:355:9: error: variable 'rc' is
used uninitialized whenever 'if' condition is true
[-Werror,-Wsometimes-uninitialized]
if (local_err) {
^~~~~~~~~
/home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:391:12: note: uninitialized use
occurs here
return rc;
^~
/home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:355:5: note: remove the 'if' if
its condition is always false
if (local_err) {
^~~~~~~~~~~~~~~~
/home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:347:11: note: initialize the
variable 'rc' to silence this warning
int rc;
^
= 0
/home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:396:26: error: incomplete
definition of type 'struct kvm_run'
int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4;
~~~~~~~~~~~^
/home/cohuck/git/qemu/include/hw/core/cpu.h:265:8: note: forward declaration of
'struct kvm_run'
struct kvm_run;
^
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH v5 00/18] s390x: Protected Virtualization support
2020-02-26 20:09 ` [PATCH v5 00/18] s390x: Protected Virtualization support Cornelia Huck
@ 2020-02-27 8:32 ` Janosch Frank
0 siblings, 0 replies; 62+ messages in thread
From: Janosch Frank @ 2020-02-27 8:32 UTC (permalink / raw)
To: Cornelia Huck; +Cc: borntraeger, qemu-s390x, qemu-devel, david
[-- Attachment #1.1: Type: text/plain, Size: 6518 bytes --]
On 2/26/20 9:09 PM, Cornelia Huck wrote:
> On Wed, 26 Feb 2020 07:20:20 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
>
>> Most of the QEMU changes for PV are related to the new IPL type with
>> subcodes 8 - 10 and the execution of the necessary Ultravisor calls to
>> IPL secure guests. Note that we can only boot into secure mode from
>> normal mode, i.e. stfle 161 is not active in secure mode.
>>
>> The other changes related to data gathering for emulation and
>> disabling addressing checks in secure mode, as well as CPU resets.
>>
>> v5:
>> * Added balloon inhibition
>> * Moved docs into docs/system
>> * Some more enable/disable changes
>> * Moved enablement/disablement of pv in separate functions
>> * Some review fixes
>>
>> v4:
>> * Sync with KVM changes
>> * Review changes
>>
>> V3:
>> * Use dedicated functions to access SIDA
>> * Smaller cleanups and segfault fixes
>> * Error reporting for Ultravisor calls
>> * Inject of RC of diag308 subcode 10 fails
>>
>> V2:
>> * Split out cleanups
>> * Internal PV state tracking
>> * Review feedback
>>
>>
>> Christian Borntraeger (1):
>> s390x: Add unpack facility feature to GA1
>>
>> Janosch Frank (17):
>> s390x: Use constant for ESA PSW address
>> Sync pv
>> s390x: protvirt: Add diag308 subcodes 8 - 10
>> s390x: protvirt: Support unpack facility
>> s390x: protvirt: Add migration blocker
>> s390x: protvirt: Handle diag 308 subcodes 0,1,3,4
>> s390x: protvirt: Inhibit balloon when switching to protected mode
>> s390x: protvirt: KVM intercept changes
>> s390x: Add SIDA memory ops
>> s390x: protvirt: Move STSI data over SIDAD
>> s390x: protvirt: SCLP interpretation
>> s390x: protvirt: Set guest IPL PSW
>> s390x: protvirt: Move diag 308 data over SIDAD
>> s390x: protvirt: Disable address checks for PV guest IO emulation
>> s390x: protvirt: Move IO control structures over SIDA
>> s390x: protvirt: Handle SIGP store status correctly
>> docs: Add protvirt docs
>>
>> docs/system/index.rst | 1 +
>> docs/system/protvirt.rst | 57 +++++++++++
>> hw/s390x/Makefile.objs | 1 +
>> hw/s390x/ipl.c | 82 +++++++++++++++-
>> hw/s390x/ipl.h | 33 +++++++
>> hw/s390x/pv.c | 106 +++++++++++++++++++++
>> hw/s390x/pv.h | 34 +++++++
>> hw/s390x/s390-virtio-ccw.c | 143 +++++++++++++++++++++++++++-
>> hw/s390x/sclp.c | 17 ++++
>> include/hw/s390x/s390-virtio-ccw.h | 1 +
>> include/hw/s390x/sclp.h | 2 +
>> linux-headers/linux/kvm.h | 45 ++++++++-
>> target/s390x/cpu.c | 27 ++++--
>> target/s390x/cpu.h | 9 +-
>> target/s390x/cpu_features_def.inc.h | 1 +
>> target/s390x/diag.c | 61 ++++++++++--
>> target/s390x/gen-features.c | 1 +
>> target/s390x/helper.c | 4 +
>> target/s390x/ioinst.c | 113 +++++++++++++++-------
>> target/s390x/kvm.c | 54 ++++++++++-
>> target/s390x/kvm_s390x.h | 2 +
>> target/s390x/mmu_helper.c | 14 +++
>> target/s390x/sigp.c | 1 +
>> 23 files changed, 744 insertions(+), 65 deletions(-)
>> create mode 100644 docs/system/protvirt.rst
>> create mode 100644 hw/s390x/pv.c
>> create mode 100644 hw/s390x/pv.h
>>
>
> I guess this is on top of my published s390-next branch? (At least I
> can apply it there...)
Yes, you can also pick from here:
https://github.com/frankjaa/qemu/commits/protvirt
>
> When I try to build the result on x86 with clang, I get
I forgot to add static inline to the non kvm functions...
The latest github branch state has that fixed
>
> In file included from /home/cohuck/git/qemu/hw/s390x/ipl.c:36:
> /home/cohuck/git/qemu/hw/s390x/pv.h:25:5: error: no previous prototype for
> function 's390_pv_vm_enable' [-Werror,-Wmissing-prototypes]
> int s390_pv_vm_enable(void) { return 0; }
> ^
> /home/cohuck/git/qemu/hw/s390x/pv.h:26:6: error: no previous prototype for
> function 's390_pv_vm_disable' [-Werror,-Wmissing-prototypes]
> void s390_pv_vm_disable(void) {}
> ^
> /home/cohuck/git/qemu/hw/s390x/pv.h:27:5: error: no previous prototype for
> function 's390_pv_set_sec_parms' [-Werror,-Wmissing-prototypes]
> int s390_pv_set_sec_parms(uint64_t origin, uint64_t length) { return 0; }
> ^
> /home/cohuck/git/qemu/hw/s390x/pv.h:28:5: error: no previous prototype for
> function 's390_pv_unpack' [-Werror,-Wmissing-prototypes]
> int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak) { return 0; }
> ^
> /home/cohuck/git/qemu/hw/s390x/pv.h:29:6: error: no previous prototype for
> function 's390_pv_perf_clear_reset' [-Werror,-Wmissing-prototypes]
> void s390_pv_perf_clear_reset(void) {}
> ^
> /home/cohuck/git/qemu/hw/s390x/pv.h:30:5: error: no previous prototype for
> function 's390_pv_verify' [-Werror,-Wmissing-prototypes]
> int s390_pv_verify(void) { return 0; }
> ^
> /home/cohuck/git/qemu/hw/s390x/pv.h:31:6: error: no previous prototype for
> function 's390_pv_unshare' [-Werror,-Wmissing-prototypes]
> void s390_pv_unshare(void) {}
> ^
>
> and
I'll look into it
>
> /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:355:9: error: variable 'rc' is
> used uninitialized whenever 'if' condition is true
> [-Werror,-Wsometimes-uninitialized]
> if (local_err) {
> ^~~~~~~~~
> /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:391:12: note: uninitialized use
> occurs here
> return rc;
> ^~
> /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:355:5: note: remove the 'if' if
> its condition is always false
> if (local_err) {
> ^~~~~~~~~~~~~~~~
> /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:347:11: note: initialize the
> variable 'rc' to silence this warning
> int rc;
> ^
> = 0
> /home/cohuck/git/qemu/hw/s390x/s390-virtio-ccw.c:396:26: error: incomplete
> definition of type 'struct kvm_run'
> int r1 = (cs->kvm_run->s390_sieic.ipa & 0x00f0) >> 4;
> ~~~~~~~~~~~^
> /home/cohuck/git/qemu/include/hw/core/cpu.h:265:8: note: forward declaration of
> 'struct kvm_run'
> struct kvm_run;
> ^
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH] pc-bios: s390x: Save iplb location in lowcore
2020-02-26 12:20 [PATCH v5 00/18] s390x: Protected Virtualization support Janosch Frank
` (18 preceding siblings ...)
2020-02-26 20:09 ` [PATCH v5 00/18] s390x: Protected Virtualization support Cornelia Huck
@ 2020-03-03 15:50 ` Janosch Frank
2020-03-03 16:13 ` David Hildenbrand
19 siblings, 1 reply; 62+ messages in thread
From: Janosch Frank @ 2020-03-03 15:50 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, david
The POP states that for a list directed IPL the IPLB is stored into
memory by the machine loader and its address is stored at offset 0x14
of the lowcore.
ZIPL currently uses the address in offset 0x14 to access the IPLB and
acquire flags about secure boot. If the IPLB address points into
memory which has an unsupported mix of flags set, ZIPL will panic
instead of booting the OS.
As the lowcore can have quite a high entropy for a guest that did drop
out of protected mode (i.e. rebooted) we encountered the ZIPL panic
quite often.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
pc-bios/s390-ccw/jump2ipl.c | 1 +
pc-bios/s390-ccw/main.c | 8 +++++++-
pc-bios/s390-ccw/netmain.c | 1 +
pc-bios/s390-ccw/s390-arch.h | 10 ++++++++--
pc-bios/s390-ccw/s390-ccw.h | 1 +
5 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index da13c43cc0..4eba2510b0 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -35,6 +35,7 @@ void jump_to_IPL_code(uint64_t address)
{
/* store the subsystem information _after_ the bootmap was loaded */
write_subsystem_identification();
+ write_iplb_location();
/* prevent unknown IPL types in the guest */
if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) {
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index a21b386280..4e65b411e1 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -9,6 +9,7 @@
*/
#include "libc.h"
+#include "helper.h"
#include "s390-arch.h"
#include "s390-ccw.h"
#include "cio.h"
@@ -22,7 +23,7 @@ QemuIplParameters qipl;
IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
static bool have_iplb;
static uint16_t cutype;
-LowCore const *lowcore; /* Yes, this *is* a pointer to address 0 */
+LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
#define LOADPARM_PROMPT "PROMPT "
#define LOADPARM_EMPTY " "
@@ -42,6 +43,11 @@ void write_subsystem_identification(void)
*zeroes = 0;
}
+void write_iplb_location(void)
+{
+ lowcore->ptr_iplb = ptr2u32(&iplb);
+}
+
void panic(const char *string)
{
sclp_print(string);
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index f2dcc01e27..309ffa30d9 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -40,6 +40,7 @@
#define DEFAULT_TFTP_RETRIES 20
extern char _start[];
+void write_iplb_location(void) {}
#define KERNEL_ADDR ((void *)0L)
#define KERNEL_MAX_SIZE ((long)_start)
diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
index 504fc7c2f0..5f36361c02 100644
--- a/pc-bios/s390-ccw/s390-arch.h
+++ b/pc-bios/s390-ccw/s390-arch.h
@@ -36,7 +36,13 @@ typedef struct LowCore {
/* prefix area: defined by architecture */
PSWLegacy ipl_psw; /* 0x000 */
uint32_t ccw1[2]; /* 0x008 */
- uint32_t ccw2[2]; /* 0x010 */
+ union {
+ uint32_t ccw2[2]; /* 0x010 */
+ struct {
+ uint32_t reserved10;
+ uint32_t ptr_iplb;
+ };
+ };
uint8_t pad1[0x80 - 0x18]; /* 0x018 */
uint32_t ext_params; /* 0x080 */
uint16_t cpu_addr; /* 0x084 */
@@ -85,7 +91,7 @@ typedef struct LowCore {
PSW io_new_psw; /* 0x1f0 */
} __attribute__((packed, aligned(8192))) LowCore;
-extern LowCore const *lowcore;
+extern LowCore *lowcore;
static inline void set_prefix(uint32_t address)
{
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 11bce7d73c..21f27e7990 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -57,6 +57,7 @@ void consume_io_int(void);
/* main.c */
void panic(const char *string);
void write_subsystem_identification(void);
+void write_iplb_location(void);
extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
unsigned int get_loadparm_index(void);
--
2.20.1
^ permalink raw reply related [flat|nested] 62+ messages in thread* Re: [PATCH] pc-bios: s390x: Save iplb location in lowcore
2020-03-03 15:50 ` [PATCH] pc-bios: s390x: Save iplb location in lowcore Janosch Frank
@ 2020-03-03 16:13 ` David Hildenbrand
2020-03-04 8:59 ` Janosch Frank
0 siblings, 1 reply; 62+ messages in thread
From: David Hildenbrand @ 2020-03-03 16:13 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
On 03.03.20 16:50, Janosch Frank wrote:
> The POP states that for a list directed IPL the IPLB is stored into
> memory by the machine loader and its address is stored at offset 0x14
> of the lowcore.
>
> ZIPL currently uses the address in offset 0x14 to access the IPLB and
> acquire flags about secure boot. If the IPLB address points into
> memory which has an unsupported mix of flags set, ZIPL will panic
> instead of booting the OS.
>
> As the lowcore can have quite a high entropy for a guest that did drop
> out of protected mode (i.e. rebooted) we encountered the ZIPL panic
> quite often.
How did this ever work? Or does this only become relevant with secure boot?
Fixes: ? Or has this always been broken?
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
> pc-bios/s390-ccw/jump2ipl.c | 1 +
> pc-bios/s390-ccw/main.c | 8 +++++++-
> pc-bios/s390-ccw/netmain.c | 1 +
> pc-bios/s390-ccw/s390-arch.h | 10 ++++++++--
> pc-bios/s390-ccw/s390-ccw.h | 1 +
> 5 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index da13c43cc0..4eba2510b0 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -35,6 +35,7 @@ void jump_to_IPL_code(uint64_t address)
> {
> /* store the subsystem information _after_ the bootmap was loaded */
> write_subsystem_identification();
> + write_iplb_location();
>
> /* prevent unknown IPL types in the guest */
> if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) {
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index a21b386280..4e65b411e1 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -9,6 +9,7 @@
> */
>
> #include "libc.h"
> +#include "helper.h"
> #include "s390-arch.h"
> #include "s390-ccw.h"
> #include "cio.h"
> @@ -22,7 +23,7 @@ QemuIplParameters qipl;
> IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
> static bool have_iplb;
> static uint16_t cutype;
> -LowCore const *lowcore; /* Yes, this *is* a pointer to address 0 */
> +LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>
> #define LOADPARM_PROMPT "PROMPT "
> #define LOADPARM_EMPTY " "
> @@ -42,6 +43,11 @@ void write_subsystem_identification(void)
> *zeroes = 0;
> }
>
> +void write_iplb_location(void)
> +{
> + lowcore->ptr_iplb = ptr2u32(&iplb);
> +}
> +
> void panic(const char *string)
> {
> sclp_print(string);
> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
> index f2dcc01e27..309ffa30d9 100644
> --- a/pc-bios/s390-ccw/netmain.c
> +++ b/pc-bios/s390-ccw/netmain.c
> @@ -40,6 +40,7 @@
> #define DEFAULT_TFTP_RETRIES 20
>
> extern char _start[];
> +void write_iplb_location(void) {}
>
> #define KERNEL_ADDR ((void *)0L)
> #define KERNEL_MAX_SIZE ((long)_start)
> diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
> index 504fc7c2f0..5f36361c02 100644
> --- a/pc-bios/s390-ccw/s390-arch.h
> +++ b/pc-bios/s390-ccw/s390-arch.h
> @@ -36,7 +36,13 @@ typedef struct LowCore {
> /* prefix area: defined by architecture */
> PSWLegacy ipl_psw; /* 0x000 */
> uint32_t ccw1[2]; /* 0x008 */
> - uint32_t ccw2[2]; /* 0x010 */
> + union {
> + uint32_t ccw2[2]; /* 0x010 */
> + struct {
> + uint32_t reserved10;
> + uint32_t ptr_iplb;
> + };
> + };
> uint8_t pad1[0x80 - 0x18]; /* 0x018 */
> uint32_t ext_params; /* 0x080 */
> uint16_t cpu_addr; /* 0x084 */
> @@ -85,7 +91,7 @@ typedef struct LowCore {
> PSW io_new_psw; /* 0x1f0 */
> } __attribute__((packed, aligned(8192))) LowCore;
>
> -extern LowCore const *lowcore;
> +extern LowCore *lowcore;
>
> static inline void set_prefix(uint32_t address)
> {
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 11bce7d73c..21f27e7990 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -57,6 +57,7 @@ void consume_io_int(void);
> /* main.c */
> void panic(const char *string);
> void write_subsystem_identification(void);
> +void write_iplb_location(void);
> extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
> unsigned int get_loadparm_index(void);
In general LGTM.
Side note: I wonder if the assert in ptr2u32() should actually be
IPL_assert((uint64_t)ptr <= 0xfffffffful, "ptr2u32: ptr too large");
^
or even better
IPL_assert((uint64_t)ptr != (uint32_t)ptr, "ptr2u32: ptr too large");
Otherwise, sign extension will simply always make this pass.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH] pc-bios: s390x: Save iplb location in lowcore
2020-03-03 16:13 ` David Hildenbrand
@ 2020-03-04 8:59 ` Janosch Frank
2020-03-04 9:05 ` David Hildenbrand
0 siblings, 1 reply; 62+ messages in thread
From: Janosch Frank @ 2020-03-04 8:59 UTC (permalink / raw)
To: David Hildenbrand, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
[-- Attachment #1.1: Type: text/plain, Size: 5738 bytes --]
On 3/3/20 5:13 PM, David Hildenbrand wrote:
> On 03.03.20 16:50, Janosch Frank wrote:
>> The POP states that for a list directed IPL the IPLB is stored into
>> memory by the machine loader and its address is stored at offset 0x14
>> of the lowcore.
>>
>> ZIPL currently uses the address in offset 0x14 to access the IPLB and
>> acquire flags about secure boot. If the IPLB address points into
>> memory which has an unsupported mix of flags set, ZIPL will panic
>> instead of booting the OS.
>>
>> As the lowcore can have quite a high entropy for a guest that did drop
>> out of protected mode (i.e. rebooted) we encountered the ZIPL panic
>> quite often.
>
> How did this ever work? Or does this only become relevant with secure boot?
I'd guess that until secure boot ZIPL never touched this and with it we
never hit the right combination of flags to trigger a ZIPL panic.
This way of getting to the IPLB was used before diag308 was available,
i.e. way before KVM got to IBM Z. It looks like ZIPL only uses it for
secure boot for some reason and hence we never implemented it before.
I'm also in discussion with the ZIPL developers to make this more robust.
>
> Fixes: ? Or has this always been broken?
See above
>
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>> pc-bios/s390-ccw/jump2ipl.c | 1 +
>> pc-bios/s390-ccw/main.c | 8 +++++++-
>> pc-bios/s390-ccw/netmain.c | 1 +
>> pc-bios/s390-ccw/s390-arch.h | 10 ++++++++--
>> pc-bios/s390-ccw/s390-ccw.h | 1 +
>> 5 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>> index da13c43cc0..4eba2510b0 100644
>> --- a/pc-bios/s390-ccw/jump2ipl.c
>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>> @@ -35,6 +35,7 @@ void jump_to_IPL_code(uint64_t address)
>> {
>> /* store the subsystem information _after_ the bootmap was loaded */
>> write_subsystem_identification();
>> + write_iplb_location();
>>
>> /* prevent unknown IPL types in the guest */
>> if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) {
>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>> index a21b386280..4e65b411e1 100644
>> --- a/pc-bios/s390-ccw/main.c
>> +++ b/pc-bios/s390-ccw/main.c
>> @@ -9,6 +9,7 @@
>> */
>>
>> #include "libc.h"
>> +#include "helper.h"
>> #include "s390-arch.h"
>> #include "s390-ccw.h"
>> #include "cio.h"
>> @@ -22,7 +23,7 @@ QemuIplParameters qipl;
>> IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
>> static bool have_iplb;
>> static uint16_t cutype;
>> -LowCore const *lowcore; /* Yes, this *is* a pointer to address 0 */
>> +LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>>
>> #define LOADPARM_PROMPT "PROMPT "
>> #define LOADPARM_EMPTY " "
>> @@ -42,6 +43,11 @@ void write_subsystem_identification(void)
>> *zeroes = 0;
>> }
>>
>> +void write_iplb_location(void)
>> +{
>> + lowcore->ptr_iplb = ptr2u32(&iplb);
>> +}
>> +
>> void panic(const char *string)
>> {
>> sclp_print(string);
>> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
>> index f2dcc01e27..309ffa30d9 100644
>> --- a/pc-bios/s390-ccw/netmain.c
>> +++ b/pc-bios/s390-ccw/netmain.c
>> @@ -40,6 +40,7 @@
>> #define DEFAULT_TFTP_RETRIES 20
>>
>> extern char _start[];
>> +void write_iplb_location(void) {}
>>
>> #define KERNEL_ADDR ((void *)0L)
>> #define KERNEL_MAX_SIZE ((long)_start)
>> diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
>> index 504fc7c2f0..5f36361c02 100644
>> --- a/pc-bios/s390-ccw/s390-arch.h
>> +++ b/pc-bios/s390-ccw/s390-arch.h
>> @@ -36,7 +36,13 @@ typedef struct LowCore {
>> /* prefix area: defined by architecture */
>> PSWLegacy ipl_psw; /* 0x000 */
>> uint32_t ccw1[2]; /* 0x008 */
>> - uint32_t ccw2[2]; /* 0x010 */
>> + union {
>> + uint32_t ccw2[2]; /* 0x010 */
>> + struct {
>> + uint32_t reserved10;
>> + uint32_t ptr_iplb;
>> + };
>> + };
>> uint8_t pad1[0x80 - 0x18]; /* 0x018 */
>> uint32_t ext_params; /* 0x080 */
>> uint16_t cpu_addr; /* 0x084 */
>> @@ -85,7 +91,7 @@ typedef struct LowCore {
>> PSW io_new_psw; /* 0x1f0 */
>> } __attribute__((packed, aligned(8192))) LowCore;
>>
>> -extern LowCore const *lowcore;
>> +extern LowCore *lowcore;
>>
>> static inline void set_prefix(uint32_t address)
>> {
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 11bce7d73c..21f27e7990 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -57,6 +57,7 @@ void consume_io_int(void);
>> /* main.c */
>> void panic(const char *string);
>> void write_subsystem_identification(void);
>> +void write_iplb_location(void);
>> extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>> unsigned int get_loadparm_index(void);
>
> In general LGTM.
>
>
> Side note: I wonder if the assert in ptr2u32() should actually be
>
> IPL_assert((uint64_t)ptr <= 0xfffffffful, "ptr2u32: ptr too large");
> ^
> or even better
>
> IPL_assert((uint64_t)ptr != (uint32_t)ptr, "ptr2u32: ptr too large");
>
> Otherwise, sign extension will simply always make this pass.
>
Do you want to add a patch or shall I add it to my cleanup series?
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 62+ messages in thread* Re: [PATCH] pc-bios: s390x: Save iplb location in lowcore
2020-03-04 8:59 ` Janosch Frank
@ 2020-03-04 9:05 ` David Hildenbrand
0 siblings, 0 replies; 62+ messages in thread
From: David Hildenbrand @ 2020-03-04 9:05 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck
On 04.03.20 09:59, Janosch Frank wrote:
> On 3/3/20 5:13 PM, David Hildenbrand wrote:
>> On 03.03.20 16:50, Janosch Frank wrote:
>>> The POP states that for a list directed IPL the IPLB is stored into
>>> memory by the machine loader and its address is stored at offset 0x14
>>> of the lowcore.
>>>
>>> ZIPL currently uses the address in offset 0x14 to access the IPLB and
>>> acquire flags about secure boot. If the IPLB address points into
>>> memory which has an unsupported mix of flags set, ZIPL will panic
>>> instead of booting the OS.
>>>
>>> As the lowcore can have quite a high entropy for a guest that did drop
>>> out of protected mode (i.e. rebooted) we encountered the ZIPL panic
>>> quite often.
>>
>> How did this ever work? Or does this only become relevant with secure boot?
>
> I'd guess that until secure boot ZIPL never touched this and with it we
> never hit the right combination of flags to trigger a ZIPL panic.
>
> This way of getting to the IPLB was used before diag308 was available,
> i.e. way before KVM got to IBM Z. It looks like ZIPL only uses it for
> secure boot for some reason and hence we never implemented it before.
>
> I'm also in discussion with the ZIPL developers to make this more robust.
>
Thanks for the clarification!
[...]
>
> Do you want to add a patch or shall I add it to my cleanup series?
Would be great if you could add a cleanup.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 62+ messages in thread