| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87zfvm2wxk.fsf@kurt.kurt.home>
Date: Tue, 27 Feb 2024 08:22:47 +0100
From: Kurt Kanzenbach <kurt@...utronix.de>
To: Paul Menzel <pmenzel@...gen.mpg.de>
Cc: Jesse Brandeburg <jesse.brandeburg@...el.com>, Tony Nguyen
<anthony.l.nguyen@...el.com>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Vinicius Costa Gomes
<vinicius.gomes@...el.com>, netdev@...r.kernel.org, Sebastian Andrzej
Siewior <bigeasy@...utronix.de>, intel-wired-lan@...ts.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH iwl-next] igc: Add MQPRIO offload support
Hello Paul,
On Mon Feb 26 2024, Paul Menzel wrote:
> Dear Kurt,
>
>
> Thank you for the patch. Some nits from my side.
>
> Am 26.02.24 um 09:51 schrieb Kurt Kanzenbach:
>> Add support for offloading MQPRIO. The hardware has four priorities as well
>> as four queues. Each queue must be a assigned with a unique priority.
>>
>> However, the priorities are only considered in TSN Tx mode. There are two
>> TSN Tx modes. In case of MQPRIO the Qbv capability is not
>> required. Therefore, use the legacy TSN Tx mode, which performs strict
>> priority arbitration.
>
> You could reflow this paragraph. The second line breaks too early.
Indeed, it does.
>
>> Example for mqprio with hardware offload:
>>
>> |tc qdisc replace dev ${INTERFACE} handle 100 parent root mqprio num_tc 4 \
>> | map 0 0 0 0 0 1 2 3 0 0 0 0 0 0 0 0 \
>> | queues 1@0 1@1 1@2 1@3 \
>> | hw 1
>
> Thank you for the example. How can you check that it actually works? No
> errors are shown?
When using tc for setting up Qdisc(s) the command just returns without
any messages. In case there's an error the user will see messages
configured below with `extack`.
>
> Also, could you please mention the datasheet name, revision and section,
> and on what exact device you tested this?
Sure.
>
>> Signed-off-by: Kurt Kanzenbach <kurt@...utronix.de>
>> ---
>> drivers/net/ethernet/intel/igc/igc.h | 10 +++-
>> drivers/net/ethernet/intel/igc/igc_defines.h | 9 ++++
>> drivers/net/ethernet/intel/igc/igc_main.c | 69 +++++++++++++++++++++++++++
>> drivers/net/ethernet/intel/igc/igc_regs.h | 2 +
>> drivers/net/ethernet/intel/igc/igc_tsn.c | 71 +++++++++++++++++++++++++++-
>> 5 files changed, 157 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
>> index 45430e246e9c..c95919f085eb 100644
>> --- a/drivers/net/ethernet/intel/igc/igc.h
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -227,6 +227,10 @@ struct igc_adapter {
>> */
>> spinlock_t qbv_tx_lock;
>>
>> + bool strict_priority_enable;
>> + u8 num_tc;
>> + u16 queue_per_tc[IGC_MAX_TX_QUEUES];
>> +
>> /* OS defined structs */
>> struct pci_dev *pdev;
>> /* lock for statistics */
>> @@ -343,9 +347,11 @@ extern char igc_driver_name[];
>> #define IGC_FLAG_RX_LEGACY BIT(16)
>> #define IGC_FLAG_TSN_QBV_ENABLED BIT(17)
>> #define IGC_FLAG_TSN_QAV_ENABLED BIT(18)
>> +#define IGC_FLAG_TSN_LEGACY_ENABLED BIT(19)
>>
>> -#define IGC_FLAG_TSN_ANY_ENABLED \
>> - (IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED)
>> +#define IGC_FLAG_TSN_ANY_ENABLED \
>> + (IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED | \
>> + IGC_FLAG_TSN_LEGACY_ENABLED)
>>
>> #define IGC_FLAG_RSS_FIELD_IPV4_UDP BIT(6)
>> #define IGC_FLAG_RSS_FIELD_IPV6_UDP BIT(7)
>> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
>> index 5f92b3c7c3d4..73502a0b4df7 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
>> @@ -547,6 +547,15 @@
>>
>> #define IGC_MAX_SR_QUEUES 2
>>
>> +#define IGC_TXARB_TXQ_PRIO_0_SHIFT 0
>> +#define IGC_TXARB_TXQ_PRIO_1_SHIFT 2
>> +#define IGC_TXARB_TXQ_PRIO_2_SHIFT 4
>> +#define IGC_TXARB_TXQ_PRIO_3_SHIFT 6
>> +#define IGC_TXARB_TXQ_PRIO_0_MASK GENMASK(1, 0)
>> +#define IGC_TXARB_TXQ_PRIO_1_MASK GENMASK(3, 2)
>> +#define IGC_TXARB_TXQ_PRIO_2_MASK GENMASK(5, 4)
>> +#define IGC_TXARB_TXQ_PRIO_3_MASK GENMASK(7, 6)
>> +
>> /* Receive Checksum Control */
>> #define IGC_RXCSUM_CRCOFL 0x00000800 /* CRC32 offload enable */
>> #define IGC_RXCSUM_PCSD 0x00002000 /* packet checksum disabled */
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index ba8d3fe186ae..c5d3cc3c4fa9 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -6424,6 +6424,13 @@ static int igc_tc_query_caps(struct igc_adapter *adapter,
>> struct igc_hw *hw = &adapter->hw;
>>
>> switch (base->type) {
>> + case TC_SETUP_QDISC_MQPRIO: {
>> + struct tc_mqprio_caps *caps = base->caps;
>> +
>> + caps->validate_queue_counts = true;
>> +
>> + return 0;
>> + }
>> case TC_SETUP_QDISC_TAPRIO: {
>> struct tc_taprio_caps *caps = base->caps;
>>
>> @@ -6441,6 +6448,65 @@ static int igc_tc_query_caps(struct igc_adapter *adapter,
>> }
>> }
>>
>> +static void igc_save_mqprio_params(struct igc_adapter *adapter, u8 num_tc,
>> + u16 *offset)
>> +{
>> + int i;
>> +
>> + adapter->strict_priority_enable = true;
>> + adapter->num_tc = num_tc;
>> +
>> + for (i = 0; i < num_tc; i++)
>> + adapter->queue_per_tc[i] = offset[i];
>> +}
>> +
>> +static int igc_tsn_enable_mqprio(struct igc_adapter *adapter,
>> + struct tc_mqprio_qopt_offload *mqprio)
>> +{
>> + struct igc_hw *hw = &adapter->hw;
>> + int i;
>> +
>> + if (hw->mac.type != igc_i225)
>> + return -EOPNOTSUPP;
>> +
>> + if (!mqprio->qopt.num_tc) {
>> + adapter->strict_priority_enable = false;
>> + goto apply;
>> + }
>> +
>> + /* There are as many TCs as Tx queues. */
>> + if (mqprio->qopt.num_tc != adapter->num_tx_queues) {
>> + NL_SET_ERR_MSG_FMT_MOD(mqprio->extack,
>> + "Only %d traffic classes supported",
>> + adapter->num_tx_queues);
>
> I’d also log `mqprio->qopt.num_tc`.
num_tc is provided by the user.
>
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + /* Only one queue per TC is supported. */
>> + for (i = 0; i < mqprio->qopt.num_tc; i++) {
>> + if (mqprio->qopt.count[i] != 1) {
>> + NL_SET_ERR_MSG_MOD(mqprio->extack,
>> + "Only one queue per TC supported");
>
> I’d also log the i and the count value.
Oki.
>
>> + return -EOPNOTSUPP;
>> + }
>> + }
>> +
>> + /* Preemption is not supported yet. */
>> + if (mqprio->preemptible_tcs) {
>> + NL_SET_ERR_MSG_MOD(mqprio->extack,
>> + "Preemption is not supported yet");
>> + return -EOPNOTSUPP;
>> + }
>
> Please mention this in the commit message, maybe also included, if a
> patch is coming soon, or what would need to be done to implement it.
Oki. At some point i'd like to see FPE implemented for igc :-).
Thanks,
Kurt
Download attachment "signature.asc" of type "application/pgp-signature" (862 bytes)
Powered by blists - more mailing lists