| Message ID | 20210913131153.1202354-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | x86: sgx_vepc: implement ioctl to EREMOVE all pages | expand |
On Mon, Sep 13, 2021 at 09:11:51AM -0400, Paolo Bonzini wrote: > Based on discussions from the previous week(end), this series implements > a ioctl that performs EREMOVE on all pages mapped by a /dev/sgx_vepc > file descriptor. Other possibilities, such as closing and reopening > the device, are racy. > > The patches are untested, but I am posting them because they are simple > and so that Yang Zhong can try using them in QEMU. > Paolo, i re-implemented one reset patch in the Qemu side to call this ioctl(), and did some tests on Windows and Linux guest, the Windows/Linux guest reboot work well. So, it is time for me to send this reset patch to Qemu community? or wait for this kernel patchset merged? thanks! Yang > Paolo > > Paolo Bonzini (2): > x86: sgx_vepc: extract sgx_vepc_remove_page > x86: sgx_vepc: implement SGX_IOC_VEPC_REMOVE ioctl > > arch/x86/include/uapi/asm/sgx.h | 2 ++ > arch/x86/kernel/cpu/sgx/virt.c | 48 ++++++++++++++++++++++++++++++--- > 2 files changed, 47 insertions(+), 3 deletions(-) > > -- > 2.27.0
On 14/09/21 09:10, Yang Zhong wrote: > On Mon, Sep 13, 2021 at 09:11:51AM -0400, Paolo Bonzini wrote: >> Based on discussions from the previous week(end), this series implements >> a ioctl that performs EREMOVE on all pages mapped by a /dev/sgx_vepc >> file descriptor. Other possibilities, such as closing and reopening >> the device, are racy. >> >> The patches are untested, but I am posting them because they are simple >> and so that Yang Zhong can try using them in QEMU. >> > > Paolo, i re-implemented one reset patch in the Qemu side to call this ioctl(), > and did some tests on Windows and Linux guest, the Windows/Linux guest reboot > work well. > > So, it is time for me to send this reset patch to Qemu community? or wait for > this kernel patchset merged? thanks! Let's wait for this patch to be accepted first. I'll wait a little more for Jarkko and Dave to comment on this, and include your "Tested-by". I will also add cond_resched() on the final submission. Paolo
On Tue, 2021-09-14 at 12:19 +0200, Paolo Bonzini wrote: > On 14/09/21 09:10, Yang Zhong wrote: > > On Mon, Sep 13, 2021 at 09:11:51AM -0400, Paolo Bonzini wrote: > > > Based on discussions from the previous week(end), this series implements > > > a ioctl that performs EREMOVE on all pages mapped by a /dev/sgx_vepc > > > file descriptor. Other possibilities, such as closing and reopening > > > the device, are racy. > > > > > > The patches are untested, but I am posting them because they are simple > > > and so that Yang Zhong can try using them in QEMU. > > > > > > > Paolo, i re-implemented one reset patch in the Qemu side to call this ioctl(), > > and did some tests on Windows and Linux guest, the Windows/Linux guest reboot > > work well. > > > > So, it is time for me to send this reset patch to Qemu community? or wait for > > this kernel patchset merged? thanks! > > Let's wait for this patch to be accepted first. I'll wait a little more > for Jarkko and Dave to comment on this, and include your "Tested-by". > > I will also add cond_resched() on the final submission. Why these would be conflicting tasks? I.e. why could not QEMU use what is available now and move forward using better mechanism, when they are available? BTW, I do all my SGX testing ATM in QEMU (for some weeks). IMHO, it's already "good enough" for many tasks, even if this fallback case is not perfectly sorted out. /Jarkko
On 14/09/21 18:42, Jarkko Sakkinen wrote: >> Let's wait for this patch to be accepted first. I'll wait a little more >> for Jarkko and Dave to comment on this, and include your "Tested-by". >> >> I will also add cond_resched() on the final submission. > Why these would be conflicting tasks? I.e. why could not QEMU use > what is available now and move forward using better mechanism, when > they are available? The implementation using close/open is quite ugly (destroying and recreating the memory block breaks a few levels of abstractions), so it's not really something I'd like to commit. Paolo
On Tue, 2021-09-14 at 19:07 +0200, Paolo Bonzini wrote: > On 14/09/21 18:42, Jarkko Sakkinen wrote: > > > Let's wait for this patch to be accepted first. I'll wait a little more > > > for Jarkko and Dave to comment on this, and include your "Tested-by". > > > > > > I will also add cond_resched() on the final submission. > > Why these would be conflicting tasks? I.e. why could not QEMU use > > what is available now and move forward using better mechanism, when > > they are available? > > The implementation using close/open is quite ugly (destroying and > recreating the memory block breaks a few levels of abstractions), so > it's not really something I'd like to commit. OK, so the driving reason for SGX_IOC_VEPC_RESET is the complex dance with opening, closing and mmapping() stuff, especially when dealing with multiple sections for one VM? OK, I think I can understand this, given how notorious it might be to get stable in the user space. Please just document this use case some way (if I got it right) to the commit message of the next version, and I think this starts to make much more sense. /Jarkko
On Tue, 2021-09-14 at 20:40 +0300, Jarkko Sakkinen wrote: > On Tue, 2021-09-14 at 19:07 +0200, Paolo Bonzini wrote: > > On 14/09/21 18:42, Jarkko Sakkinen wrote: > > > > Let's wait for this patch to be accepted first. I'll wait a little more > > > > for Jarkko and Dave to comment on this, and include your "Tested-by". > > > > > > > > I will also add cond_resched() on the final submission. > > > Why these would be conflicting tasks? I.e. why could not QEMU use > > > what is available now and move forward using better mechanism, when > > > they are available? > > > > The implementation using close/open is quite ugly (destroying and > > recreating the memory block breaks a few levels of abstractions), so > > it's not really something I'd like to commit. > > OK, so the driving reason for SGX_IOC_VEPC_RESET is the complex dance > with opening, closing and mmapping() stuff, especially when dealing > with multiple sections for one VM? OK, I think I can understand this, > given how notorious it might be to get stable in the user space. > > Please just document this use case some way (if I got it right) to > the commit message of the next version, and I think this starts to > make much more sense. I would call it bottleneck rather than race though. I would keep race for the things that can cause real race condition inside the kernel corrupting data structures or whatever. But for sure it is bottleneck that can easily cause user space to be racy without extra-ordinary carefulness. /Jarkko
On Tue, Sep 14, 2021 at 12:19:31PM +0200, Paolo Bonzini wrote: > On 14/09/21 09:10, Yang Zhong wrote: > >On Mon, Sep 13, 2021 at 09:11:51AM -0400, Paolo Bonzini wrote: > >>Based on discussions from the previous week(end), this series implements > >>a ioctl that performs EREMOVE on all pages mapped by a /dev/sgx_vepc > >>file descriptor. Other possibilities, such as closing and reopening > >>the device, are racy. > >> > >>The patches are untested, but I am posting them because they are simple > >>and so that Yang Zhong can try using them in QEMU. > >> > > > > Paolo, i re-implemented one reset patch in the Qemu side to call this ioctl(), > > and did some tests on Windows and Linux guest, the Windows/Linux guest reboot > > work well. > > > > So, it is time for me to send this reset patch to Qemu community? or wait for > > this kernel patchset merged? thanks! > > Let's wait for this patch to be accepted first. I'll wait a little > more for Jarkko and Dave to comment on this, and include your > "Tested-by". > > I will also add cond_resched() on the final submission. > Thanks Paolo, i will send Qemu patch once this patchset is accepted. This day, i also did corner cases test and updated related Qemu reset patch. do { ret = ioctl(fd, SGX_IOC_VEPC_REMOVE); /* this printf is only for debug*/ printf("-------sgx ret=%d and n=%d---\n", ret, n++); if(ret) sleep(1); } while (ret); (1). The VEPC size=10M, start 4 enclaves(each ~2G size) in the VM side. then do the 'system_reset' in the Qemu monitor tool. (2). The VEPC size=10G, start 500 enclaves(each ~20M size) in the VM side. then do the 'system_reset' in the Qemu monitor tool. The ret will show the failures number(SECS pages number, 4 and 500) got from kernel side, after sleep 1s, the ioctl will return 0 failures. If this reset is triggered by guest bios, there is 0 SECS page got from kernel, which will not block VM booting. So, until now, the kernel patches work well. If any new issue, i will update it to all. thanks! Yang > Paolo