[API-NEXT,1/3] drv: shm: function to query for physical addresses

Message ID 1478798514-20999-2-git-send-email-christophe.milard@linaro.org
State New
Headers show

Commit Message

Christophe Milard Nov. 10, 2016, 5:21 p.m.
The capability "can_getphy" is introduced and tells whether physical
address queries are available.
The function odpdrv_getphy() is added to query for physical address
(from virtual address)

Signed-off-by: Christophe Milard <christophe.milard@linaro.org>

---
 include/odp/drv/spec/shm.h | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

-- 
2.7.4

Comments

Maxim Uvarov Nov. 10, 2016, 7:05 p.m. | #1
On 11/10/16 20:21, Christophe Milard wrote:
> The capability "can_getphy" is introduced and tells whether physical

> address queries are available.

> The function odpdrv_getphy() is added to query for physical address

> (from virtual address)

>

> Signed-off-by: Christophe Milard <christophe.milard@linaro.org>

> ---

>   include/odp/drv/spec/shm.h | 35 +++++++++++++++++++++++++++++++++++

>   1 file changed, 35 insertions(+)

>

> diff --git a/include/odp/drv/spec/shm.h b/include/odp/drv/spec/shm.h

> index ef64f5d..ca3aae2 100644

> --- a/include/odp/drv/spec/shm.h

> +++ b/include/odp/drv/spec/shm.h

