[PATCH 14/16] nvmet: Allow to change 'TSAS' and 'TREQ'

[Hannes Reinecke]

On 8/9/23 12:44, Sagi Grimberg wrote:
> 
> 
> On 8/8/23 19:53, Hannes Reinecke wrote:
>> Allow to change 'TSAS' and 'TREQ' to values specified in the spec.
> 
> I think a little more description is needed here. About the
> rules of how the two interact.
> 
Ok.

>>
>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>> ---
>>   drivers/nvme/target/configfs.c | 36 +++++++++++++++++++++++++++++++---
>>   1 file changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/nvme/target/configfs.c 
>> b/drivers/nvme/target/configfs.c
>> index 7f826ac8b75c..3c2aeb763e22 100644
>> --- a/drivers/nvme/target/configfs.c
>> +++ b/drivers/nvme/target/configfs.c
>> @@ -159,10 +159,15 @@ static const struct nvmet_type_name_map 
>> nvmet_addr_treq[] = {
>>       { NVMF_TREQ_NOT_REQUIRED,    "not required" },
>>   };
>> +static inline u8 nvmet_port_treq(struct nvmet_port *port)
>> +{
>> +    return (port->disc_addr.treq & NVME_TREQ_SECURE_CHANNEL_MASK);
>> +}
> 
> The function name does not match what it does.
> 
nvmet_port_addr_treq() ?

>> +
>>   static ssize_t nvmet_addr_treq_show(struct config_item *item, char 
>> *page)
>>   {
>> -    u8 treq = to_nvmet_port(item)->disc_addr.treq &
>> -        NVME_TREQ_SECURE_CHANNEL_MASK;
>> +    struct nvmet_port *port = to_nvmet_port(item);
>> +    u8 treq = nvmet_port_treq(port);
>>       int i;
>>       for (i = 0; i < ARRAY_SIZE(nvmet_addr_treq); i++) {
>> @@ -174,11 +179,16 @@ static ssize_t nvmet_addr_treq_show(struct 
>> config_item *item, char *page)
>>       return snprintf(page, PAGE_SIZE, "\n");
>>   }
>> +static inline u8 nvmet_port_treq_mask(struct nvmet_port *port)
>> +{
>> +    return (port->disc_addr.treq & ~NVME_TREQ_SECURE_CHANNEL_MASK);
>> +}
> 
> Same here, function name and what it returns seem diverged from each
> other.
> 
nvmet_port_addr_treq_mask() ?

>> +
>>   static ssize_t nvmet_addr_treq_store(struct config_item *item,
>>           const char *page, size_t count)
>>   {
>>       struct nvmet_port *port = to_nvmet_port(item);
>> -    u8 treq = port->disc_addr.treq & ~NVME_TREQ_SECURE_CHANNEL_MASK;
>> +    u8 treq = nvmet_port_treq_mask(port);
>>       int i;
>>       if (nvmet_is_port_enabled(port, __func__))
>> @@ -193,6 +203,15 @@ static ssize_t nvmet_addr_treq_store(struct 
>> config_item *item,
>>       return -EINVAL;
>>   found:
>> +    if (port->disc_addr.trtype == NVMF_TRTYPE_TCP) {
>> +        if (port->disc_addr.tsas.tcp.sectype != 
>> NVMF_TCP_SECTYPE_TLS13) {
>> +            pr_warn("cannot change TREQ when TLS is not enabled\n");
>> +            return -EINVAL;
> 
> So what are you supposed to set in treq without tls?
> 
'not specified' is the only valid choice currrently.

>> +        } else if (nvmet_addr_treq[i].type == NVMF_TREQ_NOT_SPECIFIED) {
>> +            pr_warn("cannot set TREQ to 'not specified' when TLS is 
>> enabled\n");
>> +            return -EINVAL;
>> +        }
>> +    }
>>       treq |= nvmet_addr_treq[i].type;
>>       port->disc_addr.treq = treq;
>>       return count;
>> @@ -371,6 +390,7 @@ static ssize_t nvmet_addr_tsas_store(struct 
>> config_item *item,
>>           const char *page, size_t count)
>>   {
>>       struct nvmet_port *port = to_nvmet_port(item);
>> +    u8 treq = nvmet_port_treq_mask(port);
>>       int i;
>>       if (nvmet_is_port_enabled(port, __func__))
>> @@ -389,6 +409,16 @@ static ssize_t nvmet_addr_tsas_store(struct 
>> config_item *item,
>>   found:
>>       nvmet_port_init_tsas_tcp(port, nvmet_addr_tsas_tcp[i].type);
>> +    if (nvmet_addr_tsas_tcp[i].type == NVMF_TCP_SECTYPE_TLS13) {
>> +        if (nvmet_port_treq(port) == NVMF_TREQ_NOT_SPECIFIED)
>> +            treq |= NVMF_TREQ_REQUIRED;
>> +        else
>> +            treq |= nvmet_port_treq(port);
>> +    } else {
>> +        /* Set to 'not specified' if TLS is not enabled */
>> +        treq |= NVMF_TREQ_NOT_SPECIFIED;
>> +    }
>> +    port->disc_addr.treq = treq;
> 
> Hmm, so setting tsas modifies treq under the hood.
> Surprising...
> 
> It is not clear to me what is the user supposed to set in configfs.
> Just set tsas to tls1.3 and that's it? any treq setting will not be
> allowed after it? Its a bit confusing.

Yeah, it is. I guess we should disallow setting of the 'treq' bit from
the user, as it's pretty much defined by the TSAS value.

Will be modifying it.

Cheers,

Hannes