Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

io: Add support for address translation from driver physical address to device physical address #322

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

io: Add support for address translation from driver physical address to device physical address #322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/io.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

void metal_io_init(struct metal_io_region *io, void *virt,
const metal_phys_addr_t *physmap, size_t size,
const metal_phys_addr_t *physmap_drv,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break the existing API in the next release. When possible, we try to use a deprecation mechanism.

Here we also have the increase of th function parameters, which seems already quite significant to me. So perhaps it is a good time to rework this API.

I would create a metal_io_config structure that would contain these parameters. Then, create a new function void metal_io_init_region(struct metal_io_region *io, const struct metal_io_config *cfg). Finally, I would update metal_io_init to use metal_io_init_region and tag metal_io_init_region as deprecated.

But perhaps someone else as an alternative to propose?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like good idea. Advantage is we can extend structure as needed but keep API interface the same.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arnopo @tnmysh I've decided to add 2 new members: metal_io_ops::remote_phys_to_offset and metal_io_ops::offset_to_remote_phys.

The changes in libmetal will be similar to this commit: a90bbd8

For context see also this comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to create 2 new ops? Or could you reuse phys_to_offset and offset_to_phys?

Look to me that based on the io address you could decide to apply or not the memory translation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right @arnopo. I can use those.
Therefore, I think this PR can be closed.

This can be added to the backlog, as an improvement, for later.

unsigned int page_shift, unsigned int mem_flags,
const struct metal_io_ops *ops)
{
Expand All @@ -21,6 +22,7 @@ void metal_io_init(struct metal_io_region *io, void *virt,
io->virt = virt;
io->physmap = physmap;
io->size = size;
io->physmap_drv = physmap_drv;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be the device physmap if used on driver side. to rename (remote_physmap?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference between io->physmap and io->physmap_drv (or new name) should be documented in comment.

io->page_shift = page_shift;
if (page_shift >= sizeof(io->page_mask) * CHAR_BIT)
/* avoid overflow */
Expand Down
44 changes: 44 additions & 0 deletions lib/io.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@
of each of the pages in the I/O
region */
size_t size; /**< size of the I/O region */
const metal_phys_addr_t *physmap_drv;/**< table of base physical address
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is same as above physmap. The difference should be stated in new comment.

of each of the pages in the I/O

Check warning on line 80 in lib/io.h

View workflow job for this annotation

GitHub Actions / compliance review

BLOCK_COMMENT_STYLE 

lib/io.h:80  Block comments use * on subsequent lines
region, for driver */

Check warning on line 81 in lib/io.h

View workflow job for this annotation

GitHub Actions / compliance review

BLOCK_COMMENT_STYLE 

lib/io.h:81  Block comments use a trailing */ on a separate line
unsigned long page_shift; /**< page shift of I/O region */
metal_phys_addr_t page_mask; /**< page mask of I/O region */
unsigned int mem_flags; /**< memory attribute of the
Expand All @@ -90,13 +93,15 @@
* @param[in] virt Virtual address of region.
* @param[in] physmap Array of physical addresses per page.
* @param[in] size Size of region.
* @param[in] physmap_drv Array of physical addresses for driver.
* @param[in] page_shift Log2 of page size (-1 for single page).
* @param[in] mem_flags Memory flags
* @param[in] ops ops
*/
void
metal_io_init(struct metal_io_region *io, void *virt,
const metal_phys_addr_t *physmap, size_t size,
const metal_phys_addr_t *physmap_drv,
unsigned int page_shift, unsigned int mem_flags,
const struct metal_io_ops *ops);

Expand Down Expand Up @@ -196,6 +201,39 @@
return (*io->ops.phys_to_offset)(io, phys);
}

/**
* @brief Convert a driver physical address to a device physical address.
* @param[in] io I/O region handle.
* @param[in] phys_drv Driver physical address
* @return METAL_BAD_PHYS if invalid driver physical address,
* or device physical address
*/
static inline metal_phys_addr_t
metal_io_phys_drv_to_phys(struct metal_io_region *io, metal_phys_addr_t phys_drv)
{
size_t p;
size_t page_cnt = 1;
size_t page_size = io->size;

if (!io->physmap_drv || !io->size) {
return METAL_BAD_PHYS;
}

if (io->page_mask != (metal_phys_addr_t)(-1)) {
page_cnt = (io->size + io->page_mask) >> io->page_shift;
page_size = io->page_mask + 1;
}

for (p = 0; p < page_cnt; p++) {
if (phys_drv >= io->physmap_drv[p] &&
phys_drv < io->physmap_drv[p] + page_size) {
return phys_drv - io->physmap_drv[p] + io->physmap[p];
}
}

return METAL_BAD_PHYS;
}

/**
* @brief Convert a physical address to virtual address.
* @param[in] io Shared memory segment handle.
Expand All @@ -205,6 +243,12 @@
static inline void *
metal_io_phys_to_virt(struct metal_io_region *io, metal_phys_addr_t phys)
{
metal_phys_addr_t tmp_phys = metal_io_phys_drv_to_phys(io, phys);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you could create some regressions. This function is used to return a virtual address based on the physical address, not the remote physical address. This function is used for the buffers, but also the vrings (which use local physmap).

It seems that it would be better to provide a metal_io_ops::phys_to_offset that would point to a metal_io_remote_phys_to_offsetor a platform specific function, ensuring that the same IO region is not used for your vrings and the buffers in the vrings.


if (tmp_phys != METAL_BAD_PHYS) {
phys = tmp_phys;
}

return metal_io_virt(io, metal_io_phys_to_offset(io, phys));
}

Expand Down
Loading