> @@ -43,6 +43,12 @@ extern "C" {

>   #define ODPDRV_SHM_LOCK		0x02 /**< Memory shall be locked (no swap) */

>   

>   /**

> + * @typedef odpdrv_phys_addr_t

> + * A physical memory address

> + *

> + */

> +

> +/**

>    * Shared memory block info

>    */

>   typedef struct odpdrv_shm_info_t {

> @@ -75,6 +81,13 @@ typedef struct odpdrv_shm_capability_t {

>   	 * available memory size. */

>   	uint64_t max_align;

>   

> +	/** Can retrieve physical addresses

> +	 *

> +	 * A true (non-zero) value means that odpdrv_getphy() can be

> +	 * used to retrieve physical memory addresses, (as well as

> +	 * the debug function odp_drv_dumpphy() */

> +	int can_getphy;

         intnphys_addr:1;
> +

>   } odpdrv_shm_capability_t;

>   

>   /**

> @@ -220,6 +233,28 @@ int odpdrv_shm_print_all(const char *title);

>   uint64_t odpdrv_shm_to_u64(odpdrv_shm_t hdl);

>   

>   /**

> + * Get physical address from virtual address

> + *

> + * @addr       virtual address.

> + * @return     Physicall address or NULL

> + *

> + * @note This routine will work only if capability "can_getphy" is true.

> + */

> +odpdrv_phys_addr_t odpdrv_getphy(const void *addr);


naming is bad. By 'phy' I used to see physics layer chip, not physical 
memory.
Something similar to linux kernel might be:

odpdrv_virt_to_phys()
odpdrv_phys_to_virt()


and (I don't know how writes done from user space), but probably you 
need odp version of ioremap()


> +

> +/**

> + * Print physical address mapping

> + *

print where?
> + * @addr       virtual address.

what is len?

> + * @return     Physicall address or NULL

> + *


it's void, no return.

> + * @note This routine will work only if capability "can_getphy" is true.

> + * @note This routine is intended to be used for diagnostic purposes,

> + * and uses ODP_DBG printouts

> + */

> +void odpdrv_dumpphy(void *addr, uint64_t len);


Maybe odpdrv_memmap_print()? to print current mappings by driver?

should driver handle be passed as argument?

> +

> +/**

>    * @}

>    */

>
Francois Ozog Nov. 10, 2016, 7:35 p.m. | #2
On 10 November 2016 at 20:05, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:

> On 11/10/16 20:21, Christophe Milard wrote:

>

>> The capability "can_getphy" is introduced and tells whether physical

>> address queries are available.

>> The function odpdrv_getphy() is added to query for physical address

>> (from virtual address)

>>

>> Signed-off-by: Christophe Milard <christophe.milard@linaro.org>

>> ---

>>   include/odp/drv/spec/shm.h | 35 +++++++++++++++++++++++++++++++++++

>>   1 file changed, 35 insertions(+)

>>

>> diff --git a/include/odp/drv/spec/shm.h b/include/odp/drv/spec/shm.h

>> index ef64f5d..ca3aae2 100644

>> --- a/include/odp/drv/spec/shm.h

>> +++ b/include/odp/drv/spec/shm.h

>> @@ -43,6 +43,12 @@ extern "C" {

>>   #define ODPDRV_SHM_LOCK               0x02 /**< Memory shall be locked

>> (no swap) */

>>     /**

>> + * @typedef odpdrv_phys_addr_t

>> + * A physical memory address

>> + *

>> + */

>> +

>> +/**

>>    * Shared memory block info

>>    */

>>   typedef struct odpdrv_shm_info_t {

>> @@ -75,6 +81,13 @@ typedef struct odpdrv_shm_capability_t {

>>          * available memory size. */

>>         uint64_t max_align;

>>   +     /** Can retrieve physical addresses

>> +        *

>> +        * A true (non-zero) value means that odpdrv_getphy() can be

>> +        * used to retrieve physical memory addresses, (as well as

>> +        * the debug function odp_drv_dumpphy() */

>> +       int can_getphy;

>>

>         intnphys_addr:1;

>

>> +

>>   } odpdrv_shm_capability_t;

>>     /**

>> @@ -220,6 +233,28 @@ int odpdrv_shm_print_all(const char *title);

>>   uint64_t odpdrv_shm_to_u64(odpdrv_shm_t hdl);

>>     /**

>> + * Get physical address from virtual address

>> + *

>> + * @addr       virtual address.

>> + * @return     Physicall address or NULL

>> + *

>> + * @note This routine will work only if capability "can_getphy" is true.

>> + */

>> +odpdrv_phys_addr_t odpdrv_getphy(const void *addr);

>>

>

> naming is bad. By 'phy' I used to see physics layer chip, not physical

> memory.

> Something similar to linux kernel might be:

>

> odpdrv_virt_to_phys()

> odpdrv_phys_to_virt()

>

>

> and (I don't know how writes done from user space), but probably you need

> odp version of ioremap()

>

> [FF]: I agree for the naming. "physical memory" is then handled through

either UIO or VFIO frameworks which deal with IO particularities, we should
only use APIs from them (ioremap is a kernel API). Drivers may have to
implement one or the other or both physical access methods. Those
frameworks provide required functions to prepare DMAs properly (on some
architectures "DMA from device" and "DMA to device" require preparation
even in bare metal). VFIO is the preferred method because it is secure but
it is not yet widely available. For instance, DPDK virtio-net guest and
host drivers are using uio. vfio support is embryonic and provides poor
performance at the moment.

>

> +

>> +/**

>> + * Print physical address mapping

>> + *

>>

> print where?

>

>> + * @addr       virtual address.

>>

> what is len?

>

> + * @return     Physicall address or NULL

>> + *

>>

>

> it's void, no return.

>

> + * @note This routine will work only if capability "can_getphy" is true.

>> + * @note This routine is intended to be used for diagnostic purposes,

>> + * and uses ODP_DBG printouts

>> + */

>> +void odpdrv_dumpphy(void *addr, uint64_t len);

>>

>

> Maybe odpdrv_memmap_print()? to print current mappings by driver?

>

> should driver handle be passed as argument?

>

>

> +

>> +/**

>>    * @}

>>    */

>>

>>

>

>



-- 
[image: Linaro] <http://www.linaro.org/>
François-Frédéric Ozog | *Director Linaro Networking Group*
T: +33.67221.6485
francois.ozog@linaro.org | Skype: ffozog
Christophe Milard Nov. 11, 2016, 1 p.m. | #3
On 10 November 2016 at 20:05, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> On 11/10/16 20:21, Christophe Milard wrote:

>>

>> The capability "can_getphy" is introduced and tells whether physical

>> address queries are available.

>> The function odpdrv_getphy() is added to query for physical address

>> (from virtual address)

>>

>> Signed-off-by: Christophe Milard <christophe.milard@linaro.org>

>> ---

>>   include/odp/drv/spec/shm.h | 35 +++++++++++++++++++++++++++++++++++

>>   1 file changed, 35 insertions(+)

>>

>> diff --git a/include/odp/drv/spec/shm.h b/include/odp/drv/spec/shm.h

>> index ef64f5d..ca3aae2 100644

>> --- a/include/odp/drv/spec/shm.h

>> +++ b/include/odp/drv/spec/shm.h

>> @@ -43,6 +43,12 @@ extern "C" {

>>   #define ODPDRV_SHM_LOCK               0x02 /**< Memory shall be locked

>> (no swap) */

>>     /**

>> + * @typedef odpdrv_phys_addr_t

>> + * A physical memory address

>> + *

>> + */

>> +

>> +/**

>>    * Shared memory block info

>>    */

>>   typedef struct odpdrv_shm_info_t {

>> @@ -75,6 +81,13 @@ typedef struct odpdrv_shm_capability_t {

>>          * available memory size. */

>>         uint64_t max_align;

>>   +     /** Can retrieve physical addresses

>> +        *

>> +        * A true (non-zero) value means that odpdrv_getphy() can be

>> +        * used to retrieve physical memory addresses, (as well as

>> +        * the debug function odp_drv_dumpphy() */

>> +       int can_getphy;

>

>         intnphys_addr:1;


yes, of course =>V2

>>

>> +

>>   } odpdrv_shm_capability_t;

>>     /**

>> @@ -220,6 +233,28 @@ int odpdrv_shm_print_all(const char *title);

>>   uint64_t odpdrv_shm_to_u64(odpdrv_shm_t hdl);

>>     /**

>> + * Get physical address from virtual address

>> + *

>> + * @addr       virtual address.

>> + * @return     Physicall address or NULL

>> + *

>> + * @note This routine will work only if capability "can_getphy" is true.

>> + */

>> +odpdrv_phys_addr_t odpdrv_getphy(const void *addr);

>

>

> naming is bad. By 'phy' I used to see physics layer chip, not physical

> memory.

> Something similar to linux kernel might be:

>

> odpdrv_virt_to_phys()


OK => V2
> odpdrv_phys_to_virt()


does not exist yet. no real need as far as I can see.

>

>

> and (I don't know how writes done from user space), but probably you need

> odp version of ioremap()

>

>

>> +

>> +/**

>> + * Print physical address mapping

>> + *

>

> print where?


It is said is the descritpion body:uses ODP_DBG
I will add --enable-debug-print required too

>>

>> + * @addr       virtual address.

>

> what is len?


copy/paste error thanks for the pick =>V2

>

>> + * @return     Physicall address or NULL

>> + *

>

>

> it's void, no return.


copy/paste error thanks for the pick =>V2

>

>> + * @note This routine will work only if capability "can_getphy" is true.

>> + * @note This routine is intended to be used for diagnostic purposes,

>> + * and uses ODP_DBG printouts

>> + */

>> +void odpdrv_dumpphy(void *addr, uint64_t len);

>

>

> Maybe odpdrv_memmap_print()? to print current mappings by driver?


OK =>V2

>

> should driver handle be passed as argument?


I don't think so: first, it is less flexible: if a driver for any
reason would need to allocate a large memory area and worry about its
physycal fragmentation, then it would not be happy with just handles.
If a driver starts to split a page in different fragments, then that
single handle would not be very nice to deal with.
Drivers will care much more about the address than the handle (I guess
even that some drivers may just ignore the handle completely as memory
can be released by address (when single VA flag is set))
The excelent name you suggested, virt_to_phy, would loose some of ots
prestige! ;-)

Thanks for your comments!

Christophe.
>

>

>> +

>> +/**

>>    * @}

>>    */

>>

>

>

Patch

diff --git a/include/odp/drv/spec/shm.h b/include/odp/drv/spec/shm.h
index ef64f5d..ca3aae2 100644
--- a/include/odp/drv/spec/shm.h
+++ b/include/odp/drv/spec/shm.h
@@ -43,6 +43,12 @@  extern "C" {
 #define ODPDRV_SHM_LOCK		0x02 /**< Memory shall be locked (no swap) */
 
 /**
+ * @typedef odpdrv_phys_addr_t
+ * A physical memory address
+ *
+ */
+
+/**
  * Shared memory block info
  */
 typedef struct odpdrv_shm_info_t {
@@ -75,6 +81,13 @@  typedef struct odpdrv_shm_capability_t {
 	 * available memory size. */
 	uint64_t max_align;
 
+	/** Can retrieve physical addresses
+	 *
+	 * A true (non-zero) value means that odpdrv_getphy() can be
+	 * used to retrieve physical memory addresses, (as well as
+	 * the debug function odp_drv_dumpphy() */
+	int can_getphy;
+
 } odpdrv_shm_capability_t;
 
 /**
@@ -220,6 +233,28 @@  int odpdrv_shm_print_all(const char *title);
 uint64_t odpdrv_shm_to_u64(odpdrv_shm_t hdl);
 
 /**
+ * Get physical address from virtual address
+ *
+ * @addr       virtual address.
+ * @return     Physicall address or NULL
+ *
+ * @note This routine will work only if capability "can_getphy" is true.
+ */
+odpdrv_phys_addr_t odpdrv_getphy(const void *addr);
+
+/**
+ * Print physical address mapping
+ *
+ * @addr       virtual address.
+ * @return     Physicall address or NULL
+ *
+ * @note This routine will work only if capability "can_getphy" is true.
+ * @note This routine is intended to be used for diagnostic purposes,
+ * and uses ODP_DBG printouts
+ */
+void odpdrv_dumpphy(void *addr, uint64_t len);
+
+/**
  * @}
  */