On 2/15/23 18:06, Claudio Imbrenda wrote:
On Wed, 1 Feb 2023 08:48:32 +0000
Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:
The lowcore is an important part of any s390 cpu so we need to make
sure it's always available when we virtualize one. For non-PV guests
that would mean ensuring that the lowcore page is read and writable by
the guest.
For PV guests we additionally need to make sure that the page is owned
by the guest as it is only allowed to access them if that's the
case. The code 112 SIE intercept tells us if the lowcore pages aren't
secure anymore.
Let's check if that intercept is reported by SIE if we export the
lowcore pages. Additionally check if that's also the case if the guest
shares the lowcore which will make it readable to the host but
ownership of the page should not change.
Also we check for validities in these conditions:
* Manipulated cpu timer
* Double SIE for same vcpu
* Re-use of VCPU handle from another secure configuration
* ASCE re-use
Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
looks good, see some questions below
---
[...]
+ extern const char SNIPPET_NAME_START(asm, snippet_pv_icpt_vir_timing)[];
+ extern const char SNIPPET_NAME_END(asm, snippet_pv_icpt_vir_timing)[];
+ extern const char SNIPPET_HDR_START(asm, snippet_pv_icpt_vir_timing)[];
+ extern const char SNIPPET_HDR_END(asm, snippet_pv_icpt_vir_timing)[];
+ int size_hdr = SNIPPET_HDR_LEN(asm, snippet_pv_icpt_vir_timing);
+ int size_gbin = SNIPPET_LEN(asm, snippet_pv_icpt_vir_timing);
+
+ report_prefix_push("manipulated cpu time");
+ snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_pv_icpt_vir_timing),
+ SNIPPET_HDR_START(asm, snippet_pv_icpt_vir_timing),
+ size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
+
+ sie(&vm);
+ report(vm.sblk->icptcode == ICPT_PV_NOTIFY && vm.sblk->ipa == 0x8302 &&
+ vm.sblk->ipb == 0x50000000 && vm.save_area.guest.grs[5] == 0x44,
+ "stp done");
+ vm.sblk->cputm -= 0x280de80000 / 2;
so you are subtracting half of the value?
why not vm.sblk->cputm /= 2?
or just set a fixed (very low) magic value?
what should happen if the cpu timer is higher instead of lower?
I'll need to do some digging to find out why I used this specific
procedure. It's been a very long time since I wrote those tests.
[...]
+ snippet_pv_init(&vm, SNIPPET_NAME_START(asm, snippet_loop),
+ SNIPPET_HDR_START(asm, snippet_loop),
+ size_gbin, size_hdr, SNIPPET_UNPACK_OFF);
+
+ sie_expect_validity(&vm);
+ smp_cpu_setup(1, psw);
+ smp_cpu_setup(2, psw);
+ while (vm.sblk->icptcode != ICPT_VALIDITY) { mb(); }
maybe put the mb(); in a separate line
Can do
+ /* Yes I know this is not reliable as one cpu might overwrite it */
the wording in this comment could be improved
How about:
This might not be fully reliable but it should be sufficient for our
current goals.
[...]
+ report_prefix_push("shared");
+ sie(&vm);
+ /* Guest indicates that it has shared the new lowcore */
+ report(vm.sblk->icptcode == ICPT_PV_NOTIFY && vm.sblk->ipa == 0x8302 &&
+ vm.sblk->ipb == 0x50000000 && vm.save_area.guest.grs[5] == 0x44,
+ "intercept values");
+
+ uv_export(vm.sblk->mso + lc_off);
+ uv_export(vm.sblk->mso + lc_off + PAGE_SIZE);
why are you not testing both pages individually here, like you did
above?
Hmm, I don't think there was a reason behind this. I'll add it.
[...]
