Page MenuHomeFreeBSD
Differential D35487

iommu_gas: eliminate unneeded wrappers
ClosedPublic
Actions

Authored by alc on Jun 14 2022, 6:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 4, 10:01 PM
Unknown Object (File)
Jan 27 2025, 12:16 PM
Unknown Object (File)
Jan 27 2025, 12:11 PM
Unknown Object (File)
Jan 26 2025, 8:30 PM
Unknown Object (File)
Jan 18 2025, 5:29 PM
Unknown Object (File)
Jan 8 2025, 2:34 AM
Unknown Object (File)
Dec 11 2024, 7:55 AM
Unknown Object (File)
Dec 4 2024, 3:33 PM
View All 86 Files
Subscribers
imp

Details

Reviewers
dougm
kib
br
Commits
rGa408ed1c928a: iommu_gas: Eliminate unnecessary wrappers
rGfc2e4f15a904: iommu_gas: Eliminate unnecessary wrappers
Summary

Eliminate trivial wrappers for several iommu_gas functions. These wrappers serve no functional purpose.

Test Plan

Tested with netperf on amd64. On arm64, a kernel with the IOMMU option compiles.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

alc requested review of this revision.Jun 14 2022, 6:56 PM
alc created this revision.
alc added a reviewer: br.Jun 14 2022, 6:58 PM
br added a comment.Jun 14 2022, 7:09 PM

The idea of these wrappers was to abstract function names, i.e. make them looking generic by removing name "gas". I'm not sure what "gas" stands for, but there is no such thing on arm64.

br added a comment.Jun 14 2022, 7:12 PM

so with this I can use iommu_map() in my ARM Komeda display controller driver (part of cheribsd), not confusing anyone with 'gas'

