On Tue, Oct 24, 2017 at 1:18 AM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
> On Sat, Oct 21, 2017 at 03:54:16PM +0900, Minwoo Im wrote:
>> Add support HMB(Host Memory Block) with feature commands(Get Feature, Set
>> Feature).
>> nvme-4.14 tree supports HMB features.
>> This patch will make nvme controller to return 32MiB preferred size of HMB
>> to host via identify command.
>> Set Feature, Get Feature implemented for HMB.
>>
>> Signed-off-by: Minwoo Im <minwoo.im....@gmail.com>
>> ---
>> hw/block/nvme.c | 35 +++++++++++++++++++++++++++++++++++
>> hw/block/nvme.h | 21 ++++++++++++++++++++-
>> 2 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 6071dc1..d351781 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -605,6 +605,23 @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)
>> }
>> }
>>
>> +static uint32_t nvme_get_feature_hmb(NvmeCtrl *n, NvmeCmd *cmd)
>> +{
>> + uint32_t result = n->hmb_flag.flag;
>> + uint64_t prp1 = le64_to_cpu(cmd->prp1);
>> + uint64_t prp2 = le64_to_cpu(cmd->prp2);
>> + NvmeHmbAttr attr = {0, };
>> +
>> + attr.hsize = cpu_to_le32(n->hmb_attr.hsize);
>> + attr.hmdlal = cpu_to_le32(n->hmb_attr.hmdlal);
>> + attr.hmdlau = cpu_to_le32(n->hmb_attr.hmdlau);
>> + attr.hmdlec = cpu_to_le32(n->hmb_attr.hmdlec);
>> +
>> + nvme_dma_read_prp(n, (uint8_t *)&attr, sizeof(attr), prp1, prp2);
>
> This read can fail if prp1/prp2 are invalid. How should the error be
> reported?
either or both prp1 and prp2 failed, there's no error handling in this code.
agreed it needs to be implemented if this code will be sent as a patch.
>
>> +
>> + return result;
>> +}
>> +
>> static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest
>> *req)
>> {
>> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
>> @@ -617,6 +634,9 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
>> *cmd, NvmeRequest *req)
>> case NVME_NUMBER_OF_QUEUES:
>> result = cpu_to_le32((n->num_queues - 1) | ((n->num_queues - 1) <<
>> 16));
>> break;
>> + case NVME_HOST_MEMORY_BUFFER:
>> + result = nvme_get_feature_hmb(n, cmd);
>> + break;
>> default:
>> return NVME_INVALID_FIELD | NVME_DNR;
>> }
>> @@ -625,6 +645,16 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
>> *cmd, NvmeRequest *req)
>> return NVME_SUCCESS;
>> }
>>
>> +static void nvme_set_feature_hmb(NvmeCtrl *n, NvmeCmd *cmd)
>> +{
>> + n->hmb_flag.flag = le32_to_cpu(cmd->cdw11);
>> +
>> + n->hmb_attr.hsize = le32_to_cpu(cmd->cdw12);
>> + n->hmb_attr.hmdlal = le32_to_cpu(cmd->cdw13);
>> + n->hmb_attr.hmdlau = le32_to_cpu(cmd->cdw14);
>> + n->hmb_attr.hmdlec = le32_to_cpu(cmd->cdw15);
>> +}
>> +
>> static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest
>> *req)
>> {
>> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
>> @@ -638,6 +668,9 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeCmd
>> *cmd, NvmeRequest *req)
>> req->cqe.result =
>> cpu_to_le32((n->num_queues - 1) | ((n->num_queues - 1) << 16));
>> break;
>> + case NVME_HOST_MEMORY_BUFFER:
>> + nvme_set_feature_hmb(n, cmd);
>> + break;
>> default:
>> return NVME_INVALID_FIELD | NVME_DNR;
>> }
>> @@ -985,6 +1018,8 @@ static int nvme_init(PCIDevice *pci_dev)
>> id->oacs = cpu_to_le16(0);
>> id->frmw = 7 << 1;
>> id->lpa = 1 << 0;
>> + id->hmpre = 0x2000;
>
> Missing cpu_to_le32()?
agreed hmpre should be set with le32 instead of raw value.
>
>> + id->hmmin = 0x0;
>> id->sqes = (0x6 << 4) | 0x6;
>> id->cqes = (0x4 << 4) | 0x4;
>> id->nn = cpu_to_le32(n->num_namespaces);
>> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
>> index 6aab338..fab748b 100644
>> --- a/hw/block/nvme.h
>> +++ b/hw/block/nvme.h
>> @@ -552,7 +552,10 @@ typedef struct NvmeIdCtrl {
>> uint8_t lpa;
>> uint8_t elpe;
>> uint8_t npss;
>> - uint8_t rsvd511[248];
>> + uint8_t rsvd271[8];
>> + uint32_t hmpre;
>> + uint32_t hmmin;
>> + uint8_t rsvd511[232];
>> uint8_t sqes;
>> uint8_t cqes;
>> uint16_t rsvd515;
>> @@ -623,9 +626,22 @@ enum NvmeFeatureIds {
>> NVME_INTERRUPT_VECTOR_CONF = 0x9,
>> NVME_WRITE_ATOMICITY = 0xa,
>> NVME_ASYNCHRONOUS_EVENT_CONF = 0xb,
>> + NVME_HOST_MEMORY_BUFFER = 0xd,
>> NVME_SOFTWARE_PROGRESS_MARKER = 0x80
>> };
>>
>> +typedef struct NvmeHmbFlag {
>> + uint32_t flag;
>> +} NvmeHmbFlag;
>> +
>> +typedef struct NvmeHmbAttr {
>> + uint32_t hsize;
>> + uint32_t hmdlal;
>> + uint32_t hmdlau;
>> + uint32_t hmdlec;
>> + uint8_t rsvd4095[4080];
>> +} NvmeHmbAttr;
>> +
>> typedef struct NvmeRangeType {
>> uint8_t type;
>> uint8_t attributes;
>> @@ -776,6 +792,9 @@ typedef struct NvmeCtrl {
>> uint32_t cmbloc;
>> uint8_t *cmbuf;
>>
>> + NvmeHmbFlag hmb_flag;
>> + NvmeHmbAttr hmb_attr;
>> +
>> char *serial;
>> NvmeNamespace *namespaces;
>> NvmeSQueue **sq;
>> --
>> 2.7.4
>>
>>
I'll send it again with delicate verification and much deeper considerations.
Thank you for a review for this patch.
