Volatile Write Cache(VWC) feature is set in nvme_ns_setup() in the
initial time. This feature is related to block device backed, but this
feature is controlled in controller level via Set/Get Features command.
This patch removed dependency between nvme and nvme-ns to manage the VWC
flag value. Also, it open coded the Get Features for VWC to check all
namespaces attached to the controller, and if false detected, return
directly false.
Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
hw/block/nvme-ns.c | 4 ----
hw/block/nvme.c | 15 ++++++++++++---
hw/block/nvme.h | 1 -
3 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 32662230130b..c403cd36b6bd 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -89,10 +89,6 @@ static int nvme_ns_init_blk(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
return -1;
}
- if (blk_enable_write_cache(ns->blkconf.blk)) {
- n->features.vwc = 0x1;
- }
-
return 0;
}
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index cf0fe28fe6eb..b2a9c48a9d81 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3033,6 +3033,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
uint16_t iv;
NvmeNamespace *ns;
+ int i;
static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
[NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
@@ -3108,7 +3109,17 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
result = ns->features.err_rec;
goto out;
case NVME_VOLATILE_WRITE_CACHE:
- result = n->features.vwc;
+ for (i = 1; i <= n->num_namespaces; i++) {
+ ns = nvme_ns(n, i);
+ if (!ns) {
+ continue;
+ }
+
+ result = blk_enable_write_cache(ns->blkconf.blk);
+ if (!result) {
+ break;
+ }
+ }
trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
goto out;
case NVME_ASYNCHRONOUS_EVENT_CONF:
@@ -3271,8 +3282,6 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest *req)
ns->features.err_rec = dw11;
break;
case NVME_VOLATILE_WRITE_CACHE:
- n->features.vwc = dw11 & 0x1;
-
for (i = 1; i <= n->num_namespaces; i++) {
ns = nvme_ns(n, i);
if (!ns) {
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index b7fbcca39d9f..5ba83ee77841 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -117,7 +117,6 @@ typedef struct NvmeFeatureVal {
uint16_t temp_thresh_low;
};
uint32_t async_config;
- uint32_t vwc;
} NvmeFeatureVal;
typedef struct NvmeCtrl {
--
2.17.1
On Jan 17 23:53, Minwoo Im wrote: > Volatile Write Cache(VWC) feature is set in nvme_ns_setup() in the > initial time. This feature is related to block device backed, but this > feature is controlled in controller level via Set/Get Features command. > > This patch removed dependency between nvme and nvme-ns to manage the VWC > flag value. Also, it open coded the Get Features for VWC to check all > namespaces attached to the controller, and if false detected, return > directly false. > > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com> The VWC feature really should be namespace specific. I wonder why they didn't fix that when they added an NSID to the Flush command... Anyway, this is much better. Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
Hi Minwoo,
On Sun, Jan 17, 2021 at 11:53:32PM +0900, Minwoo Im wrote:
> Volatile Write Cache(VWC) feature is set in nvme_ns_setup() in the
> initial time. This feature is related to block device backed, but this
> feature is controlled in controller level via Set/Get Features command.
>
> This patch removed dependency between nvme and nvme-ns to manage the VWC
> flag value. Also, it open coded the Get Features for VWC to check all
> namespaces attached to the controller, and if false detected, return
> directly false.
>
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> ---
> hw/block/nvme-ns.c | 4 ----
> hw/block/nvme.c | 15 ++++++++++++---
> hw/block/nvme.h | 1 -
> 3 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> index 32662230130b..c403cd36b6bd 100644
> --- a/hw/block/nvme-ns.c
> +++ b/hw/block/nvme-ns.c
> @@ -89,10 +89,6 @@ static int nvme_ns_init_blk(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> return -1;
> }
>
> - if (blk_enable_write_cache(ns->blkconf.blk)) {
> - n->features.vwc = 0x1;
> - }
> -
> return 0;
> }
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index cf0fe28fe6eb..b2a9c48a9d81 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -3033,6 +3033,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
> NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
> uint16_t iv;
> NvmeNamespace *ns;
> + int i;
>
> static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
> [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> @@ -3108,7 +3109,17 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
> result = ns->features.err_rec;
> goto out;
> case NVME_VOLATILE_WRITE_CACHE:
> - result = n->features.vwc;
>
> This change breaks the build on ubuntu 16.04, gcc is detecting error
> as below
> ../hw/block/nvme.c: In function ‘nvme_process_sq’:
> ../hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized
> in this function [-Werror=maybe-uninitialized]
> trace_pci_nvme_getfeat_vwcache(result ? "enabled" :
> "disabled");
> ^
> ../hw/block/nvme.c:3150:14: note: ‘result’ was
> declared here
> uint32_t result;
> ^
>GCC version 5.4.0.
>Tested initilizing result variable, build looks good then.
>
>Regards,
>Sai Pavan
>
> + for (i = 1; i <= n->num_namespaces; i++) {
> + ns = nvme_ns(n, i);
> + if (!ns) {
> + continue;
> + }
> +
> + result = blk_enable_write_cache(ns->blkconf.blk);
> + if (!result) {
> + break;
> + }
> + }
> trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> goto out;
> case NVME_ASYNCHRONOUS_EVENT_CONF:
> @@ -3271,8 +3282,6 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest *req)
> ns->features.err_rec = dw11;
> break;
> case NVME_VOLATILE_WRITE_CACHE:
> - n->features.vwc = dw11 & 0x1;
> -
> for (i = 1; i <= n->num_namespaces; i++) {
> ns = nvme_ns(n, i);
> if (!ns) {
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index b7fbcca39d9f..5ba83ee77841 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -117,7 +117,6 @@ typedef struct NvmeFeatureVal {
> uint16_t temp_thresh_low;
> };
> uint32_t async_config;
> - uint32_t vwc;
> } NvmeFeatureVal;
>
> typedef struct NvmeCtrl {
> --
> 2.17.1
>
>
Hi Minwoo,
Please ignore this mail, I see a fix already floating around in the list.
Regards,
Sai Pavan
On Thu, Feb 11, 2021 at 12:15:57PM +0530, Sai Pavan Boddu wrote:
> Hi Minwoo,
>
> On Sun, Jan 17, 2021 at 11:53:32PM +0900, Minwoo Im wrote:
> > Volatile Write Cache(VWC) feature is set in nvme_ns_setup() in the
> > initial time. This feature is related to block device backed, but this
> > feature is controlled in controller level via Set/Get Features command.
> >
> > This patch removed dependency between nvme and nvme-ns to manage the VWC
> > flag value. Also, it open coded the Get Features for VWC to check all
> > namespaces attached to the controller, and if false detected, return
> > directly false.
> >
> > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> > ---
> > hw/block/nvme-ns.c | 4 ----
> > hw/block/nvme.c | 15 ++++++++++++---
> > hw/block/nvme.h | 1 -
> > 3 files changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> > index 32662230130b..c403cd36b6bd 100644
> > --- a/hw/block/nvme-ns.c
> > +++ b/hw/block/nvme-ns.c
> > @@ -89,10 +89,6 @@ static int nvme_ns_init_blk(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> > return -1;
> > }
> >
> > - if (blk_enable_write_cache(ns->blkconf.blk)) {
> > - n->features.vwc = 0x1;
> > - }
> > -
> > return 0;
> > }
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index cf0fe28fe6eb..b2a9c48a9d81 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -3033,6 +3033,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
> > NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
> > uint16_t iv;
> > NvmeNamespace *ns;
> > + int i;
> >
> > static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
> > [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> > @@ -3108,7 +3109,17 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest *req)
> > result = ns->features.err_rec;
> > goto out;
> > case NVME_VOLATILE_WRITE_CACHE:
> > - result = n->features.vwc;
> >
> > This change breaks the build on ubuntu 16.04, gcc is detecting error
> > as below
> > ../hw/block/nvme.c: In function ‘nvme_process_sq’:
> > ../hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized
> > in this function [-Werror=maybe-uninitialized]
> > trace_pci_nvme_getfeat_vwcache(result ? "enabled" :
> > "disabled");
> > ^
> > ../hw/block/nvme.c:3150:14: note: ‘result’ was
> > declared here
> > uint32_t result;
> > ^
> >GCC version 5.4.0.
> >Tested initilizing result variable, build looks good then.
> >
> >Regards,
> >Sai Pavan
> >
> > + for (i = 1; i <= n->num_namespaces; i++) {
> > + ns = nvme_ns(n, i);
> > + if (!ns) {
> > + continue;
> > + }
> > +
> > + result = blk_enable_write_cache(ns->blkconf.blk);
> > + if (!result) {
> > + break;
> > + }
> > + }
> > trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> > goto out;
> > case NVME_ASYNCHRONOUS_EVENT_CONF:
> > @@ -3271,8 +3282,6 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest *req)
> > ns->features.err_rec = dw11;
> > break;
> > case NVME_VOLATILE_WRITE_CACHE:
> > - n->features.vwc = dw11 & 0x1;
> > -
> > for (i = 1; i <= n->num_namespaces; i++) {
> > ns = nvme_ns(n, i);
> > if (!ns) {
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index b7fbcca39d9f..5ba83ee77841 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -117,7 +117,6 @@ typedef struct NvmeFeatureVal {
> > uint16_t temp_thresh_low;
> > };
> > uint32_t async_config;
> > - uint32_t vwc;
> > } NvmeFeatureVal;
> >
> > typedef struct NvmeCtrl {
> > --
> > 2.17.1
> >
> >
>
© 2016 - 2026 Red Hat, Inc.