sys/dev/drm/komeda/komeda_plane.c:      error = iommu_map(iodom,
alc added a comment.Jun 14 2022, 7:21 PM
In D35487#804780, @br wrote:

The idea of these wrappers was to abstract function names, i.e. make them looking generic by removing name "gas". I'm not sure what "gas" stands for, but there is no such thing on arm64.

I believe that it stood for "guest address space".

I also considered renaming iommu_gas_map, etc. to iommu_map, etc., but that entailed more changes. Nonetheless, I would be happy to take that approach.

br accepted this revision.Jun 14 2022, 8:08 PM
In D35487#804790, @alc wrote:
In D35487#804780, @br wrote:

The idea of these wrappers was to abstract function names, i.e. make them looking generic by removing name "gas". I'm not sure what "gas" stands for, but there is no such thing on arm64.

I believe that it stood for "guest address space".

I also considered renaming iommu_gas_map, etc. to iommu_map, etc., but that entailed more changes. Nonetheless, I would be happy to take that approach.

as you wish, I am more or less fine with 'guest address space' since it is not amd64-specific. But the approach would be good.

This revision is now accepted and ready to land.Jun 14 2022, 8:08 PM
dougm accepted this revision.Jun 15 2022, 6:31 AM
kib accepted this revision.Jun 15 2022, 2:29 PM
Closed by commit rGfc2e4f15a904: iommu_gas: Eliminate unnecessary wrappers (authored by alc). · Explain WhyJun 17 2022, 6:20 AM
This revision was automatically updated to reflect the committed changes.
alc added a commit: rGfc2e4f15a904: iommu_gas: Eliminate unnecessary wrappers.
Herald added a subscriber: imp. · View Herald TranscriptJun 17 2022, 6:20 AM
dougm added a commit: rGa408ed1c928a: iommu_gas: Eliminate unnecessary wrappers.Jul 6 2022, 5:12 PM

Revision Contents
Changeset List

PathSize
sys/
dev/
iommu/
8 lines
8 lines
41 lines

Diff 106999

View Options

sys/dev/iommu/busdma_iommu.c

Show All 26 Lines
* ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/ */
#include "gve.h" #include "gve.h"
#include "gve_adminq.h" #include "gve_adminq.h"
#include "gve_dqo.h" #include "gve_dqo.h"
#define GVE_DRIVER_VERSION "GVE-FBSD-1.3.2\n" #define GVE_DRIVER_VERSION "GVE-FBSD-1.3.3\n"
#define GVE_VERSION_MAJOR 1 #define GVE_VERSION_MAJOR 1
#define GVE_VERSION_MINOR 3 #define GVE_VERSION_MINOR 3
#define GVE_VERSION_SUB 2 #define GVE_VERSION_SUB 3
#define GVE_DEFAULT_RX_COPYBREAK 256 #define GVE_DEFAULT_RX_COPYBREAK 256
/* Devices supported by this driver. */ /* Devices supported by this driver. */
static struct gve_dev { static struct gve_dev {
uint16_t vendor_id; uint16_t vendor_id;
uint16_t device_id; uint16_t device_id;
const char *name; const char *name;
▲ Show 20 Lines • Show All 206 Lines • ▼ Show 20 Linesgve_adjust_tx_queues(struct gve_priv *priv, uint16_t new_queue_cnt)
} }
priv->tx_cfg.num_queues = new_queue_cnt; priv->tx_cfg.num_queues = new_queue_cnt;
err = gve_up(priv); err = gve_up(priv);
if (err != 0) if (err != 0)
gve_schedule_reset(priv); gve_schedule_reset(priv);
return (err); return (err);
}
int
gve_adjust_ring_sizes(struct gve_priv *priv, uint16_t new_desc_cnt, bool is_rx)
{
int err;
uint16_t prev_desc_cnt;
GVE_IFACE_LOCK_ASSERT(priv->gve_iface_lock);
gve_down(priv);
if (is_rx) {
gve_free_rx_rings(priv, 0, priv->rx_cfg.num_queues);
prev_desc_cnt = priv->rx_desc_cnt;
priv->rx_desc_cnt = new_desc_cnt;
err = gve_alloc_rx_rings(priv, 0, priv->rx_cfg.num_queues);
if (err != 0) {
device_printf(priv->dev,
"Failed to allocate rings. Trying to start back up with previous ring size.");
priv->rx_desc_cnt = prev_desc_cnt;
err = gve_alloc_rx_rings(priv, 0, priv->rx_cfg.num_queues);
}
} else {
gve_free_tx_rings(priv, 0, priv->tx_cfg.num_queues);
prev_desc_cnt = priv->tx_desc_cnt;
priv->tx_desc_cnt = new_desc_cnt;
err = gve_alloc_tx_rings(priv, 0, priv->tx_cfg.num_queues);
if (err != 0) {
device_printf(priv->dev,
"Failed to allocate rings. Trying to start back up with previous ring size.");
priv->tx_desc_cnt = prev_desc_cnt;
err = gve_alloc_tx_rings(priv, 0, priv->tx_cfg.num_queues);
}
}
if (err != 0) {
device_printf(priv->dev, "Failed to allocate rings! Cannot start device back up!");
gve_down(priv);
markjUnsubmitted
Done
Inline Actions

Is this gve_down() call needed, given that we already bought the interface down?

markj: Is this gve_down() call needed, given that we already bought the interface down?
return (err);
}
err = gve_up(priv);
if (err != 0) {
gve_schedule_reset(priv);
return (err);
}
return (0);
} }
static int static int
gve_set_mtu(if_t ifp, uint32_t new_mtu) gve_set_mtu(if_t ifp, uint32_t new_mtu)
{ {
struct gve_priv *priv = if_getsoftc(ifp); struct gve_priv *priv = if_getsoftc(ifp);
const uint32_t max_problem_range = 8227; const uint32_t max_problem_range = 8227;
const uint32_t min_problem_range = 7822; const uint32_t min_problem_range = 7822;
▲ Show 20 Lines • Show All 755 Lines • Show Last 20 Lines
View Options

sys/dev/iommu/iommu.h

Show All 26 Lines
* ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/ */
#include "gve.h" #include "gve.h"
#include "gve_adminq.h" #include "gve_adminq.h"
#include "gve_dqo.h" #include "gve_dqo.h"
#define GVE_DRIVER_VERSION "GVE-FBSD-1.3.2\n" #define GVE_DRIVER_VERSION "GVE-FBSD-1.3.3\n"
#define GVE_VERSION_MAJOR 1 #define GVE_VERSION_MAJOR 1
#define GVE_VERSION_MINOR 3 #define GVE_VERSION_MINOR 3
#define GVE_VERSION_SUB 2 #define GVE_VERSION_SUB 3
#define GVE_DEFAULT_RX_COPYBREAK 256 #define GVE_DEFAULT_RX_COPYBREAK 256
/* Devices supported by this driver. */ /* Devices supported by this driver. */
static struct gve_dev { static struct gve_dev {
uint16_t vendor_id; uint16_t vendor_id;
uint16_t device_id; uint16_t device_id;
const char *name; const char *name;
▲ Show 20 Lines • Show All 206 Lines • ▼ Show 20 Linesgve_adjust_tx_queues(struct gve_priv *priv, uint16_t new_queue_cnt)
} }
priv->tx_cfg.num_queues = new_queue_cnt; priv->tx_cfg.num_queues = new_queue_cnt;
err = gve_up(priv); err = gve_up(priv);
if (err != 0) if (err != 0)
gve_schedule_reset(priv); gve_schedule_reset(priv);
return (err); return (err);
}
int
gve_adjust_ring_sizes(struct gve_priv *priv, uint16_t new_desc_cnt, bool is_rx)
{
int err;
uint16_t prev_desc_cnt;
GVE_IFACE_LOCK_ASSERT(priv->gve_iface_lock);
gve_down(priv);
if (is_rx) {
gve_free_rx_rings(priv, 0, priv->rx_cfg.num_queues);
prev_desc_cnt = priv->rx_desc_cnt;
priv->rx_desc_cnt = new_desc_cnt;
err = gve_alloc_rx_rings(priv, 0, priv->rx_cfg.num_queues);
if (err != 0) {
device_printf(priv->dev,
"Failed to allocate rings. Trying to start back up with previous ring size.");
priv->rx_desc_cnt = prev_desc_cnt;
err = gve_alloc_rx_rings(priv, 0, priv->rx_cfg.num_queues);
}
} else {
gve_free_tx_rings(priv, 0, priv->tx_cfg.num_queues);
prev_desc_cnt = priv->tx_desc_cnt;
priv->tx_desc_cnt = new_desc_cnt;
err = gve_alloc_tx_rings(priv, 0, priv->tx_cfg.num_queues);
if (err != 0) {
device_printf(priv->dev,
"Failed to allocate rings. Trying to start back up with previous ring size.");
priv->tx_desc_cnt = prev_desc_cnt;
err = gve_alloc_tx_rings(priv, 0, priv->tx_cfg.num_queues);
}
}
if (err != 0) {
device_printf(priv->dev, "Failed to allocate rings! Cannot start device back up!");
gve_down(priv);
markjUnsubmitted
Done
Inline Actions

Is this gve_down() call needed, given that we already bought the interface down?

markj: Is this gve_down() call needed, given that we already bought the interface down?
return (err);
}
err = gve_up(priv);
if (err != 0) {
gve_schedule_reset(priv);
return (err);
}
return (0);
} }
static int static int
gve_set_mtu(if_t ifp, uint32_t new_mtu) gve_set_mtu(if_t ifp, uint32_t new_mtu)
{ {
struct gve_priv *priv = if_getsoftc(ifp); struct gve_priv *priv = if_getsoftc(ifp);
const uint32_t max_problem_range = 8227; const uint32_t max_problem_range = 8227;
const uint32_t min_problem_range = 7822; const uint32_t min_problem_range = 7822;
▲ Show 20 Lines • Show All 755 Lines • Show Last 20 Lines
View Options

sys/dev/iommu/iommu_gas.c

Show All 26 Lines
* ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/ */
#include "gve.h" #include "gve.h"
#include "gve_adminq.h" #include "gve_adminq.h"
#include "gve_dqo.h" #include "gve_dqo.h"
#define GVE_DRIVER_VERSION "GVE-FBSD-1.3.2\n" #define GVE_DRIVER_VERSION "GVE-FBSD-1.3.3\n"
#define GVE_VERSION_MAJOR 1 #define GVE_VERSION_MAJOR 1
#define GVE_VERSION_MINOR 3 #define GVE_VERSION_MINOR 3
#define GVE_VERSION_SUB 2 #define GVE_VERSION_SUB 3
#define GVE_DEFAULT_RX_COPYBREAK 256 #define GVE_DEFAULT_RX_COPYBREAK 256
/* Devices supported by this driver. */ /* Devices supported by this driver. */
static struct gve_dev { static struct gve_dev {
uint16_t vendor_id; uint16_t vendor_id;
uint16_t device_id; uint16_t device_id;
const char *name; const char *name;
▲ Show 20 Lines • Show All 206 Lines • ▼ Show 20 Linesgve_adjust_tx_queues(struct gve_priv *priv, uint16_t new_queue_cnt)
} }
priv->tx_cfg.num_queues = new_queue_cnt; priv->tx_cfg.num_queues = new_queue_cnt;
err = gve_up(priv); err = gve_up(priv);
if (err != 0) if (err != 0)
gve_schedule_reset(priv); gve_schedule_reset(priv);
return (err); return (err);
}
int
gve_adjust_ring_sizes(struct gve_priv *priv, uint16_t new_desc_cnt, bool is_rx)
{
int err;
uint16_t prev_desc_cnt;
GVE_IFACE_LOCK_ASSERT(priv->gve_iface_lock);
gve_down(priv);
if (is_rx) {
gve_free_rx_rings(priv, 0, priv->rx_cfg.num_queues);
prev_desc_cnt = priv->rx_desc_cnt;
priv->rx_desc_cnt = new_desc_cnt;
err = gve_alloc_rx_rings(priv, 0, priv->rx_cfg.num_queues);
if (err != 0) {
device_printf(priv->dev,
"Failed to allocate rings. Trying to start back up with previous ring size.");
priv->rx_desc_cnt = prev_desc_cnt;
err = gve_alloc_rx_rings(priv, 0, priv->rx_cfg.num_queues);
}
} else {
gve_free_tx_rings(priv, 0, priv->tx_cfg.num_queues);
prev_desc_cnt = priv->tx_desc_cnt;
priv->tx_desc_cnt = new_desc_cnt;
err = gve_alloc_tx_rings(priv, 0, priv->tx_cfg.num_queues);
if (err != 0) {
device_printf(priv->dev,
"Failed to allocate rings. Trying to start back up with previous ring size.");
priv->tx_desc_cnt = prev_desc_cnt;
err = gve_alloc_tx_rings(priv, 0, priv->tx_cfg.num_queues);
}
}
if (err != 0) {
device_printf(priv->dev, "Failed to allocate rings! Cannot start device back up!");
gve_down(priv);
markjUnsubmitted
Done
Inline Actions

Is this gve_down() call needed, given that we already bought the interface down?

markj: Is this gve_down() call needed, given that we already bought the interface down?
return (err);
}
err = gve_up(priv);
if (err != 0) {
gve_schedule_reset(priv);
return (err);
}
return (0);
} }
static int static int
gve_set_mtu(if_t ifp, uint32_t new_mtu) gve_set_mtu(if_t ifp, uint32_t new_mtu)
{ {
struct gve_priv *priv = if_getsoftc(ifp); struct gve_priv *priv = if_getsoftc(ifp);
const uint32_t max_problem_range = 8227; const uint32_t max_problem_range = 8227;
const uint32_t min_problem_range = 7822; const uint32_t min_problem_range = 7822;
▲ Show 20 Lines • Show All 755 Lines • Show Last 20 Lines