Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1695554 > unrolled thread
| Started by | David Laight <David.Laight@ACULAB.COM> |
|---|---|
| First post | 2017-07-25 12:00 +0200 |
| Last post | 2017-07-27 09:50 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
RE: [RFC Part1 PATCH v3 13/17] x86/io: Unroll string I/O when SEV is active David Laight <David.Laight@ACULAB.COM> - 2017-07-25 12:00 +0200
Re: [RFC Part1 PATCH v3 13/17] x86/io: Unroll string I/O when SEV is active Arnd Bergmann <arnd@arndb.de> - 2017-07-26 12:50 +0200
Re: [RFC Part1 PATCH v3 13/17] x86/io: Unroll string I/O when SEV is active "H. Peter Anvin" <hpa@zytor.com> - 2017-07-26 21:50 +0200
RE: [RFC Part1 PATCH v3 13/17] x86/io: Unroll string I/O when SEV is active David Laight <David.Laight@ACULAB.COM> - 2017-07-27 09:50 +0200
| From | David Laight <David.Laight@ACULAB.COM> |
|---|---|
| Date | 2017-07-25 12:00 +0200 |
| Subject | RE: [RFC Part1 PATCH v3 13/17] x86/io: Unroll string I/O when SEV is active |
| Message-ID | <u74Nb-1aa-3@gated-at.bofh.it> |
From: Brijesh Singh
> Sent: 24 July 2017 20:08
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> Secure Encrypted Virtualization (SEV) does not support string I/O, so
> unroll the string I/O operation into a loop operating on one element at
> a time.
>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
> arch/x86/include/asm/io.h | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index e080a39..2f3c002 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -327,14 +327,32 @@ static inline unsigned type in##bwl##_p(int port) \
> \
> static inline void outs##bwl(int port, const void *addr, unsigned long count) \
> {
Is it even worth leaving these as inline functions?
Given the speed of IO cycles it is unlikely that the cost of calling a real
function will be significant.
The code bloat reduction will be significant.
David
[toc] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-07-26 12:50 +0200 |
| Subject | Re: [RFC Part1 PATCH v3 13/17] x86/io: Unroll string I/O when SEV is active |
| Message-ID | <u7s38-7CE-23@gated-at.bofh.it> |
| In reply to | #1695554 |
On Tue, Jul 25, 2017 at 11:51 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Brijesh Singh
>> Sent: 24 July 2017 20:08
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> Secure Encrypted Virtualization (SEV) does not support string I/O, so
>> unroll the string I/O operation into a loop operating on one element at
>> a time.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> arch/x86/include/asm/io.h | 26 ++++++++++++++++++++++----
>> 1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
>> index e080a39..2f3c002 100644
>> --- a/arch/x86/include/asm/io.h
>> +++ b/arch/x86/include/asm/io.h
>> @@ -327,14 +327,32 @@ static inline unsigned type in##bwl##_p(int port) \
>> \
>> static inline void outs##bwl(int port, const void *addr, unsigned long count) \
>> {
This will clash with a fix I did to add a "memory" clobber
for the traditional implementation, see
https://patchwork.kernel.org/patch/9854573/
> Is it even worth leaving these as inline functions?
> Given the speed of IO cycles it is unlikely that the cost of calling a real
> function will be significant.
> The code bloat reduction will be significant.
I think the smallest code would be the original "rep insb" etc, which
should be smaller than a function call, unlike the loop. Then again,
there is a rather small number of affected device drivers, almost all
of them for ancient hardware that you won't even build in a 64-bit
x86 kernel, see the list below. The only user I found that is actually
still relevant is drivers/tty/hvc/hvc_xen.c, which uses it for the early
console.
Arnd
---
Full list for reference
$ git grep -wl '\(__ide_\|\)\(in\|out\)s\(b\|w\|l\)' drivers sound/
drivers/atm/horizon.c
drivers/cdrom/gdrom.c
drivers/gpu/drm/vmwgfx/vmwgfx_msg.h
drivers/ide/ide-io-std.c
drivers/isdn/hardware/avm/avmcard.h
drivers/isdn/hardware/eicon/divasmain.c
drivers/isdn/hardware/mISDN/avmfritz.c
drivers/isdn/hardware/mISDN/iohelper.h
drivers/isdn/hardware/mISDN/netjet.c
drivers/isdn/hardware/mISDN/w6692.c
drivers/isdn/hisax/asuscom.c
drivers/isdn/hisax/avm_a1.c
drivers/isdn/hisax/avm_a1p.c
drivers/isdn/hisax/avm_pci.c
drivers/isdn/hisax/diva.c
drivers/isdn/hisax/elsa.c
drivers/isdn/hisax/gazel.c
drivers/isdn/hisax/hisax_fcpcipnp.c
drivers/isdn/hisax/ix1_micro.c
drivers/isdn/hisax/mic.c
drivers/isdn/hisax/netjet.c
drivers/isdn/hisax/niccy.c
drivers/isdn/hisax/saphir.c
drivers/isdn/hisax/sedlbauer.c
drivers/isdn/hisax/sportster.c
drivers/isdn/hisax/teles3.c
drivers/isdn/hisax/w6692.c
drivers/isdn/hisax/w6692.h
drivers/media/rc/winbond-cir.c
drivers/mtd/spi-nor/aspeed-smc.c
drivers/net/appletalk/cops.c
drivers/net/arcnet/arcdevice.h
drivers/net/arcnet/com20020.c
drivers/net/ethernet/3com/3c509.c
drivers/net/ethernet/3com/3c515.c
drivers/net/ethernet/3com/3c574_cs.c
drivers/net/ethernet/3com/3c589_cs.c
drivers/net/ethernet/8390/axnet_cs.c
drivers/net/ethernet/8390/mcf8390.c
drivers/net/ethernet/8390/ne.c
drivers/net/ethernet/8390/ne2k-pci.c
drivers/net/ethernet/8390/pcnet_cs.c
drivers/net/ethernet/8390/smc-ultra.c
drivers/net/ethernet/amd/nmclan_cs.c
drivers/net/ethernet/fujitsu/fmvj18x_cs.c
drivers/net/ethernet/hp/hp100.c
drivers/net/ethernet/smsc/smc9194.c
drivers/net/ethernet/smsc/smc91c92_cs.c
drivers/net/ethernet/smsc/smc91x.h
drivers/net/ethernet/xircom/xirc2ps_cs.c
drivers/net/sb1000.c
drivers/net/wan/sbni.c
drivers/net/wireless/cisco/airo.c
drivers/net/wireless/intersil/hostap/hostap_cs.c
drivers/net/wireless/intersil/hostap/hostap_plx.c
drivers/net/wireless/marvell/libertas/if_cs.c
drivers/net/wireless/wl3501_cs.c
drivers/parport/parport_pc.c
drivers/pcmcia/m32r_cfc.c
drivers/scsi/NCR53c406a.c
drivers/scsi/aha152x.c
drivers/scsi/eata_pio.c
drivers/scsi/fdomain.c
drivers/scsi/g_NCR5380.c
drivers/scsi/imm.c
drivers/scsi/nsp32_io.h
drivers/scsi/pcmcia/nsp_io.h
drivers/scsi/pcmcia/sym53c500_cs.c
drivers/scsi/ppa.c
drivers/scsi/qlogicfas408.c
drivers/scsi/sym53c416.c
drivers/staging/comedi/drivers/adl_pci9111.c
drivers/staging/comedi/drivers/cb_pcidas.c
drivers/staging/comedi/drivers/das16m1.c
drivers/staging/comedi/drivers/das1800.c
drivers/staging/iio/adc/ad7606_par.c
drivers/tty/hvc/hvc_xen.c
drivers/tty/isicom.c
drivers/tty/rocket_int.h
drivers/usb/host/isp1362.h
drivers/usb/musb/blackfin.c
sound/drivers/opl4/opl4_lib.c
sound/isa/gus/gus_dram.c
sound/isa/gus/gus_pcm.c
[toc] | [prev] | [next] | [standalone]
| From | "H. Peter Anvin" <hpa@zytor.com> |
|---|---|
| Date | 2017-07-26 21:50 +0200 |
| Subject | Re: [RFC Part1 PATCH v3 13/17] x86/io: Unroll string I/O when SEV is active |
| Message-ID | <u7AtI-4y2-17@gated-at.bofh.it> |
| In reply to | #1696979 |
<ard.biesheuvel@linaro.org>,Eric Biederman <ebiederm@xmission.com>,Tejun Heo <tj@kernel.org>,Paolo Bonzini <pbonzini@redhat.com>,Andrew Morton <akpm@linux-foundation.org>,"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,Lu Baolu <baolu.lu@linux.intel.com>
From: hpa@zytor.com
Message-ID: <AF533772-BD88-4EDA-AD26-7EFA2878F123@zytor.com>
On July 26, 2017 9:24:45 PM GMT+02:00, Brijesh Singh <brijesh.singh@amd.com> wrote:
>
>Hi Arnd and David,
>
>On 07/26/2017 05:45 AM, Arnd Bergmann wrote:
>> On Tue, Jul 25, 2017 at 11:51 AM, David Laight
><David.Laight@aculab.com> wrote:
>>> From: Brijesh Singh
>>>> Sent: 24 July 2017 20:08
>>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>>
>>>> Secure Encrypted Virtualization (SEV) does not support string I/O,
>so
>>>> unroll the string I/O operation into a loop operating on one
>element at
>>>> a time.
>>>>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>>> ---
>>>> arch/x86/include/asm/io.h | 26 ++++++++++++++++++++++----
>>>> 1 file changed, 22 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
>>>> index e080a39..2f3c002 100644
>>>> --- a/arch/x86/include/asm/io.h
>>>> +++ b/arch/x86/include/asm/io.h
>>>> @@ -327,14 +327,32 @@ static inline unsigned type in##bwl##_p(int
>port) \
>>>>
> \
>>>> static inline void outs##bwl(int port, const void *addr, unsigned
>long count) \
>>>> {
>>
>> This will clash with a fix I did to add a "memory" clobber
>> for the traditional implementation, see
>> https://patchwork.kernel.org/patch/9854573/
>>
>>> Is it even worth leaving these as inline functions?
>>> Given the speed of IO cycles it is unlikely that the cost of calling
>a real
>>> function will be significant.
>>> The code bloat reduction will be significant.
>>
>> I think the smallest code would be the original "rep insb" etc, which
>> should be smaller than a function call, unlike the loop. Then again,
>> there is a rather small number of affected device drivers, almost all
>> of them for ancient hardware that you won't even build in a 64-bit
>> x86 kernel, see the list below. The only user I found that is
>actually
>> still relevant is drivers/tty/hvc/hvc_xen.c, which uses it for the
>early
>> console.
>
>
>There are some indirect user of string I/O functions. The following
>functions
>defined in lib/iomap.c calls rep version of ins and outs.
>
>- ioread8_rep, ioread16_rep, ioread32_rep
>- iowrite8_rep, iowrite16_rep, iowrite32_rep
>
>I found that several drivers use above functions.
>
>Here is one approach to convert it into non-inline functions. In this
>approach,
>I have added a new file arch/x86/kernel/io.c which provides non rep
>version of
>string I/O routines. The file gets built and used only when
>AMD_MEM_ENCRYPT is
>enabled. On positive side, if we don't build kernel with
>AMD_MEM_ENCRYPT support
>then we use inline routines, when AMD_MEM_ENCRYPT is built then we make
>a function
>call. Inside the function we unroll only when SEV is active.
>
>Do you see any issue with this approach ? thanks
>
>diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
>index e080a39..104927d 100644
>--- a/arch/x86/include/asm/io.h
>+++ b/arch/x86/include/asm/io.h
>@@ -323,8 +323,9 @@ static inline unsigned type in##bwl##_p(int port)
> \
> unsigned type value = in##bwl(port); \
> slow_down_io(); \
> return value; \
>-}
>\
>-
>\
>+}
>+
>+#define BUILDIO_REP(bwl, bw, type)
>\
>static inline void outs##bwl(int port, const void *addr, unsigned long
>count) \
>{
>\
> asm volatile("rep; outs" #bwl \
>@@ -335,12 +336,31 @@ static inline void ins##bwl(int port, void *addr,
>unsigned long count) \
>{
>\
> asm volatile("rep; ins" #bwl \
> : "+D"(addr), "+c"(count) : "d"(port)); \
>-}
>+}
>\
>
> BUILDIO(b, b, char)
> BUILDIO(w, w, short)
> BUILDIO(l, , int)
>
>+#ifdef CONFIG_AMD_MEM_ENCRYPT
>+extern void outsb_try_rep(int port, const void *addr, unsigned long
>count);
>+extern void insb_try_rep(int port, void *addr, unsigned long count);
>+extern void outsw_try_rep(int port, const void *addr, unsigned long
>count);
>+extern void insw_try_rep(int port, void *addr, unsigned long count);
>+extern void outsl_try_rep(int port, const void *addr, unsigned long
>count);
>+extern void insl_try_rep(int port, void *addr, unsigned long count);
>+#define outsb outsb_try_rep
>+#define insb insb_try_rep
>+#define outsw outsw_try_rep
>+#define insw insw_try_rep
>+#define outsl outsl_try_rep
>+#define insl insl_try_rep
>+#else
>+BUILDIO_REP(b, b, char)
>+BUILDIO_REP(w, w, short)
>+BUILDIO_REP(l, , int)
>+#endif
>+
> extern void *xlate_dev_mem_ptr(phys_addr_t phys);
> extern void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr);
>
>diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>index a01892b..3b6e2a3 100644
>--- a/arch/x86/kernel/Makefile
>+++ b/arch/x86/kernel/Makefile
>@@ -42,6 +42,7 @@ CFLAGS_irq.o := -I$(src)/../include/asm/trace
>
> obj-y := process_$(BITS).o signal.o
> obj-$(CONFIG_COMPAT) += signal_compat.o
>+obj-$(CONFIG_AMD_MEM_ENCRYPT) += io.o
>obj-y += traps.o irq.o irq_$(BITS).o
>dumpstack_$(BITS).o
> obj-y += time.o ioport.o dumpstack.o nmi.o
> obj-$(CONFIG_MODIFY_LDT_SYSCALL) += ldt.o
>diff --git a/arch/x86/kernel/io.c b/arch/x86/kernel/io.c
>new file mode 100644
>index 0000000..f58afa9
>--- /dev/null
>+++ b/arch/x86/kernel/io.c
>@@ -0,0 +1,87 @@
>+#include <linux/types.h>
>+#include <linux/io.h>
>+#include <asm/io.h>
>+
>+void outsb_try_rep(int port, const void *addr, unsigned long count)
>+{
>+ if (sev_active()) {
>+ unsigned char *value = (unsigned char *)addr;
>+ while (count) {
>+ outb(*value, port);
>+ value++;
>+ count--;
>+ }
>+ } else {
>+ asm volatile("rep; outsb" : "+S"(addr), "+c"(count) :
>"d"(port));
>+ }
>+}
>+
>+void insb_try_rep(int port, void *addr, unsigned long count)
>+{
>+ if (sev_active()) {
>+ unsigned char *value = (unsigned char *)addr;
>+ while (count) {
>+ *value = inb(port);
>+ value++;
>+ count--;
>+ }
>+ } else {
>+ asm volatile("rep; insb" : "+D"(addr), "+c"(count) :
>"d"(port));
>+ }
>+}
>+
>+void outsw_try_rep(int port, const void *addr, unsigned long count)
>+{
>+ if (sev_active()) {
>+ unsigned short *value = (unsigned short *)addr;
>+ while (count) {
>+ outw(*value, port);
>+ value++;
>+ count--;
>+ }
>+ } else {
>+ asm volatile("rep; outsw" : "+S"(addr), "+c"(count) :
>"d"(port));
>+ }
>+}
>+void insw_try_rep(int port, void *addr, unsigned long count)
>+{
>+ if (sev_active()) {
>+ unsigned short *value = (unsigned short *)addr;
>+ while (count) {
>+ *value = inw(port);
>+ value++;
>+ count--;
>+ }
>+ } else {
>+ asm volatile("rep; insw" : "+D"(addr), "+c"(count) :
>"d"(port));
>+ }
>+}
>+
>+void outsl_try_rep(int port, const void *addr, unsigned long count)
>+{
>+ if (sev_active()) {
>+ unsigned int *value = (unsigned int *)addr;
>+ while (count) {
>+ outl(*value, port);
>+ value++;
>+ count--;
>+ }
>+ } else {
>+ asm volatile("rep; outsl" : "+S"(addr), "+c"(count) :
>"d"(port));
>+ }
>+}
>+
>+void insl_try_rep(int port, void *addr, unsigned long count)
>+{
>+ if (sev_active()) {
>+ unsigned int *value = (unsigned int *)addr;
>+ while (count) {
>+ *value = inl(port);
>+ value++;
>+ count--;
>+ }
>+ } else {
>+ asm volatile("rep; insl" : "+D"(addr), "+c"(count) :
>"d"(port));
>+ }
>+}
What the heck?
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
[toc] | [prev] | [next] | [standalone]
| From | David Laight <David.Laight@ACULAB.COM> |
|---|---|
| Date | 2017-07-27 09:50 +0200 |
| Message-ID | <u7LIt-3gV-7@gated-at.bofh.it> |
| In reply to | #1697516 |
From: Brijesh Singh > Sent: 26 July 2017 21:07 ... > I am not sure if I understand your concern. > > Are you commenting on amount of code duplication ? If so, I can certainly improve > and use the similar macro used into header file to generate the functions body. If you are careful the real functions could expand the inline functions that get used when SEV is compiled out. Oh, if you are looking at this, can you fix memcpy_to_io() so that it is never 'rep movsb'? David
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web