Re: [PATCH v2 1/2] hw/nvme: add support for boot partiotions

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 2826 bytes --]

On Jun  3 10:48, Stefan Hajnoczi wrote:
>On Tue, Jun 01, 2021 at 08:07:48PM +0530, Gollu Appalanaidu wrote:
>> @@ -5546,6 +5665,47 @@ static void nvme_write_bar(NvmeCtrl *n, hwaddr offset, uint64_t data,
>>          NVME_GUEST_ERR(pci_nvme_ub_mmiowr_cmbsz_readonly,
>>                         "invalid write to read only CMBSZ, ignored");
>>          return;
>> +    case 0x44:  /* BPRSEL */
>> +        n->bar.bprsel = data & 0xffffffff;
>> +        size_t bp_len = NVME_BPRSEL_BPRSZ(n->bar.bprsel) * 4 * KiB;
>> +        int64_t bp_offset = NVME_BPRSEL_BPROF(n->bar.bprsel) * 4 * KiB;
>> +        int64_t off = 0;
>> +        struct nvme_bp_read_ctx *ctx;
>> +
>> +        trace_pci_nvme_mmio_bprsel(data, n->bar.bprsel,
>> +                                   NVME_BPRSEL_BPID(n->bar.bpinfo),
>> +                                   bp_offset, bp_len);
>> +
>> +        if (bp_len + bp_offset > n->bp_size) {
>> +            NVME_BPINFO_CLEAR_BRS(n->bar.bpinfo);
>> +            NVME_BPINFO_SET_BRS(n->bar.bpinfo, NVME_BPINFO_BRS_ERROR);
>> +            return;
>> +        }
>> +
>> +        off = NVME_BPRSEL_BPID(n->bar.bpinfo) * n->bp_size + bp_offset;
>> +
>> +        NVME_BPINFO_CLEAR_BRS(n->bar.bpinfo);
>> +        NVME_BPINFO_SET_BRS(n->bar.bpinfo, NVME_BPINFO_BRS_READING);
>> +
>> +        ctx = g_new(struct nvme_bp_read_ctx, 1);
>> +
>> +        ctx->n = n;
>> +
>> +        pci_dma_sglist_init(&ctx->qsg, &n->parent_obj, 1);
>> +
>> +        qemu_sglist_add(&ctx->qsg, n->bar.bpmbl, bp_len);
>> +
>> +        dma_blk_read(n->blk_bp, &ctx->qsg, off , BDRV_SECTOR_SIZE,
>> +                     nvme_bp_read_cb, ctx);
>
>The returned BlockAIOCB is not stored. Two questions:
>

Thanks for these comments Stefan, I've commented below how I think they
can be resolved.

>1. Can the guest allocate unbounded amounts of QEMU memory (struct
>   nvme_bp_read_ctx) by repeatedly writing to this register?
>

Yeah, the QSQ should just be a member of the controller struct and then 
have that as the cb_arg to dma_blk_read. Then, the QSG can be 
initialized and the host address added when BPMBL is written instead of 
here.

We don't want the QSG to change while the read is in progress, so the 
write to BPMBL should check BPINFO.BRS and not do anything if the QSG is 
"in use". The spec says that the host *shall* not modify the registers 
when a read is in progress, so we can safely just ignore the write.

>2. What happens if the NVMe device is hot unplugged or reset while a
>   boot partition read request is in flight?

The spec also says that the host *shall* not reset or shutdown the 
controller while a read is in progress, but again, we need to protect 
QEMU so the aiocb must be saved on the controller struct and cancelled 
appropriately.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]