Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
Hi On Wed, Feb 28, 2018 at 6:17 PM, Michael S. Tsirkinwrote: > On Wed, Feb 28, 2018 at 04:41:51PM +0100, Marc-André Lureau wrote: >> I don't know if it's always safe to enable dma in read_raw(), how >> could we know? Is there a check we could use to choose one or ther >> other (and thus avoiding explicit dma/readfn argument)? > > IMHO the way to go is not to try to do zero copy. > Allocate a buffer and DMA there, then copy. Sounds fine to me, I'll resend this patch separately if the rest from v16 is applied. thanks
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
Hi On Wed, Feb 28, 2018 at 6:17 PM, Michael S. Tsirkin wrote: > On Wed, Feb 28, 2018 at 04:41:51PM +0100, Marc-André Lureau wrote: >> I don't know if it's always safe to enable dma in read_raw(), how >> could we know? Is there a check we could use to choose one or ther >> other (and thus avoiding explicit dma/readfn argument)? > > IMHO the way to go is not to try to do zero copy. > Allocate a buffer and DMA there, then copy. Sounds fine to me, I'll resend this patch separately if the rest from v16 is applied. thanks
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
On Wed, Feb 28, 2018 at 04:41:51PM +0100, Marc-André Lureau wrote: > I don't know if it's always safe to enable dma in read_raw(), how > could we know? Is there a check we could use to choose one or ther > other (and thus avoiding explicit dma/readfn argument)? IMHO the way to go is not to try to do zero copy. Allocate a buffer and DMA there, then copy. -- MST
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
On Wed, Feb 28, 2018 at 04:41:51PM +0100, Marc-André Lureau wrote: > I don't know if it's always safe to enable dma in read_raw(), how > could we know? Is there a check we could use to choose one or ther > other (and thus avoiding explicit dma/readfn argument)? IMHO the way to go is not to try to do zero copy. Allocate a buffer and DMA there, then copy. -- MST
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
On Wed, Feb 28, 2018 at 05:00:58PM +0100, Marc-André Lureau wrote: > Hi > > On Wed, Feb 28, 2018 at 4:48 PM, Michael S. Tsirkinwrote: > > On Wed, Feb 28, 2018 at 04:41:51PM +0100, Marc-André Lureau wrote: > >> I don't know if it's always safe to enable dma in read_raw(), how > >> could we know? Is there a check we could use to choose one or ther > >> other (and thus avoiding explicit dma/readfn argument)? > > > > I'm not sure - but does it really matter? Is anyone reading large files > > like this in production where speed matters? > > Why even bother with DMA? > > The difference is quite significante for not so small files, as shown above. > > And if they access the fw_cfg entries at boot time, or when starting > things etc, this may speed things up. Question would be whether anyone at all does this. -- MST
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
On Wed, Feb 28, 2018 at 05:00:58PM +0100, Marc-André Lureau wrote: > Hi > > On Wed, Feb 28, 2018 at 4:48 PM, Michael S. Tsirkin wrote: > > On Wed, Feb 28, 2018 at 04:41:51PM +0100, Marc-André Lureau wrote: > >> I don't know if it's always safe to enable dma in read_raw(), how > >> could we know? Is there a check we could use to choose one or ther > >> other (and thus avoiding explicit dma/readfn argument)? > > > > I'm not sure - but does it really matter? Is anyone reading large files > > like this in production where speed matters? > > Why even bother with DMA? > > The difference is quite significante for not so small files, as shown above. > > And if they access the fw_cfg entries at boot time, or when starting > things etc, this may speed things up. Question would be whether anyone at all does this. -- MST
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
Hi On Wed, Feb 28, 2018 at 4:48 PM, Michael S. Tsirkinwrote: > On Wed, Feb 28, 2018 at 04:41:51PM +0100, Marc-André Lureau wrote: >> I don't know if it's always safe to enable dma in read_raw(), how >> could we know? Is there a check we could use to choose one or ther >> other (and thus avoiding explicit dma/readfn argument)? > > I'm not sure - but does it really matter? Is anyone reading large files > like this in production where speed matters? > Why even bother with DMA? The difference is quite significante for not so small files, as shown above. And if they access the fw_cfg entries at boot time, or when starting things etc, this may speed things up.
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
Hi On Wed, Feb 28, 2018 at 4:48 PM, Michael S. Tsirkin wrote: > On Wed, Feb 28, 2018 at 04:41:51PM +0100, Marc-André Lureau wrote: >> I don't know if it's always safe to enable dma in read_raw(), how >> could we know? Is there a check we could use to choose one or ther >> other (and thus avoiding explicit dma/readfn argument)? > > I'm not sure - but does it really matter? Is anyone reading large files > like this in production where speed matters? > Why even bother with DMA? The difference is quite significante for not so small files, as shown above. And if they access the fw_cfg entries at boot time, or when starting things etc, this may speed things up.
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
On Wed, Feb 28, 2018 at 04:41:51PM +0100, Marc-André Lureau wrote: > I don't know if it's always safe to enable dma in read_raw(), how > could we know? Is there a check we could use to choose one or ther > other (and thus avoiding explicit dma/readfn argument)? I'm not sure - but does it really matter? Is anyone reading large files like this in production where speed matters? Why even bother with DMA? -- MST
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
On Wed, Feb 28, 2018 at 04:41:51PM +0100, Marc-André Lureau wrote: > I don't know if it's always safe to enable dma in read_raw(), how > could we know? Is there a check we could use to choose one or ther > other (and thus avoiding explicit dma/readfn argument)? I'm not sure - but does it really matter? Is anyone reading large files like this in production where speed matters? Why even bother with DMA? -- MST
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
On Wed, Feb 28, 2018 at 4:35 PM, Michael S. Tsirkinwrote: > On Wed, Feb 28, 2018 at 01:27:02PM +0100, Marc-André Lureau wrote: >> Hi >> >> On Tue, Feb 27, 2018 at 1:04 AM, Michael S. Tsirkin wrote: >> > On Thu, Feb 15, 2018 at 10:33:12PM +0100, Marc-André Lureau wrote: >> >> Modify fw_cfg_read_blob() to use DMA if the device supports it. >> >> Return errors, because the operation may fail. >> >> >> >> So far, only one call in fw_cfg_register_dir_entries() is using >> >> kmalloc'ed buf and is thus clearly eligible to DMA read. >> >> >> >> Initially, I didn't implement DMA read to speed up boot time, but as a >> >> first step before introducing DMA write (since read operations were >> >> already presents). Even more, I didn't realize fw-cfg entries were >> >> being read by the kernel during boot by default. But actally fw-cfg >> >> entries are being populated during module probe. I knew DMA improved a >> >> lot bios boot time (the main reason the DMA interface was added >> >> afaik). Let see the time it would take to read the whole ACPI >> >> tables (128kb allocated) >> >> >> >> # time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw >> >> - with DMA: sys 0m0.003s >> >> - without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s >> >> >> >> FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel >> >> boot to populate sysfs qemu_fw_cfg directory, and it is quite >> >> small (1-2kb). Since it does not expose itself, in order to measure >> >> the time it takes to read such small file, I took a comparable sized >> >> file of 2048 bytes and exposed it (-fw_cfg test,file=file with a >> >> modified read_raw enabling DMA) >> >> >> >> # perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw >> >> >/dev/null >> >> - with DMA: >> >> 0.636037 task-clock (msec) #0.141 CPUs >> >> utilized( +- 1.19% ) >> >> - without DMA: >> >> 6.430128 task-clock (msec) #0.622 CPUs >> >> utilized( +- 0.22% ) >> >> >> >> That's a few msec saved during boot by enabling DMA read (the gain >> >> would be more substantial if other & bigger fw-cfg entries are read by >> >> others from sysfs, unfortunately, it's not clear if we can always >> >> enable DMA there) >> >> >> >> Signed-off-by: Marc-André Lureau >> >> --- >> >> drivers/firmware/qemu_fw_cfg.c | 61 >> >> ++ >> >> 1 file changed, 50 insertions(+), 11 deletions(-) >> >> >> >> diff --git a/drivers/firmware/qemu_fw_cfg.c >> >> b/drivers/firmware/qemu_fw_cfg.c >> >> index 3015e77aebca..94df57e9be66 100644 >> >> --- a/drivers/firmware/qemu_fw_cfg.c >> >> +++ b/drivers/firmware/qemu_fw_cfg.c >> >> @@ -124,12 +124,47 @@ static ssize_t fw_cfg_dma_transfer(void *address, >> >> u32 length, u32 control) >> >> return ret; >> >> } >> >> >> >> +/* with acpi & dev locks taken */ >> >> +static ssize_t fw_cfg_read_blob_dma(u16 key, >> >> + void *buf, loff_t pos, size_t count) >> >> +{ >> >> + ssize_t ret; >> >> + >> >> + if (pos == 0) { >> >> + ret = fw_cfg_dma_transfer(buf, count, key << 16 >> >> + | FW_CFG_DMA_CTL_SELECT >> >> + | FW_CFG_DMA_CTL_READ); >> >> + } else { >> >> + fw_cfg_sel_endianness(key); >> >> + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP); >> >> + if (ret < 0) >> >> + return ret; >> >> + ret = fw_cfg_dma_transfer(buf, count, >> >> + FW_CFG_DMA_CTL_READ); >> >> + } >> >> + >> >> + return ret; >> >> +} >> >> + >> >> +/* with acpi & dev locks taken */ >> >> +static ssize_t fw_cfg_read_blob_io(u16 key, >> >> + void *buf, loff_t pos, size_t count) >> >> +{ >> >> + fw_cfg_sel_endianness(key); >> >> + while (pos-- > 0) >> >> + ioread8(fw_cfg_reg_data); >> >> + ioread8_rep(fw_cfg_reg_data, buf, count); >> >> + return count; >> >> +} >> >> + >> >> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) >> >> */ >> >> static ssize_t fw_cfg_read_blob(u16 key, >> >> - void *buf, loff_t pos, size_t count) >> >> + void *buf, loff_t pos, size_t count, >> >> + bool dma) >> >> { >> >> u32 glk = -1U; >> >> acpi_status status; >> >> + ssize_t ret; >> >> >> >> /* If we have ACPI, ensure mutual exclusion against any potential >> >>* device access by the firmware, e.g. via AML methods: >> > >> > so this adds a dma flag to fw_cfg_read_blob. >> > >> > >> > >> >> @@ -143,14 +178,17 @@ static ssize_t fw_cfg_read_blob(u16 key, >> >> } >> >> >> >> mutex_lock(_cfg_dev_lock); >> >> - fw_cfg_sel_endianness(key); >> >> -
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
On Wed, Feb 28, 2018 at 4:35 PM, Michael S. Tsirkin wrote: > On Wed, Feb 28, 2018 at 01:27:02PM +0100, Marc-André Lureau wrote: >> Hi >> >> On Tue, Feb 27, 2018 at 1:04 AM, Michael S. Tsirkin wrote: >> > On Thu, Feb 15, 2018 at 10:33:12PM +0100, Marc-André Lureau wrote: >> >> Modify fw_cfg_read_blob() to use DMA if the device supports it. >> >> Return errors, because the operation may fail. >> >> >> >> So far, only one call in fw_cfg_register_dir_entries() is using >> >> kmalloc'ed buf and is thus clearly eligible to DMA read. >> >> >> >> Initially, I didn't implement DMA read to speed up boot time, but as a >> >> first step before introducing DMA write (since read operations were >> >> already presents). Even more, I didn't realize fw-cfg entries were >> >> being read by the kernel during boot by default. But actally fw-cfg >> >> entries are being populated during module probe. I knew DMA improved a >> >> lot bios boot time (the main reason the DMA interface was added >> >> afaik). Let see the time it would take to read the whole ACPI >> >> tables (128kb allocated) >> >> >> >> # time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw >> >> - with DMA: sys 0m0.003s >> >> - without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s >> >> >> >> FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel >> >> boot to populate sysfs qemu_fw_cfg directory, and it is quite >> >> small (1-2kb). Since it does not expose itself, in order to measure >> >> the time it takes to read such small file, I took a comparable sized >> >> file of 2048 bytes and exposed it (-fw_cfg test,file=file with a >> >> modified read_raw enabling DMA) >> >> >> >> # perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw >> >> >/dev/null >> >> - with DMA: >> >> 0.636037 task-clock (msec) #0.141 CPUs >> >> utilized( +- 1.19% ) >> >> - without DMA: >> >> 6.430128 task-clock (msec) #0.622 CPUs >> >> utilized( +- 0.22% ) >> >> >> >> That's a few msec saved during boot by enabling DMA read (the gain >> >> would be more substantial if other & bigger fw-cfg entries are read by >> >> others from sysfs, unfortunately, it's not clear if we can always >> >> enable DMA there) >> >> >> >> Signed-off-by: Marc-André Lureau >> >> --- >> >> drivers/firmware/qemu_fw_cfg.c | 61 >> >> ++ >> >> 1 file changed, 50 insertions(+), 11 deletions(-) >> >> >> >> diff --git a/drivers/firmware/qemu_fw_cfg.c >> >> b/drivers/firmware/qemu_fw_cfg.c >> >> index 3015e77aebca..94df57e9be66 100644 >> >> --- a/drivers/firmware/qemu_fw_cfg.c >> >> +++ b/drivers/firmware/qemu_fw_cfg.c >> >> @@ -124,12 +124,47 @@ static ssize_t fw_cfg_dma_transfer(void *address, >> >> u32 length, u32 control) >> >> return ret; >> >> } >> >> >> >> +/* with acpi & dev locks taken */ >> >> +static ssize_t fw_cfg_read_blob_dma(u16 key, >> >> + void *buf, loff_t pos, size_t count) >> >> +{ >> >> + ssize_t ret; >> >> + >> >> + if (pos == 0) { >> >> + ret = fw_cfg_dma_transfer(buf, count, key << 16 >> >> + | FW_CFG_DMA_CTL_SELECT >> >> + | FW_CFG_DMA_CTL_READ); >> >> + } else { >> >> + fw_cfg_sel_endianness(key); >> >> + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP); >> >> + if (ret < 0) >> >> + return ret; >> >> + ret = fw_cfg_dma_transfer(buf, count, >> >> + FW_CFG_DMA_CTL_READ); >> >> + } >> >> + >> >> + return ret; >> >> +} >> >> + >> >> +/* with acpi & dev locks taken */ >> >> +static ssize_t fw_cfg_read_blob_io(u16 key, >> >> + void *buf, loff_t pos, size_t count) >> >> +{ >> >> + fw_cfg_sel_endianness(key); >> >> + while (pos-- > 0) >> >> + ioread8(fw_cfg_reg_data); >> >> + ioread8_rep(fw_cfg_reg_data, buf, count); >> >> + return count; >> >> +} >> >> + >> >> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) >> >> */ >> >> static ssize_t fw_cfg_read_blob(u16 key, >> >> - void *buf, loff_t pos, size_t count) >> >> + void *buf, loff_t pos, size_t count, >> >> + bool dma) >> >> { >> >> u32 glk = -1U; >> >> acpi_status status; >> >> + ssize_t ret; >> >> >> >> /* If we have ACPI, ensure mutual exclusion against any potential >> >>* device access by the firmware, e.g. via AML methods: >> > >> > so this adds a dma flag to fw_cfg_read_blob. >> > >> > >> > >> >> @@ -143,14 +178,17 @@ static ssize_t fw_cfg_read_blob(u16 key, >> >> } >> >> >> >> mutex_lock(_cfg_dev_lock); >> >> - fw_cfg_sel_endianness(key); >> >> - while (pos-- > 0) >> >> - ioread8(fw_cfg_reg_data);
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
On Wed, Feb 28, 2018 at 01:27:02PM +0100, Marc-André Lureau wrote: > Hi > > On Tue, Feb 27, 2018 at 1:04 AM, Michael S. Tsirkinwrote: > > On Thu, Feb 15, 2018 at 10:33:12PM +0100, Marc-André Lureau wrote: > >> Modify fw_cfg_read_blob() to use DMA if the device supports it. > >> Return errors, because the operation may fail. > >> > >> So far, only one call in fw_cfg_register_dir_entries() is using > >> kmalloc'ed buf and is thus clearly eligible to DMA read. > >> > >> Initially, I didn't implement DMA read to speed up boot time, but as a > >> first step before introducing DMA write (since read operations were > >> already presents). Even more, I didn't realize fw-cfg entries were > >> being read by the kernel during boot by default. But actally fw-cfg > >> entries are being populated during module probe. I knew DMA improved a > >> lot bios boot time (the main reason the DMA interface was added > >> afaik). Let see the time it would take to read the whole ACPI > >> tables (128kb allocated) > >> > >> # time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw > >> - with DMA: sys 0m0.003s > >> - without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s > >> > >> FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel > >> boot to populate sysfs qemu_fw_cfg directory, and it is quite > >> small (1-2kb). Since it does not expose itself, in order to measure > >> the time it takes to read such small file, I took a comparable sized > >> file of 2048 bytes and exposed it (-fw_cfg test,file=file with a > >> modified read_raw enabling DMA) > >> > >> # perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw > >> >/dev/null > >> - with DMA: > >> 0.636037 task-clock (msec) #0.141 CPUs utilized > >>( +- 1.19% ) > >> - without DMA: > >> 6.430128 task-clock (msec) #0.622 CPUs utilized > >>( +- 0.22% ) > >> > >> That's a few msec saved during boot by enabling DMA read (the gain > >> would be more substantial if other & bigger fw-cfg entries are read by > >> others from sysfs, unfortunately, it's not clear if we can always > >> enable DMA there) > >> > >> Signed-off-by: Marc-André Lureau > >> --- > >> drivers/firmware/qemu_fw_cfg.c | 61 > >> ++ > >> 1 file changed, 50 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/firmware/qemu_fw_cfg.c > >> b/drivers/firmware/qemu_fw_cfg.c > >> index 3015e77aebca..94df57e9be66 100644 > >> --- a/drivers/firmware/qemu_fw_cfg.c > >> +++ b/drivers/firmware/qemu_fw_cfg.c > >> @@ -124,12 +124,47 @@ static ssize_t fw_cfg_dma_transfer(void *address, > >> u32 length, u32 control) > >> return ret; > >> } > >> > >> +/* with acpi & dev locks taken */ > >> +static ssize_t fw_cfg_read_blob_dma(u16 key, > >> + void *buf, loff_t pos, size_t count) > >> +{ > >> + ssize_t ret; > >> + > >> + if (pos == 0) { > >> + ret = fw_cfg_dma_transfer(buf, count, key << 16 > >> + | FW_CFG_DMA_CTL_SELECT > >> + | FW_CFG_DMA_CTL_READ); > >> + } else { > >> + fw_cfg_sel_endianness(key); > >> + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP); > >> + if (ret < 0) > >> + return ret; > >> + ret = fw_cfg_dma_transfer(buf, count, > >> + FW_CFG_DMA_CTL_READ); > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +/* with acpi & dev locks taken */ > >> +static ssize_t fw_cfg_read_blob_io(u16 key, > >> + void *buf, loff_t pos, size_t count) > >> +{ > >> + fw_cfg_sel_endianness(key); > >> + while (pos-- > 0) > >> + ioread8(fw_cfg_reg_data); > >> + ioread8_rep(fw_cfg_reg_data, buf, count); > >> + return count; > >> +} > >> + > >> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) > >> */ > >> static ssize_t fw_cfg_read_blob(u16 key, > >> - void *buf, loff_t pos, size_t count) > >> + void *buf, loff_t pos, size_t count, > >> + bool dma) > >> { > >> u32 glk = -1U; > >> acpi_status status; > >> + ssize_t ret; > >> > >> /* If we have ACPI, ensure mutual exclusion against any potential > >>* device access by the firmware, e.g. via AML methods: > > > > so this adds a dma flag to fw_cfg_read_blob. > > > > > > > >> @@ -143,14 +178,17 @@ static ssize_t fw_cfg_read_blob(u16 key, > >> } > >> > >> mutex_lock(_cfg_dev_lock); > >> - fw_cfg_sel_endianness(key); > >> - while (pos-- > 0) > >> - ioread8(fw_cfg_reg_data); > >> - ioread8_rep(fw_cfg_reg_data, buf, count); > >> + if (dma && fw_cfg_dma_enabled()) { > >> + ret =
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
On Wed, Feb 28, 2018 at 01:27:02PM +0100, Marc-André Lureau wrote: > Hi > > On Tue, Feb 27, 2018 at 1:04 AM, Michael S. Tsirkin wrote: > > On Thu, Feb 15, 2018 at 10:33:12PM +0100, Marc-André Lureau wrote: > >> Modify fw_cfg_read_blob() to use DMA if the device supports it. > >> Return errors, because the operation may fail. > >> > >> So far, only one call in fw_cfg_register_dir_entries() is using > >> kmalloc'ed buf and is thus clearly eligible to DMA read. > >> > >> Initially, I didn't implement DMA read to speed up boot time, but as a > >> first step before introducing DMA write (since read operations were > >> already presents). Even more, I didn't realize fw-cfg entries were > >> being read by the kernel during boot by default. But actally fw-cfg > >> entries are being populated during module probe. I knew DMA improved a > >> lot bios boot time (the main reason the DMA interface was added > >> afaik). Let see the time it would take to read the whole ACPI > >> tables (128kb allocated) > >> > >> # time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw > >> - with DMA: sys 0m0.003s > >> - without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s > >> > >> FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel > >> boot to populate sysfs qemu_fw_cfg directory, and it is quite > >> small (1-2kb). Since it does not expose itself, in order to measure > >> the time it takes to read such small file, I took a comparable sized > >> file of 2048 bytes and exposed it (-fw_cfg test,file=file with a > >> modified read_raw enabling DMA) > >> > >> # perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw > >> >/dev/null > >> - with DMA: > >> 0.636037 task-clock (msec) #0.141 CPUs utilized > >>( +- 1.19% ) > >> - without DMA: > >> 6.430128 task-clock (msec) #0.622 CPUs utilized > >>( +- 0.22% ) > >> > >> That's a few msec saved during boot by enabling DMA read (the gain > >> would be more substantial if other & bigger fw-cfg entries are read by > >> others from sysfs, unfortunately, it's not clear if we can always > >> enable DMA there) > >> > >> Signed-off-by: Marc-André Lureau > >> --- > >> drivers/firmware/qemu_fw_cfg.c | 61 > >> ++ > >> 1 file changed, 50 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/firmware/qemu_fw_cfg.c > >> b/drivers/firmware/qemu_fw_cfg.c > >> index 3015e77aebca..94df57e9be66 100644 > >> --- a/drivers/firmware/qemu_fw_cfg.c > >> +++ b/drivers/firmware/qemu_fw_cfg.c > >> @@ -124,12 +124,47 @@ static ssize_t fw_cfg_dma_transfer(void *address, > >> u32 length, u32 control) > >> return ret; > >> } > >> > >> +/* with acpi & dev locks taken */ > >> +static ssize_t fw_cfg_read_blob_dma(u16 key, > >> + void *buf, loff_t pos, size_t count) > >> +{ > >> + ssize_t ret; > >> + > >> + if (pos == 0) { > >> + ret = fw_cfg_dma_transfer(buf, count, key << 16 > >> + | FW_CFG_DMA_CTL_SELECT > >> + | FW_CFG_DMA_CTL_READ); > >> + } else { > >> + fw_cfg_sel_endianness(key); > >> + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP); > >> + if (ret < 0) > >> + return ret; > >> + ret = fw_cfg_dma_transfer(buf, count, > >> + FW_CFG_DMA_CTL_READ); > >> + } > >> + > >> + return ret; > >> +} > >> + > >> +/* with acpi & dev locks taken */ > >> +static ssize_t fw_cfg_read_blob_io(u16 key, > >> + void *buf, loff_t pos, size_t count) > >> +{ > >> + fw_cfg_sel_endianness(key); > >> + while (pos-- > 0) > >> + ioread8(fw_cfg_reg_data); > >> + ioread8_rep(fw_cfg_reg_data, buf, count); > >> + return count; > >> +} > >> + > >> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) > >> */ > >> static ssize_t fw_cfg_read_blob(u16 key, > >> - void *buf, loff_t pos, size_t count) > >> + void *buf, loff_t pos, size_t count, > >> + bool dma) > >> { > >> u32 glk = -1U; > >> acpi_status status; > >> + ssize_t ret; > >> > >> /* If we have ACPI, ensure mutual exclusion against any potential > >>* device access by the firmware, e.g. via AML methods: > > > > so this adds a dma flag to fw_cfg_read_blob. > > > > > > > >> @@ -143,14 +178,17 @@ static ssize_t fw_cfg_read_blob(u16 key, > >> } > >> > >> mutex_lock(_cfg_dev_lock); > >> - fw_cfg_sel_endianness(key); > >> - while (pos-- > 0) > >> - ioread8(fw_cfg_reg_data); > >> - ioread8_rep(fw_cfg_reg_data, buf, count); > >> + if (dma && fw_cfg_dma_enabled()) { > >> + ret = fw_cfg_read_blob_dma(key, buf, pos, count); > >> + }
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
Hi On Tue, Feb 27, 2018 at 1:04 AM, Michael S. Tsirkinwrote: > On Thu, Feb 15, 2018 at 10:33:12PM +0100, Marc-André Lureau wrote: >> Modify fw_cfg_read_blob() to use DMA if the device supports it. >> Return errors, because the operation may fail. >> >> So far, only one call in fw_cfg_register_dir_entries() is using >> kmalloc'ed buf and is thus clearly eligible to DMA read. >> >> Initially, I didn't implement DMA read to speed up boot time, but as a >> first step before introducing DMA write (since read operations were >> already presents). Even more, I didn't realize fw-cfg entries were >> being read by the kernel during boot by default. But actally fw-cfg >> entries are being populated during module probe. I knew DMA improved a >> lot bios boot time (the main reason the DMA interface was added >> afaik). Let see the time it would take to read the whole ACPI >> tables (128kb allocated) >> >> # time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw >> - with DMA: sys 0m0.003s >> - without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s >> >> FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel >> boot to populate sysfs qemu_fw_cfg directory, and it is quite >> small (1-2kb). Since it does not expose itself, in order to measure >> the time it takes to read such small file, I took a comparable sized >> file of 2048 bytes and exposed it (-fw_cfg test,file=file with a >> modified read_raw enabling DMA) >> >> # perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw >/dev/null >> - with DMA: >> 0.636037 task-clock (msec) #0.141 CPUs utilized >> ( +- 1.19% ) >> - without DMA: >> 6.430128 task-clock (msec) #0.622 CPUs utilized >> ( +- 0.22% ) >> >> That's a few msec saved during boot by enabling DMA read (the gain >> would be more substantial if other & bigger fw-cfg entries are read by >> others from sysfs, unfortunately, it's not clear if we can always >> enable DMA there) >> >> Signed-off-by: Marc-André Lureau >> --- >> drivers/firmware/qemu_fw_cfg.c | 61 >> ++ >> 1 file changed, 50 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c >> index 3015e77aebca..94df57e9be66 100644 >> --- a/drivers/firmware/qemu_fw_cfg.c >> +++ b/drivers/firmware/qemu_fw_cfg.c >> @@ -124,12 +124,47 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 >> length, u32 control) >> return ret; >> } >> >> +/* with acpi & dev locks taken */ >> +static ssize_t fw_cfg_read_blob_dma(u16 key, >> + void *buf, loff_t pos, size_t count) >> +{ >> + ssize_t ret; >> + >> + if (pos == 0) { >> + ret = fw_cfg_dma_transfer(buf, count, key << 16 >> + | FW_CFG_DMA_CTL_SELECT >> + | FW_CFG_DMA_CTL_READ); >> + } else { >> + fw_cfg_sel_endianness(key); >> + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP); >> + if (ret < 0) >> + return ret; >> + ret = fw_cfg_dma_transfer(buf, count, >> + FW_CFG_DMA_CTL_READ); >> + } >> + >> + return ret; >> +} >> + >> +/* with acpi & dev locks taken */ >> +static ssize_t fw_cfg_read_blob_io(u16 key, >> + void *buf, loff_t pos, size_t count) >> +{ >> + fw_cfg_sel_endianness(key); >> + while (pos-- > 0) >> + ioread8(fw_cfg_reg_data); >> + ioread8_rep(fw_cfg_reg_data, buf, count); >> + return count; >> +} >> + >> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */ >> static ssize_t fw_cfg_read_blob(u16 key, >> - void *buf, loff_t pos, size_t count) >> + void *buf, loff_t pos, size_t count, >> + bool dma) >> { >> u32 glk = -1U; >> acpi_status status; >> + ssize_t ret; >> >> /* If we have ACPI, ensure mutual exclusion against any potential >>* device access by the firmware, e.g. via AML methods: > > so this adds a dma flag to fw_cfg_read_blob. > > > >> @@ -143,14 +178,17 @@ static ssize_t fw_cfg_read_blob(u16 key, >> } >> >> mutex_lock(_cfg_dev_lock); >> - fw_cfg_sel_endianness(key); >> - while (pos-- > 0) >> - ioread8(fw_cfg_reg_data); >> - ioread8_rep(fw_cfg_reg_data, buf, count); >> + if (dma && fw_cfg_dma_enabled()) { >> + ret = fw_cfg_read_blob_dma(key, buf, pos, count); >> + } else { >> + ret = fw_cfg_read_blob_io(key, buf, pos, count); >> + } >> + >> mutex_unlock(_cfg_dev_lock); >> >> acpi_release_global_lock(glk); >> - return count; >> + >> + return ret; >> } >> >> #ifdef CONFIG_CRASH_CORE > > If set to false
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
Hi On Tue, Feb 27, 2018 at 1:04 AM, Michael S. Tsirkin wrote: > On Thu, Feb 15, 2018 at 10:33:12PM +0100, Marc-André Lureau wrote: >> Modify fw_cfg_read_blob() to use DMA if the device supports it. >> Return errors, because the operation may fail. >> >> So far, only one call in fw_cfg_register_dir_entries() is using >> kmalloc'ed buf and is thus clearly eligible to DMA read. >> >> Initially, I didn't implement DMA read to speed up boot time, but as a >> first step before introducing DMA write (since read operations were >> already presents). Even more, I didn't realize fw-cfg entries were >> being read by the kernel during boot by default. But actally fw-cfg >> entries are being populated during module probe. I knew DMA improved a >> lot bios boot time (the main reason the DMA interface was added >> afaik). Let see the time it would take to read the whole ACPI >> tables (128kb allocated) >> >> # time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw >> - with DMA: sys 0m0.003s >> - without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s >> >> FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel >> boot to populate sysfs qemu_fw_cfg directory, and it is quite >> small (1-2kb). Since it does not expose itself, in order to measure >> the time it takes to read such small file, I took a comparable sized >> file of 2048 bytes and exposed it (-fw_cfg test,file=file with a >> modified read_raw enabling DMA) >> >> # perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw >/dev/null >> - with DMA: >> 0.636037 task-clock (msec) #0.141 CPUs utilized >> ( +- 1.19% ) >> - without DMA: >> 6.430128 task-clock (msec) #0.622 CPUs utilized >> ( +- 0.22% ) >> >> That's a few msec saved during boot by enabling DMA read (the gain >> would be more substantial if other & bigger fw-cfg entries are read by >> others from sysfs, unfortunately, it's not clear if we can always >> enable DMA there) >> >> Signed-off-by: Marc-André Lureau >> --- >> drivers/firmware/qemu_fw_cfg.c | 61 >> ++ >> 1 file changed, 50 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c >> index 3015e77aebca..94df57e9be66 100644 >> --- a/drivers/firmware/qemu_fw_cfg.c >> +++ b/drivers/firmware/qemu_fw_cfg.c >> @@ -124,12 +124,47 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 >> length, u32 control) >> return ret; >> } >> >> +/* with acpi & dev locks taken */ >> +static ssize_t fw_cfg_read_blob_dma(u16 key, >> + void *buf, loff_t pos, size_t count) >> +{ >> + ssize_t ret; >> + >> + if (pos == 0) { >> + ret = fw_cfg_dma_transfer(buf, count, key << 16 >> + | FW_CFG_DMA_CTL_SELECT >> + | FW_CFG_DMA_CTL_READ); >> + } else { >> + fw_cfg_sel_endianness(key); >> + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP); >> + if (ret < 0) >> + return ret; >> + ret = fw_cfg_dma_transfer(buf, count, >> + FW_CFG_DMA_CTL_READ); >> + } >> + >> + return ret; >> +} >> + >> +/* with acpi & dev locks taken */ >> +static ssize_t fw_cfg_read_blob_io(u16 key, >> + void *buf, loff_t pos, size_t count) >> +{ >> + fw_cfg_sel_endianness(key); >> + while (pos-- > 0) >> + ioread8(fw_cfg_reg_data); >> + ioread8_rep(fw_cfg_reg_data, buf, count); >> + return count; >> +} >> + >> /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */ >> static ssize_t fw_cfg_read_blob(u16 key, >> - void *buf, loff_t pos, size_t count) >> + void *buf, loff_t pos, size_t count, >> + bool dma) >> { >> u32 glk = -1U; >> acpi_status status; >> + ssize_t ret; >> >> /* If we have ACPI, ensure mutual exclusion against any potential >>* device access by the firmware, e.g. via AML methods: > > so this adds a dma flag to fw_cfg_read_blob. > > > >> @@ -143,14 +178,17 @@ static ssize_t fw_cfg_read_blob(u16 key, >> } >> >> mutex_lock(_cfg_dev_lock); >> - fw_cfg_sel_endianness(key); >> - while (pos-- > 0) >> - ioread8(fw_cfg_reg_data); >> - ioread8_rep(fw_cfg_reg_data, buf, count); >> + if (dma && fw_cfg_dma_enabled()) { >> + ret = fw_cfg_read_blob_dma(key, buf, pos, count); >> + } else { >> + ret = fw_cfg_read_blob_io(key, buf, pos, count); >> + } >> + >> mutex_unlock(_cfg_dev_lock); >> >> acpi_release_global_lock(glk); >> - return count; >> + >> + return ret; >> } >> >> #ifdef CONFIG_CRASH_CORE > > If set to false it does io, if set to true it does dma. > > I
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
On Thu, Feb 15, 2018 at 10:33:12PM +0100, Marc-André Lureau wrote: > Modify fw_cfg_read_blob() to use DMA if the device supports it. > Return errors, because the operation may fail. > > So far, only one call in fw_cfg_register_dir_entries() is using > kmalloc'ed buf and is thus clearly eligible to DMA read. > > Initially, I didn't implement DMA read to speed up boot time, but as a > first step before introducing DMA write (since read operations were > already presents). Even more, I didn't realize fw-cfg entries were > being read by the kernel during boot by default. But actally fw-cfg > entries are being populated during module probe. I knew DMA improved a > lot bios boot time (the main reason the DMA interface was added > afaik). Let see the time it would take to read the whole ACPI > tables (128kb allocated) > > # time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw > - with DMA: sys 0m0.003s > - without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s > > FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel > boot to populate sysfs qemu_fw_cfg directory, and it is quite > small (1-2kb). Since it does not expose itself, in order to measure > the time it takes to read such small file, I took a comparable sized > file of 2048 bytes and exposed it (-fw_cfg test,file=file with a > modified read_raw enabling DMA) > > # perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw >/dev/null > - with DMA: > 0.636037 task-clock (msec) #0.141 CPUs utilized > ( +- 1.19% ) > - without DMA: > 6.430128 task-clock (msec) #0.622 CPUs utilized > ( +- 0.22% ) > > That's a few msec saved during boot by enabling DMA read (the gain > would be more substantial if other & bigger fw-cfg entries are read by > others from sysfs, unfortunately, it's not clear if we can always > enable DMA there) > > Signed-off-by: Marc-André Lureau> --- > drivers/firmware/qemu_fw_cfg.c | 61 > ++ > 1 file changed, 50 insertions(+), 11 deletions(-) > > diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c > index 3015e77aebca..94df57e9be66 100644 > --- a/drivers/firmware/qemu_fw_cfg.c > +++ b/drivers/firmware/qemu_fw_cfg.c > @@ -124,12 +124,47 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 > length, u32 control) > return ret; > } > > +/* with acpi & dev locks taken */ > +static ssize_t fw_cfg_read_blob_dma(u16 key, > + void *buf, loff_t pos, size_t count) > +{ > + ssize_t ret; > + > + if (pos == 0) { > + ret = fw_cfg_dma_transfer(buf, count, key << 16 > + | FW_CFG_DMA_CTL_SELECT > + | FW_CFG_DMA_CTL_READ); > + } else { > + fw_cfg_sel_endianness(key); > + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP); > + if (ret < 0) > + return ret; > + ret = fw_cfg_dma_transfer(buf, count, > + FW_CFG_DMA_CTL_READ); > + } > + > + return ret; > +} > + > +/* with acpi & dev locks taken */ > +static ssize_t fw_cfg_read_blob_io(u16 key, > + void *buf, loff_t pos, size_t count) > +{ > + fw_cfg_sel_endianness(key); > + while (pos-- > 0) > + ioread8(fw_cfg_reg_data); > + ioread8_rep(fw_cfg_reg_data, buf, count); > + return count; > +} > + > /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */ > static ssize_t fw_cfg_read_blob(u16 key, > - void *buf, loff_t pos, size_t count) > + void *buf, loff_t pos, size_t count, > + bool dma) > { > u32 glk = -1U; > acpi_status status; > + ssize_t ret; > > /* If we have ACPI, ensure mutual exclusion against any potential >* device access by the firmware, e.g. via AML methods: so this adds a dma flag to fw_cfg_read_blob. > @@ -143,14 +178,17 @@ static ssize_t fw_cfg_read_blob(u16 key, > } > > mutex_lock(_cfg_dev_lock); > - fw_cfg_sel_endianness(key); > - while (pos-- > 0) > - ioread8(fw_cfg_reg_data); > - ioread8_rep(fw_cfg_reg_data, buf, count); > + if (dma && fw_cfg_dma_enabled()) { > + ret = fw_cfg_read_blob_dma(key, buf, pos, count); > + } else { > + ret = fw_cfg_read_blob_io(key, buf, pos, count); > + } > + > mutex_unlock(_cfg_dev_lock); > > acpi_release_global_lock(glk); > - return count; > + > + return ret; > } > > #ifdef CONFIG_CRASH_CORE If set to false it does io, if set to true it does dma. I would prefer passing an accessor function pointer since that's clearer than true/false. > @@ -284,7 +322,7 @@ static int
Re: [PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
On Thu, Feb 15, 2018 at 10:33:12PM +0100, Marc-André Lureau wrote: > Modify fw_cfg_read_blob() to use DMA if the device supports it. > Return errors, because the operation may fail. > > So far, only one call in fw_cfg_register_dir_entries() is using > kmalloc'ed buf and is thus clearly eligible to DMA read. > > Initially, I didn't implement DMA read to speed up boot time, but as a > first step before introducing DMA write (since read operations were > already presents). Even more, I didn't realize fw-cfg entries were > being read by the kernel during boot by default. But actally fw-cfg > entries are being populated during module probe. I knew DMA improved a > lot bios boot time (the main reason the DMA interface was added > afaik). Let see the time it would take to read the whole ACPI > tables (128kb allocated) > > # time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw > - with DMA: sys 0m0.003s > - without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s > > FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel > boot to populate sysfs qemu_fw_cfg directory, and it is quite > small (1-2kb). Since it does not expose itself, in order to measure > the time it takes to read such small file, I took a comparable sized > file of 2048 bytes and exposed it (-fw_cfg test,file=file with a > modified read_raw enabling DMA) > > # perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw >/dev/null > - with DMA: > 0.636037 task-clock (msec) #0.141 CPUs utilized > ( +- 1.19% ) > - without DMA: > 6.430128 task-clock (msec) #0.622 CPUs utilized > ( +- 0.22% ) > > That's a few msec saved during boot by enabling DMA read (the gain > would be more substantial if other & bigger fw-cfg entries are read by > others from sysfs, unfortunately, it's not clear if we can always > enable DMA there) > > Signed-off-by: Marc-André Lureau > --- > drivers/firmware/qemu_fw_cfg.c | 61 > ++ > 1 file changed, 50 insertions(+), 11 deletions(-) > > diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c > index 3015e77aebca..94df57e9be66 100644 > --- a/drivers/firmware/qemu_fw_cfg.c > +++ b/drivers/firmware/qemu_fw_cfg.c > @@ -124,12 +124,47 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 > length, u32 control) > return ret; > } > > +/* with acpi & dev locks taken */ > +static ssize_t fw_cfg_read_blob_dma(u16 key, > + void *buf, loff_t pos, size_t count) > +{ > + ssize_t ret; > + > + if (pos == 0) { > + ret = fw_cfg_dma_transfer(buf, count, key << 16 > + | FW_CFG_DMA_CTL_SELECT > + | FW_CFG_DMA_CTL_READ); > + } else { > + fw_cfg_sel_endianness(key); > + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP); > + if (ret < 0) > + return ret; > + ret = fw_cfg_dma_transfer(buf, count, > + FW_CFG_DMA_CTL_READ); > + } > + > + return ret; > +} > + > +/* with acpi & dev locks taken */ > +static ssize_t fw_cfg_read_blob_io(u16 key, > + void *buf, loff_t pos, size_t count) > +{ > + fw_cfg_sel_endianness(key); > + while (pos-- > 0) > + ioread8(fw_cfg_reg_data); > + ioread8_rep(fw_cfg_reg_data, buf, count); > + return count; > +} > + > /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */ > static ssize_t fw_cfg_read_blob(u16 key, > - void *buf, loff_t pos, size_t count) > + void *buf, loff_t pos, size_t count, > + bool dma) > { > u32 glk = -1U; > acpi_status status; > + ssize_t ret; > > /* If we have ACPI, ensure mutual exclusion against any potential >* device access by the firmware, e.g. via AML methods: so this adds a dma flag to fw_cfg_read_blob. > @@ -143,14 +178,17 @@ static ssize_t fw_cfg_read_blob(u16 key, > } > > mutex_lock(_cfg_dev_lock); > - fw_cfg_sel_endianness(key); > - while (pos-- > 0) > - ioread8(fw_cfg_reg_data); > - ioread8_rep(fw_cfg_reg_data, buf, count); > + if (dma && fw_cfg_dma_enabled()) { > + ret = fw_cfg_read_blob_dma(key, buf, pos, count); > + } else { > + ret = fw_cfg_read_blob_io(key, buf, pos, count); > + } > + > mutex_unlock(_cfg_dev_lock); > > acpi_release_global_lock(glk); > - return count; > + > + return ret; > } > > #ifdef CONFIG_CRASH_CORE If set to false it does io, if set to true it does dma. I would prefer passing an accessor function pointer since that's clearer than true/false. > @@ -284,7 +322,7 @@ static int fw_cfg_do_platform_probe(struct > platform_device *pdev)
[PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
Modify fw_cfg_read_blob() to use DMA if the device supports it. Return errors, because the operation may fail. So far, only one call in fw_cfg_register_dir_entries() is using kmalloc'ed buf and is thus clearly eligible to DMA read. Initially, I didn't implement DMA read to speed up boot time, but as a first step before introducing DMA write (since read operations were already presents). Even more, I didn't realize fw-cfg entries were being read by the kernel during boot by default. But actally fw-cfg entries are being populated during module probe. I knew DMA improved a lot bios boot time (the main reason the DMA interface was added afaik). Let see the time it would take to read the whole ACPI tables (128kb allocated) # time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw - with DMA: sys 0m0.003s - without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel boot to populate sysfs qemu_fw_cfg directory, and it is quite small (1-2kb). Since it does not expose itself, in order to measure the time it takes to read such small file, I took a comparable sized file of 2048 bytes and exposed it (-fw_cfg test,file=file with a modified read_raw enabling DMA) # perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw >/dev/null - with DMA: 0.636037 task-clock (msec) #0.141 CPUs utilized ( +- 1.19% ) - without DMA: 6.430128 task-clock (msec) #0.622 CPUs utilized ( +- 0.22% ) That's a few msec saved during boot by enabling DMA read (the gain would be more substantial if other & bigger fw-cfg entries are read by others from sysfs, unfortunately, it's not clear if we can always enable DMA there) Signed-off-by: Marc-André Lureau--- drivers/firmware/qemu_fw_cfg.c | 61 ++ 1 file changed, 50 insertions(+), 11 deletions(-) diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c index 3015e77aebca..94df57e9be66 100644 --- a/drivers/firmware/qemu_fw_cfg.c +++ b/drivers/firmware/qemu_fw_cfg.c @@ -124,12 +124,47 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control) return ret; } +/* with acpi & dev locks taken */ +static ssize_t fw_cfg_read_blob_dma(u16 key, + void *buf, loff_t pos, size_t count) +{ + ssize_t ret; + + if (pos == 0) { + ret = fw_cfg_dma_transfer(buf, count, key << 16 + | FW_CFG_DMA_CTL_SELECT + | FW_CFG_DMA_CTL_READ); + } else { + fw_cfg_sel_endianness(key); + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP); + if (ret < 0) + return ret; + ret = fw_cfg_dma_transfer(buf, count, + FW_CFG_DMA_CTL_READ); + } + + return ret; +} + +/* with acpi & dev locks taken */ +static ssize_t fw_cfg_read_blob_io(u16 key, + void *buf, loff_t pos, size_t count) +{ + fw_cfg_sel_endianness(key); + while (pos-- > 0) + ioread8(fw_cfg_reg_data); + ioread8_rep(fw_cfg_reg_data, buf, count); + return count; +} + /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */ static ssize_t fw_cfg_read_blob(u16 key, - void *buf, loff_t pos, size_t count) + void *buf, loff_t pos, size_t count, + bool dma) { u32 glk = -1U; acpi_status status; + ssize_t ret; /* If we have ACPI, ensure mutual exclusion against any potential * device access by the firmware, e.g. via AML methods: @@ -143,14 +178,17 @@ static ssize_t fw_cfg_read_blob(u16 key, } mutex_lock(_cfg_dev_lock); - fw_cfg_sel_endianness(key); - while (pos-- > 0) - ioread8(fw_cfg_reg_data); - ioread8_rep(fw_cfg_reg_data, buf, count); + if (dma && fw_cfg_dma_enabled()) { + ret = fw_cfg_read_blob_dma(key, buf, pos, count); + } else { + ret = fw_cfg_read_blob_io(key, buf, pos, count); + } + mutex_unlock(_cfg_dev_lock); acpi_release_global_lock(glk); - return count; + + return ret; } #ifdef CONFIG_CRASH_CORE @@ -284,7 +322,7 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev) /* verify fw_cfg device signature */ if (fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, - 0, FW_CFG_SIG_SIZE) < 0 || + 0, FW_CFG_SIG_SIZE, false) < 0 || memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) { fw_cfg_io_cleanup(); return -ENODEV; @@ -468,7 +506,8 @@ static ssize_t
[PATCH v15 11/11] RFC: fw_cfg: do DMA read operation
Modify fw_cfg_read_blob() to use DMA if the device supports it. Return errors, because the operation may fail. So far, only one call in fw_cfg_register_dir_entries() is using kmalloc'ed buf and is thus clearly eligible to DMA read. Initially, I didn't implement DMA read to speed up boot time, but as a first step before introducing DMA write (since read operations were already presents). Even more, I didn't realize fw-cfg entries were being read by the kernel during boot by default. But actally fw-cfg entries are being populated during module probe. I knew DMA improved a lot bios boot time (the main reason the DMA interface was added afaik). Let see the time it would take to read the whole ACPI tables (128kb allocated) # time cat /sys/firmware/qemu_fw_cfg/by_name/etc/acpi/tables/raw - with DMA: sys 0m0.003s - without DMA (-global fw_cfg.dma_enabled=off): sys 0m7.674s FW_CFG_FILE_DIR (0x19) is the only "file" that is read during kernel boot to populate sysfs qemu_fw_cfg directory, and it is quite small (1-2kb). Since it does not expose itself, in order to measure the time it takes to read such small file, I took a comparable sized file of 2048 bytes and exposed it (-fw_cfg test,file=file with a modified read_raw enabling DMA) # perf stat -r 100 cat /sys/firmware/qemu_fw_cfg/by_name/test/raw >/dev/null - with DMA: 0.636037 task-clock (msec) #0.141 CPUs utilized ( +- 1.19% ) - without DMA: 6.430128 task-clock (msec) #0.622 CPUs utilized ( +- 0.22% ) That's a few msec saved during boot by enabling DMA read (the gain would be more substantial if other & bigger fw-cfg entries are read by others from sysfs, unfortunately, it's not clear if we can always enable DMA there) Signed-off-by: Marc-André Lureau --- drivers/firmware/qemu_fw_cfg.c | 61 ++ 1 file changed, 50 insertions(+), 11 deletions(-) diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c index 3015e77aebca..94df57e9be66 100644 --- a/drivers/firmware/qemu_fw_cfg.c +++ b/drivers/firmware/qemu_fw_cfg.c @@ -124,12 +124,47 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control) return ret; } +/* with acpi & dev locks taken */ +static ssize_t fw_cfg_read_blob_dma(u16 key, + void *buf, loff_t pos, size_t count) +{ + ssize_t ret; + + if (pos == 0) { + ret = fw_cfg_dma_transfer(buf, count, key << 16 + | FW_CFG_DMA_CTL_SELECT + | FW_CFG_DMA_CTL_READ); + } else { + fw_cfg_sel_endianness(key); + ret = fw_cfg_dma_transfer(NULL, pos, FW_CFG_DMA_CTL_SKIP); + if (ret < 0) + return ret; + ret = fw_cfg_dma_transfer(buf, count, + FW_CFG_DMA_CTL_READ); + } + + return ret; +} + +/* with acpi & dev locks taken */ +static ssize_t fw_cfg_read_blob_io(u16 key, + void *buf, loff_t pos, size_t count) +{ + fw_cfg_sel_endianness(key); + while (pos-- > 0) + ioread8(fw_cfg_reg_data); + ioread8_rep(fw_cfg_reg_data, buf, count); + return count; +} + /* read chunk of given fw_cfg blob (caller responsible for sanity-check) */ static ssize_t fw_cfg_read_blob(u16 key, - void *buf, loff_t pos, size_t count) + void *buf, loff_t pos, size_t count, + bool dma) { u32 glk = -1U; acpi_status status; + ssize_t ret; /* If we have ACPI, ensure mutual exclusion against any potential * device access by the firmware, e.g. via AML methods: @@ -143,14 +178,17 @@ static ssize_t fw_cfg_read_blob(u16 key, } mutex_lock(_cfg_dev_lock); - fw_cfg_sel_endianness(key); - while (pos-- > 0) - ioread8(fw_cfg_reg_data); - ioread8_rep(fw_cfg_reg_data, buf, count); + if (dma && fw_cfg_dma_enabled()) { + ret = fw_cfg_read_blob_dma(key, buf, pos, count); + } else { + ret = fw_cfg_read_blob_io(key, buf, pos, count); + } + mutex_unlock(_cfg_dev_lock); acpi_release_global_lock(glk); - return count; + + return ret; } #ifdef CONFIG_CRASH_CORE @@ -284,7 +322,7 @@ static int fw_cfg_do_platform_probe(struct platform_device *pdev) /* verify fw_cfg device signature */ if (fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, - 0, FW_CFG_SIG_SIZE) < 0 || + 0, FW_CFG_SIG_SIZE, false) < 0 || memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) { fw_cfg_io_cleanup(); return -ENODEV; @@ -468,7 +506,8 @@ static ssize_t fw_cfg_sysfs_read_raw(struct file