RE: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
> -Original Message- > From: Keith Busch > Sent: Friday, September 18, 2020 5:10 PM > To: Klaus Jensen > Cc: Dmitry Fomichev ; Fam Zheng > ; Kevin Wolf ; Damien Le Moal > ; qemu-block@nongnu.org; Niklas Cassel > ; Klaus Jensen ; qemu- > de...@nongnu.org; Alistair Francis ; Philippe > Mathieu-Daudé ; Matias Bjorling > > Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result > > On Tue, Sep 15, 2020 at 10:48:49PM +0200, Klaus Jensen wrote: > > On Sep 15 20:44, Dmitry Fomichev wrote: > > > > > > It is not necessary to change it in NST patch since result64 field is not > > > used > > > in that patch. But if you insist, I can move it to NST patch :) > > > > You are right of course, but since it is a change to the "spec" related > > data structures that go into include/block/nvme.h, I think it belongs in > > "hw/block/nvme: Introduce the Namespace Types definitions". > > Just my $.02, unless you're making exernal APIs, I really find it easier > to review internal changes inline with the patches that use it. > > Another example, there are patches in this series that introduce trace > points, but the patch that use them come later. I find that harder to > review since I need to look at two different patches to understand its > value. > I decided to split the trace events to be separate because I felt that it could make the reviewing process simpler. Since the majority of the patches in this series are on the large side, I thought it would be easier to open them in separate sessions rather than to review a large single diff. Let me know if you'd like me to fold the tracing stuff together with the code... DF > I realize this particular patch is implementing a spec feature, but I'd > prefer to see how it's used over of making a round trip to the spec.
Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
On Tue, Sep 15, 2020 at 10:48:49PM +0200, Klaus Jensen wrote: > On Sep 15 20:44, Dmitry Fomichev wrote: > > > > It is not necessary to change it in NST patch since result64 field is not > > used > > in that patch. But if you insist, I can move it to NST patch :) > > You are right of course, but since it is a change to the "spec" related > data structures that go into include/block/nvme.h, I think it belongs in > "hw/block/nvme: Introduce the Namespace Types definitions". Just my $.02, unless you're making exernal APIs, I really find it easier to review internal changes inline with the patches that use it. Another example, there are patches in this series that introduce trace points, but the patch that use them come later. I find that harder to review since I need to look at two different patches to understand its value. I realize this particular patch is implementing a spec feature, but I'd prefer to see how it's used over of making a round trip to the spec.
Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
On Sep 15 20:44, Dmitry Fomichev wrote: > > -Original Message- > > From: Klaus Jensen > > Sent: Tuesday, September 15, 2020 3:56 PM > > To: Dmitry Fomichev > > Cc: Fam Zheng ; Kevin Wolf ; > > Damien Le Moal ; qemu-block@nongnu.org; > > Niklas Cassel ; Klaus Jensen > > ; qemu-de...@nongnu.org; Alistair Francis > > ; Keith Busch ; Philippe > > Mathieu-Daudé ; Matias Bjorling > > > > Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result > > > > On Sep 15 18:56, Dmitry Fomichev wrote: > > > > -Original Message- > > > > From: Klaus Jensen > > > > Sent: Tuesday, September 15, 2020 3:37 AM > > > > To: Dmitry Fomichev > > > > Cc: Keith Busch ; Klaus Jensen > > > > ; Kevin Wolf ; Philippe > > > > Mathieu-Daudé ; Maxim Levitsky > > > > ; Fam Zheng ; Niklas Cassel > > > > ; Damien Le Moal > > ; > > > > qemu-block@nongnu.org; qemu-de...@nongnu.org; Alistair Francis > > > > ; Matias Bjorling > > > > Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result > > > > > > > > On Sep 14 07:14, Dmitry Fomichev wrote: > > > > > From: Ajay Joshi > > > > > > > > > > A new write command, Zone Append, is added as a part of Zoned > > > > > Namespace Command Set. Upon successful completion of this > > command, > > > > > the controller returns the start LBA of the performed write operation > > > > > in cqe.result field. Therefore, the maximum size of this variable > > > > > needs to be changed from 32 to 64 bit, consuming the reserved 32 bit > > > > > field that follows the result in CQE struct. Since the existing > > > > > commands are expected to return a 32 bit LE value, two separate > > > > > variables, result32 and result64, are now kept in a union. > > > > > > > > > > Signed-off-by: Ajay Joshi > > > > > Signed-off-by: Dmitry Fomichev > > > > > Reviewed-by: Klaus Jensen > > > > > > > > I know that I R-b'ed this, but can this be moved to the namespace types > > > > patch, since that is the TP that changes this. > > > > > > You probably meant the ZNS patch since result64 is first used there to > > return > > > ZA starting data LBA. Sure, I can move this stuff to that patch. > > > > > > > No, I actually did mean the NST patch since TP 4056 is the TP that > > "unreserves" dw1 in the CQE. > > It is not necessary to change it in NST patch since result64 field is not used > in that patch. But if you insist, I can move it to NST patch :) You are right of course, but since it is a change to the "spec" related data structures that go into include/block/nvme.h, I think it belongs in "hw/block/nvme: Introduce the Namespace Types definitions". signature.asc Description: PGP signature
RE: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
> -Original Message- > From: Klaus Jensen > Sent: Tuesday, September 15, 2020 3:56 PM > To: Dmitry Fomichev > Cc: Fam Zheng ; Kevin Wolf ; > Damien Le Moal ; qemu-block@nongnu.org; > Niklas Cassel ; Klaus Jensen > ; qemu-de...@nongnu.org; Alistair Francis > ; Keith Busch ; Philippe > Mathieu-Daudé ; Matias Bjorling > > Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result > > On Sep 15 18:56, Dmitry Fomichev wrote: > > > -Original Message- > > > From: Klaus Jensen > > > Sent: Tuesday, September 15, 2020 3:37 AM > > > To: Dmitry Fomichev > > > Cc: Keith Busch ; Klaus Jensen > > > ; Kevin Wolf ; Philippe > > > Mathieu-Daudé ; Maxim Levitsky > > > ; Fam Zheng ; Niklas Cassel > > > ; Damien Le Moal > ; > > > qemu-block@nongnu.org; qemu-de...@nongnu.org; Alistair Francis > > > ; Matias Bjorling > > > Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result > > > > > > On Sep 14 07:14, Dmitry Fomichev wrote: > > > > From: Ajay Joshi > > > > > > > > A new write command, Zone Append, is added as a part of Zoned > > > > Namespace Command Set. Upon successful completion of this > command, > > > > the controller returns the start LBA of the performed write operation > > > > in cqe.result field. Therefore, the maximum size of this variable > > > > needs to be changed from 32 to 64 bit, consuming the reserved 32 bit > > > > field that follows the result in CQE struct. Since the existing > > > > commands are expected to return a 32 bit LE value, two separate > > > > variables, result32 and result64, are now kept in a union. > > > > > > > > Signed-off-by: Ajay Joshi > > > > Signed-off-by: Dmitry Fomichev > > > > Reviewed-by: Klaus Jensen > > > > > > I know that I R-b'ed this, but can this be moved to the namespace types > > > patch, since that is the TP that changes this. > > > > You probably meant the ZNS patch since result64 is first used there to > return > > ZA starting data LBA. Sure, I can move this stuff to that patch. > > > > No, I actually did mean the NST patch since TP 4056 is the TP that > "unreserves" dw1 in the CQE. It is not necessary to change it in NST patch since result64 field is not used in that patch. But if you insist, I can move it to NST patch :)
Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
On Sep 15 18:56, Dmitry Fomichev wrote: > > -Original Message- > > From: Klaus Jensen > > Sent: Tuesday, September 15, 2020 3:37 AM > > To: Dmitry Fomichev > > Cc: Keith Busch ; Klaus Jensen > > ; Kevin Wolf ; Philippe > > Mathieu-Daudé ; Maxim Levitsky > > ; Fam Zheng ; Niklas Cassel > > ; Damien Le Moal ; > > qemu-block@nongnu.org; qemu-de...@nongnu.org; Alistair Francis > > ; Matias Bjorling > > Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result > > > > On Sep 14 07:14, Dmitry Fomichev wrote: > > > From: Ajay Joshi > > > > > > A new write command, Zone Append, is added as a part of Zoned > > > Namespace Command Set. Upon successful completion of this command, > > > the controller returns the start LBA of the performed write operation > > > in cqe.result field. Therefore, the maximum size of this variable > > > needs to be changed from 32 to 64 bit, consuming the reserved 32 bit > > > field that follows the result in CQE struct. Since the existing > > > commands are expected to return a 32 bit LE value, two separate > > > variables, result32 and result64, are now kept in a union. > > > > > > Signed-off-by: Ajay Joshi > > > Signed-off-by: Dmitry Fomichev > > > Reviewed-by: Klaus Jensen > > > > I know that I R-b'ed this, but can this be moved to the namespace types > > patch, since that is the TP that changes this. > > You probably meant the ZNS patch since result64 is first used there to return > ZA starting data LBA. Sure, I can move this stuff to that patch. > No, I actually did mean the NST patch since TP 4056 is the TP that "unreserves" dw1 in the CQE. signature.asc Description: PGP signature
RE: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
> -Original Message- > From: Klaus Jensen > Sent: Tuesday, September 15, 2020 3:37 AM > To: Dmitry Fomichev > Cc: Keith Busch ; Klaus Jensen > ; Kevin Wolf ; Philippe > Mathieu-Daudé ; Maxim Levitsky > ; Fam Zheng ; Niklas Cassel > ; Damien Le Moal ; > qemu-block@nongnu.org; qemu-de...@nongnu.org; Alistair Francis > ; Matias Bjorling > Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result > > On Sep 14 07:14, Dmitry Fomichev wrote: > > From: Ajay Joshi > > > > A new write command, Zone Append, is added as a part of Zoned > > Namespace Command Set. Upon successful completion of this command, > > the controller returns the start LBA of the performed write operation > > in cqe.result field. Therefore, the maximum size of this variable > > needs to be changed from 32 to 64 bit, consuming the reserved 32 bit > > field that follows the result in CQE struct. Since the existing > > commands are expected to return a 32 bit LE value, two separate > > variables, result32 and result64, are now kept in a union. > > > > Signed-off-by: Ajay Joshi > > Signed-off-by: Dmitry Fomichev > > Reviewed-by: Klaus Jensen > > I know that I R-b'ed this, but can this be moved to the namespace types > patch, since that is the TP that changes this. You probably meant the ZNS patch since result64 is first used there to return ZA starting data LBA. Sure, I can move this stuff to that patch. > > Also, I don't think we should touch the tracing in the block driver > since it is not aware of namespace types. Ok
Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
On Sep 14 07:14, Dmitry Fomichev wrote: > From: Ajay Joshi > > A new write command, Zone Append, is added as a part of Zoned > Namespace Command Set. Upon successful completion of this command, > the controller returns the start LBA of the performed write operation > in cqe.result field. Therefore, the maximum size of this variable > needs to be changed from 32 to 64 bit, consuming the reserved 32 bit > field that follows the result in CQE struct. Since the existing > commands are expected to return a 32 bit LE value, two separate > variables, result32 and result64, are now kept in a union. > > Signed-off-by: Ajay Joshi > Signed-off-by: Dmitry Fomichev > Reviewed-by: Klaus Jensen I know that I R-b'ed this, but can this be moved to the namespace types patch, since that is the TP that changes this. Also, I don't think we should touch the tracing in the block driver since it is not aware of namespace types. signature.asc Description: PGP signature
