From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@seagate.com header.s=proofpoint header.b=t/bfuY3l; spf=pass (domain: seagate.com, ip: 67.231.152.68, mailfrom: tyler.j.erickson@seagate.com) Received: from mx0b-00003501.pphosted.com (mx0b-00003501.pphosted.com [67.231.152.68]) by groups.io with SMTP; Tue, 17 Sep 2019 09:47:03 -0700 Received: from pps.filterd (m0075034.ppops.net [127.0.0.1]) by mx0b-00003501.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id x8HGj6tk031932 for ; Tue, 17 Sep 2019 12:47:02 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=seagate.com; h=mime-version : references : in-reply-to : from : date : message-id : subject : to : cc : content-type; s=proofpoint; bh=c3St4bn1IfOWOKLtaaa6XWxxzFMLkjcbkWMKSwuPk8M=; b=t/bfuY3lv0ya/NRPuPW2Dou6OPTGquRJd96VM7FOFZTYTyVgo3nfN58NFNL5jfhRGupg rpH0V5K0QhDlDW5H/NBW4sYuj2GoBiMmdpR0Ts8XOHPIkocTMNAVe7f1I2wc+x1yjuKg tSgSpE0t2es/9B2dmXNhxTuODROkCEIXH2tZW9XYF6hpihEoAIB59zpVLIpzgsssOPvs 7N3HRD6yQx9QQ5ciPSp5oE0FzRZS7TUYanGGclEFt7J3CsRzf4jBFJw8y1kjodmGYu7L h60UfYRVIUYywyjUOnhu6ZeX5EibW1kpISQjxkvkwuVbL4jqTlfzZNqw1YMOg+6RD+sT SA== Authentication-Results: seagate.com; dkim=pass header.d=seagate.com header.s=google Received: from mail-lf1-f69.google.com (mail-lf1-f69.google.com [209.85.167.69]) by mx0b-00003501.pphosted.com with ESMTP id 2v1dtdd7kr-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Tue, 17 Sep 2019 12:47:01 -0400 Received: by mail-lf1-f69.google.com with SMTP id q3so1029090lfc.5 for ; Tue, 17 Sep 2019 09:47:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=seagate.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=c3St4bn1IfOWOKLtaaa6XWxxzFMLkjcbkWMKSwuPk8M=; b=hrWr0aje+ptYn0FYe82FmRSSKJPK9E0jWNTOKFPlphXNzDNXdZyibPS79/j8MYKLJf YP6tCFpFdGXwecMbI9HwwC/smuR/LmIzOvTx7SttcBdKV7+PdnoLrsuwUjjnUnNNiqAP 1kCX/i6bk09+oDuxz/QTvESCjf4ZUtOetLELxDYVDGV1Lk/dn3DYnkBAaiCtnZCsB46r ajI4aOe+W0F+jqu434RldQUOcmG7QU/u/No4N2ZVUfIV0FGIzSV+jnefd7SHECZgX9Jw cQ5n1wDR8wG9bpStKnAt7KtyJS1N1UFly6kw8IzzUBOJkDdByNeBtuFRr9F+P4GZXtEs L1Fg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=c3St4bn1IfOWOKLtaaa6XWxxzFMLkjcbkWMKSwuPk8M=; b=tidosvC7hNfSCIwiLo6eChIbSaKKpG47ymnst25yrzUh1n7eiRtmr+TPXTIAXnVFrw XNMnTmKch8+hwHE2inHr6PiswigY5vLix3874VXPJ5pw6P6GTG/FNMtvEhJXuZPeSH4H lSVg9irsDw+Rc0927FFuRGu+KYfKhTgFOIeLd9QxvOY8kYEDma0sQHlD6IagfDWrOfQ3 jhcCNQE1CmM2vgNj1fLWzVuAs89gaDDEk81zGKMK8c4lIK+4G2/a8FKDBIMg27bvEyhm SDbqTTrrfhH2ZyUvA5Z2LF9KPFMD+59hC5ALUE0COBNchW9xkBSvvK7/mO1r9wMaELNN VRwQ== X-Gm-Message-State: APjAAAWPz4tuTe6FneXURzj/7ec4wovGNLCNN3a+2xO/H+SEiGugAtJh dBRROKA3gX73TlEXcINqQv9WDTJRZoiKmSDoqD/nBLP3obOHvsFhV0T3qR6FTLOFGr4GwO9BDAN pga/znule1qwgow2qPeMSZzbzz+uSJJf2Tg581et1TbsTFDF49nzhivI= X-Received: by 2002:a19:5516:: with SMTP id n22mr2807475lfe.49.1568738819691; Tue, 17 Sep 2019 09:46:59 -0700 (PDT) X-Google-Smtp-Source: APXvYqzFz3W285KjmQJW+HhU+phjyVYgm70LD6oiPDtM8miOvq/KKx9CwNcz5mm04b0B5j6p1fpfQwm+t5xXx2Iu5Qc= X-Received: by 2002:a19:5516:: with SMTP id n22mr2807462lfe.49.1568738819377; Tue, 17 Sep 2019 09:46:59 -0700 (PDT) MIME-Version: 1.0 References: <20190903135457.26560-1-tyler.j.erickson@seagate.com> <20190903135457.26560-2-tyler.j.erickson@seagate.com> In-Reply-To: From: "Tyler J Erickson" Date: Tue, 17 Sep 2019 10:50:07 -0600 Message-ID: Subject: Re: [edk2-devel] [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin commands To: "Wu, Hao A" Cc: "devel@edk2.groups.io" , "Ni, Ray" X-Proofpoint-PolicyRoute: Outbound X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.70,1.0.8 definitions=2019-09-17_08:2019-09-17,2019-09-17 signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 spamscore=0 priorityscore=1501 mlxscore=0 adultscore=0 clxscore=1015 suspectscore=0 phishscore=0 impostorscore=0 malwarescore=0 lowpriorityscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-1908290000 definitions=main-1909170160 X-Proofpoint-Spam-Policy: Default Domain Policy Content-Type: multipart/alternative; boundary="0000000000001320ab0592c278d3" --0000000000001320ab0592c278d3 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Hao, Sorry for the late reply. I tested and can confirm that if the NSID of the command and the provided NamespaceID as an input match, the commands do go through on the EDK2 driver. This is not the case with a driver built into a motherboard that I have here, as it only accepts values for the current NSID and nothing else= . This is ok since I can load the EDK2 driver in place instead. Since this is unclear in the documentation for the passthru function, can we make a documentation change instead? Thanks, -Tyler On Wed, Sep 4, 2019 at 8:07 PM Wu, Hao A wrote: > Hello Tyler, > > > > Some inline comments below. > > > > Best Regards, > > Hao Wu > > > > *From:* Tyler J Erickson [mailto:tyler.j.erickson@seagate.com] > *Sent:* Wednesday, September 04, 2019 11:07 PM > *To:* Wu, Hao A > *Cc:* devel@edk2.groups.io; Ni, Ray > *Subject:* Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other > NSIDs for Admin commands > > > > Hi Hao, > > > > I tried making both the NamespaceID and NSID values the same when callin= g > the passthru function for these admin commands and it didn't work. I > think that is due to another place in the passthru code filtering the > NamespaceId input values on line 517-520. > > The NamespaceId parameter is being checked to make sure it isn't greater > than the number of supported namespaces by the controller and it makes s= ure > it isn't set to (UINT32)-1 (All F's). > > > > > > [Hao]: I think the check on line 517-520 blocks =E2=80=98NamespaceId=E2= =80=99 with value > greater than the number of namespace. But it allows the value to be > MAX_UINT32, which means the command takes effect on all namespaces. > > > > > > I think this is correct since the NamespaceId input is what is discovere= d > when calling the EFI_NVM_EXPRESS_PASS_THRU_GET_NEXT_NAMESPACE function. > This is what I used to get the available namespaces available in the sys= tem. > > > > In my case I'm sending some commands in my application with the NSID set > to UINT32_MAX in order to request controller wide SMART/Health log data > among other things. There are some commands where setting the NSID to > another value may also be useful. For example, DST can use the NSID in t= he > command to change between testing only the controller (0), testing all > namespaces (UINT32_MAX), and a specific namespace. > > > > > > [Hao]: Yes. Just as you said, commands like DST or Get Log Page will hav= e > different target when different Namespace values are provided. > > I think for your case, you can: > > 1. Set both the 'NamespaceId' parameter and 'Nsid' field to 0 if > the target is the controller; > > 2. Set both of them MAX_UINT32 if the target is all the namespaces= ; > > 3. Set both of them to a corresponding ID if the target is a > specific namespace. > > > > As you can see from the current implementation of NvmExpressPassThru() > function, the 'NamespaceId' parameter does not affect the construct of > the submission queue item. > > The 'NamespaceId' parameter only serves as a valid check for the 'Nsid' > field in the EFI_NVM_EXPRESS_COMMAND structure. > > > > > > The modification I made was done where the passthru command's NSID was > actually being checked, which seemed like the best place to add this > exception for admin commands. > > > -Tyler > > > > > > On Tue, Sep 3, 2019 at 9:39 PM Wu, Hao A wrote: > > > -----Original Message----- > > From: Wu, Hao A > > Sent: Wednesday, September 04, 2019 11:39 AM > > To: devel@edk2.groups.io; Tyler Erickson > > Cc: Wu, Hao A; Ni, Ray > > Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs > > for Admin commands > > > > Repost the mail to the list. > > > > Best Regards, > > Hao Wu > > > > -----Original Message----- > > From: Tyler Erickson [mailto:tyler.j.erickson@seagate.com] > > Sent: Tuesday, September 03, 2019 9:55 PM > > To: edk2-devel@lists.01.org > > Cc: Wu, Hao A; Ni, Ray > > Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs > > for Admin commands > > > > Cc: Hao A Wu > > Cc: Ray Ni > > Signed-off-by: Tyler Erickson > > --- > > MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c > > b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c > > index 8e721379466a..78a3c663ded4 100644 > > --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c > > +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c > > @@ -561,7 +561,7 @@ NvmExpressPassThru ( > > Sq =3D Private->SqBuffer[QueueId] + Private->SqTdbl[QueueId].Sqt; > > Cq =3D Private->CqBuffer[QueueId] + Private->CqHdbl[QueueId].Cqh; > > > > - if (Packet->NvmeCmd->Nsid !=3D NamespaceId) { > > + if (Packet->QueueType !=3D NVME_ADMIN_QUEUE && Packet->NvmeCmd- > > >Nsid !=3D NamespaceId) { > > > Hello, > > Per my understanding to the codes, I think the: > > * Input parameter 'NamespaceId' for the PassThru() service > * The 'Nsid' field of the EFI_NVM_EXPRESS_COMMAND > > are of the same meaning. > > Do you think setting these 2 values identical when calling the PassThru(= ) > service can resolve the issue you met? > > Best Regards, > Hao Wu > > > > return EFI_INVALID_PARAMETER; > > } > > > > -- > > 2.17.1 > >=20 > > --0000000000001320ab0592c278d3 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Hao,

Sorry for the=C2=A0late reply.<= /div>
I tested and can confirm that if the NSID of the command and the = provided NamespaceID as an input match, the commands do go through on the E= DK2 driver. This is not the case with a driver built into a motherboard tha= t I have here, as it only accepts values for the current NSID and nothing e= lse. This is ok since I can load the EDK2 driver in place instead.

Since this is unclear in the documentation for the passthr= u function, can we make a documentation change instead?

Thanks,
-Tyler


On Wed, Sep 4, 2019 at 8:07 PM Wu, Hao A <hao.a.wu@intel.com> wrote:

Hello Tyler,

=C2=A0

Some inline comments below.

=C2=A0

Best Regards,

Hao Wu

=C2=A0

From: Tyler J Erickson [mailto:tyler.j.erickson@seagate.com]
Sent: Wednesday, September 04, 2019 11:07 PM
To: Wu, Hao A
Cc: devel= @edk2.groups.io; Ni, Ray
Subject: Re: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs for Admin c= ommands

=C2=A0

Hi Hao,

=C2=A0

I tried making both the NamespaceID and NSID values the same when cal= ling the passthru function= for these admin commands and it didn't work. I think that is due to an= other place in the passthru code fil= tering the NamespaceId input values on line 517-520.

The NamespaceId parameter is being checked to make sure it isn't = greater than the number of supported namespaces by the controller and it ma= kes sure it isn't set to (UINT32)-1 (All F's).=C2=A0<= /p>

=C2=A0

=C2=A0

[Hao]: I think the check on line 517-520 bl= ocks =E2=80=98NamespaceId= =E2=80=99 with value greater than the number of namespace. But it allows the value to be MAX_UINT32, which mean= s the command takes effect on all namespaces.

=C2=A0

=C2=A0

I think this is correct since the NamespaceId input is what is discov= ered when calling the=C2=A0EFI_NVM_EXPRESS_PASS_THRU_GET_NEXT_NAMESPACE fun= ction. This is what I used to get the available namespaces available in the system.

=C2=A0

In my case I'm sending some commands in my appl= ication with the NSID set to UINT32_MAX in order to request controller wide= SMART/Health log data among other things. There are some commands where se= tting the NSID to another value may also be useful. For example, DST can use the NSID in the command to change bet= ween testing only the controller (0), testing all namespaces (UINT32_MAX), = and a specific namespace.

=C2=A0

=C2=A0

[Hao]: Yes. Just as you said, commands like= DST or Get Log Page will have different target when different Namespace va= lues are provided.

I think for your case, you can:

1.=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 Set both the 'NamespaceId' parameter and 'Nsid' field to 0 if the target is the controller;

2.=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 Set both of them MAX_UINT32 if the tar= get is all the namespaces;

3.=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 Set both of them to a corresponding ID= if the target is a specific namespace.

=C2=A0

As you can see from the current implementat= ion of NvmExpressPassThru() function, the 'Na= mespaceId' parameter does not affect the construct of the submis= sion queue item.

The 'NamespaceId' parameter only serves as a valid ch= eck for the 'Nsid' field in the EFI_NVM_EXPRESS_COMMAND structure.

=C2=A0<= u>

=C2=A0

The modification I made was done where the passthru command's NSID was actually being checked, which seeme= d like the best place to add this exception for admin commands.


-Tyler

=C2=A0

=C2=A0

On Tue, Sep 3, 2019 at 9:39 PM Wu, Hao A <hao.a.wu@intel.com>= ; wrote:

> -----Original Mes= sage-----
> From: Wu, Hao A
> Sent: Wednesday, September 04, 2019 11:39 AM
> To: devel@e= dk2.groups.io; Tyler Erickson
> Cc: Wu, Hao A; Ni, Ray
> Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> for Admin commands
>
> Repost the mail to the list.
>
> Best Regards,
> Hao Wu
>
> -----Original Message-----
> From: Tyler Erickson [mailto:tyler.j.erickson@seagate.com]
> Sent: Tuesday, September 03, 2019 9:55 PM
> To: edk2= -devel@lists.01.org
> Cc: Wu, Hao A; Ni, Ray
> Subject: [PATCH v1 1/1] MdeModulePkg/NvmExpressDxe: Allow other NSIDs
> for Admin commands
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <= ray.ni@intel.com>
> Signed-off-by: Tyler Erickson <tyler.j.erickson@seagate.com>
> ---
>=C2=A0 MdeModulePkg/Bus/Pci/NvmExpress= Dxe/NvmExpressPass= thru.c | 2 +-
>=C2=A0 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a= /MdeModulePkg/Bus/Pci/NvmExpressDxe/= NvmExpressPassthru.c
> b/MdeModulePkg/Bus/= Pci/NvmExpressDxe<= /span>/NvmExpressPassthru= .c
> index 8e721379466a..78a3c663ded4 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpress= Dxe/NvmExpressPass= thru.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpress= Dxe/NvmExpressPass= thru.c
> @@ -561,7 +561,7 @@ NvmExpressPassThru (
>=C2=A0 =C2=A0 Sq=C2=A0 =3D Private->SqBuffer[QueueId<= /span>] + Private->SqT= dbl[QueueId= ].Sqt;
>=C2=A0 =C2=A0 Cq=C2=A0 =3D Private->CqBuffer[QueueId<= /span>] + Private->CqH= dbl[QueueId= ].Cqh;
>
> -=C2=A0 if (Packet->NvmeCmd->Ns= id !=3D NamespaceId) { > +=C2=A0 if (Packet->QueueType !=3D NVME_ADMIN_QUEUE && Packet->NvmeCmd-
> >Nsid != =3D NamespaceId) = {


Hello,

Per my understanding to the codes, I think the:

* Input parameter 'N= amespaceId' for the PassThru() service
* The 'Nsid&#= 39; field of the EFI_NVM_EXPRESS_COMMAND

are of the same meaning.

Do you think setting these 2 values identical when calling the PassThru()<= br> service can resolve the issue you met?

Best Regards,
Hao Wu


>=C2=A0 =C2=A0 =C2=A0 return EFI_INVALID_PARAMETER;
>=C2=A0 =C2=A0 }
>
> --
> 2.17.1

--0000000000001320ab0592c278d3--