Re: [Qemu-devel] [PATCH 16/16] nvme: support multiple namespaces

[Ross Lagerwall]

On 7/5/19 8:23 AM, Klaus Birkelund Jensen wrote:
> This adds support for multiple namespaces by introducing a new 'nvme-ns'
> device model. The nvme device creates a bus named from the device name
> ('id'). The nvme-ns devices then connect to this and registers
> themselves with the nvme device.
>
> This changes how an nvme device is created. Example with two namespaces:
>
>    -drive file=nvme0n1.img,if=none,id=disk1
>    -drive file=nvme0n2.img,if=none,id=disk2
>    -device nvme,serial=deadbeef,id=nvme0
>    -device nvme-ns,drive=disk1,bus=nvme0,nsid=1
>    -device nvme-ns,drive=disk2,bus=nvme0,nsid=2
>
> A maximum of 256 namespaces can be configured.
snip
>   static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
>   {
> +    NvmeNamespace *ns = req->ns;
> +
>       uint32_t dw10 = le32_to_cpu(cmd->cdw10);
>       uint32_t dw11 = le32_to_cpu(cmd->cdw11);
>       uint32_t result;
> @@ -1464,7 +1474,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd 
> *cmd, NvmeRequest *req)
>           result = cpu_to_le32(n->features.err_rec);
>           break;
>       case NVME_VOLATILE_WRITE_CACHE:
> -        result = blk_enable_write_cache(n->conf.blk);
> +        result = blk_enable_write_cache(ns->conf.blk);
>           trace_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
>           break;

I tried this patch series by installing Windows with a single NVME 
controller having two namespaces. QEMU crashed in get_feature / 
NVME_VOLATILE_WRITE_CACHE because req->ns was NULL.

nvme_get_feature / nvme_set_feature look wrong to me since I can't see 
how req->ns would have been set. Should they have similar code to 
nvme_io_cmd to set req->ns from cmd->nsid?

After working around this issue everything else seemed to be working 
well. Thanks for your work on this patch series.

Thanks,
--
Ross Lagerwall