Re: [PATCH v2 2/4] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()
Philippe Mathieu-Daudé 于2020年12月3日周四 下午8:38写道：
>
> On 12/3/20 1:02 PM, Li Qiang wrote:
> > Philippe Mathieu-Daudé 于2020年12月3日周四 下午7:37写道：
> >>
> >> Hi Li,
> >>
> >> On 12/3/20 12:21 PM, Li Qiang wrote:
> >>> Philippe Mathieu-Daudé 于2020年12月2日周三 上午3:13写道：
> >>>>
> >>>> cdb_len can not be zero... (or less than 6) here, else we have a
> >>>> out-of-bound read first in scsi_cdb_length():
> >>>>
> >>>> 71 int scsi_cdb_length(uint8_t *buf)
> >>>> 72 {
> >>>> 73 int cdb_len;
> >>>> 74
> >>>> 75 switch (buf[0] >> 5) {
> >>>
> >>> Hi Philippe,
> >>>
> >>> Here I not read the spec.
> >>
> >> Neither did I...
> >>
> >>> Just guest from your patch, this 'buf[0]>>5'
> >>> indicates/related with the cdb length, right?
> >>
> >> This is my understanding too.
> >>
> >>> So here(this patch) you just want to ensure the 'buf[0]>>5' and the
> >>> 'cdb_len' is consistent.
> >>>
> >>> But I don't think here is a 'out-of-bound' read issue.
> >>>
> >>>
> >>> The 'buf' is from here 'cdb'.
> >>> static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
> >>>int frame_cmd)
> >>> {
> >>>
> >>> cdb = cmd->frame->pass.cdb;
> >>>
> >>> 'cmd->frame->pass.cdb' is an array in heap and its data is mmaped
> >>> from the guest.
> >>>
> >>> The guest can put any data in 'pass.cdb' which 'buf[0]>>5' can be 0 or
> >>> less than 6.
> >>>
> >>> So every read of this 'pass.cdb'[0~15] is valid. Not an oob.
> >>
> >> OK maybe not OOB but infoleak?
> >
> > No. We refer 'infoleak' in qemu situation if the guest can get some
> > memory(not the guest itself, but the qemu's process memory) from the
> > qemu.
> >
> > However here the 'cdb' is actually mmaped from guest. It can be anything.
> > I think here it is just no use data.
>
> 'pass.cdb'[0~15] is allocated. If it gets filled with only
> 1 byte: 0x04, then scsi_cdb_length() returns buflen = 16
> while only 1 byte is filled, so callers will read 1 byte
> set and 15 random bytes.
Yes but no harm.
>
> You are saying this is not an "INFOleak" because the
> leaked memory is allocated on the heap, so nothing critical /
> useful gets stored there?
Yes, 'cmd->frame' is totally mapped from guest in here:
'cmd->frame = pci_dma_map(pcid, frame, _size_p, 0);'
What's the data in 'cdb' is not important from security perspective.
>
> While this might not be a security problem, this still produces
> unpredictable code behavior, so deserve to be fixed.
Yes I agree this. If we follow the exact hardware spec we need to
check how hardware handle this issue.
However as there is no harmful occurs, I think it's enough to focus
the origin issue--"g_mamloc overflow because scsi_cdb_length return
-1"
Thanks,
Li Qiang
>
> >>
> >>>> 76 case 0:
> >>>> 77 cdb_len = 6;
> >>>> 78 break;
> >>>>
> >>>> Then another out-of-bound read when the size returned by
> >>>> scsi_cdb_length() is used.
> >>>
> >>> Where is this?
> >>
> >> IIRC scsi_req_parse_cdb().
> >>
> >>>
> >>> So I think your intention is to ensure 'cdb_len' is consistent with
> >>> 'cdb[0]>>5'.
> >>>
> >>> Please correct me if I'm wrong.
> >>
> >> I'll recheck and go back to you during January.
> >>
> >> Regards,
> >>
> >> Phil.
> >>
> >>>>
> >>>> Figured out after reviewing:
> >>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg757937.html
> >>>>
> >>>> And reproduced fuzzing:
> >>>>
> >>>> qemu-fuzz-i386: hw/scsi/megasas.c:1679: int
> >>>> megasas_handle_scsi(MegasasState *, MegasasCmd *, int):
> >>>> Assertion `len > 0 && cdb_len >= len' failed.
> >>>> ==1689590== ERROR: libFuzzer: deadly signal
> >>>> #8 0x7f7a5d918e75 in __assert_fail (/lib64/libc.so.6+0x34e75)
> >>>> #9 0x55a1b95cf6d4 in megasas_handle_scsi hw/scsi/megasas.c:1679:5
> >>>> #1
Re: [PATCH v2 2/4] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()
Philippe Mathieu-Daudé 于2020年12月3日周四 下午7:37写道：
>
> Hi Li,
>
> On 12/3/20 12:21 PM, Li Qiang wrote:
> > Philippe Mathieu-Daudé 于2020年12月2日周三 上午3:13写道：
> >>
> >> cdb_len can not be zero... (or less than 6) here, else we have a
> >> out-of-bound read first in scsi_cdb_length():
> >>
> >> 71 int scsi_cdb_length(uint8_t *buf)
> >> 72 {
> >> 73 int cdb_len;
> >> 74
> >> 75 switch (buf[0] >> 5) {
> >
> > Hi Philippe,
> >
> > Here I not read the spec.
>
> Neither did I...
>
> > Just guest from your patch, this 'buf[0]>>5'
> > indicates/related with the cdb length, right?
>
> This is my understanding too.
>
> > So here(this patch) you just want to ensure the 'buf[0]>>5' and the
> > 'cdb_len' is consistent.
> >
> > But I don't think here is a 'out-of-bound' read issue.
> >
> >
> > The 'buf' is from here 'cdb'.
> > static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
> >int frame_cmd)
> > {
> >
> > cdb = cmd->frame->pass.cdb;
> >
> > 'cmd->frame->pass.cdb' is an array in heap and its data is mmaped
> > from the guest.
> >
> > The guest can put any data in 'pass.cdb' which 'buf[0]>>5' can be 0 or
> > less than 6.
> >
> > So every read of this 'pass.cdb'[0~15] is valid. Not an oob.
>
> OK maybe not OOB but infoleak?
No. We refer 'infoleak' in qemu situation if the guest can get some
memory(not the guest itself, but the qemu's process memory) from the
qemu.
However here the 'cdb' is actually mmaped from guest. It can be anything.
I think here it is just no use data.
Thanks,
Li Qiang
>
> >> 76 case 0:
> >> 77 cdb_len = 6;
> >> 78 break;
> >>
> >> Then another out-of-bound read when the size returned by
> >> scsi_cdb_length() is used.
> >
> > Where is this?
>
> IIRC scsi_req_parse_cdb().
>
> >
> > So I think your intention is to ensure 'cdb_len' is consistent with
> > 'cdb[0]>>5'.
> >
> > Please correct me if I'm wrong.
>
> I'll recheck and go back to you during January.
>
> Regards,
>
> Phil.
>
> >>
> >> Figured out after reviewing:
> >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg757937.html
> >>
> >> And reproduced fuzzing:
> >>
> >> qemu-fuzz-i386: hw/scsi/megasas.c:1679: int
> >> megasas_handle_scsi(MegasasState *, MegasasCmd *, int):
> >> Assertion `len > 0 && cdb_len >= len' failed.
> >> ==1689590== ERROR: libFuzzer: deadly signal
> >> #8 0x7f7a5d918e75 in __assert_fail (/lib64/libc.so.6+0x34e75)
> >> #9 0x55a1b95cf6d4 in megasas_handle_scsi hw/scsi/megasas.c:1679:5
> >> #10 0x55a1b95cf6d4 in megasas_handle_frame hw/scsi/megasas.c:1975:24
> >> #11 0x55a1b95cf6d4 in megasas_mmio_write hw/scsi/megasas.c:2132:9
> >> #12 0x55a1b981972e in memory_region_write_accessor
> >> softmmu/memory.c:491:5
> >> #13 0x55a1b981972e in access_with_adjusted_size
> >> softmmu/memory.c:552:18
> >> #14 0x55a1b981972e in memory_region_dispatch_write
> >> softmmu/memory.c:1501:16
> >> #15 0x55a1b97f0ab0 in flatview_write_continue
> >> softmmu/physmem.c:2759:23
> >> #16 0x55a1b97ec3f2 in flatview_write softmmu/physmem.c:2799:14
> >> #17 0x55a1b97ec3f2 in address_space_write softmmu/physmem.c:2891:18
> >> #18 0x55a1b985c7cd in cpu_outw softmmu/ioport.c:70:5
> >> #19 0x55a1b99577ac in qtest_process_command softmmu/qtest.c:481:13
> >>
> >> Inspired-by: Daniele Buono
> >> Inspired-by: Alexander Bulekov
> >> Signed-off-by: Philippe Mathieu-Daudé
> >> ---
> >> hw/scsi/megasas.c | 5 +
> >> 1 file changed, 5 insertions(+)
> >>
> >> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> >> index 1a5fc5857db..f5ad4425b5b 100644
> >> --- a/hw/scsi/megasas.c
> >> +++ b/hw/scsi/megasas.c
> >> @@ -1667,6 +1667,7 @@ static int megasas_handle_scsi(MegasasState *s,
> >> MegasasCmd *cmd,
> >> {
> >> uint8_t *cdb;
> >> int target_id, lun_id, cdb_len;
> >> +int len = -1;
> >> bool is_write;
> >> struct SCSIDevice *sdev = NULL;
> >> bool is_logical = (frame_cmd == MFI_CMD_LD_SCSI_IO);
> >> @@ -1676,6 +1677,10 @@ static int megasas_handle_scsi(MegasasState *s,
> >> MegasasCmd *cmd,
> >> lun_id = cmd->frame->header.lun_id;
> >> cdb_len = cmd->frame->header.cdb_len;
> >>
> >> +if (cdb_len > 0) {
> >> +len = scsi_cdb_length(cdb);
> >> +}
> >> +assert(len > 0 && cdb_len >= len);
> >> if (is_logical) {
> >> if (target_id >= MFI_MAX_LD || lun_id != 0) {
> >> trace_megasas_scsi_target_not_present(
> >> --
> >> 2.26.2
> >>
> >>
> >
>
Re: [PATCH v2 2/4] hw/scsi/megasas: Assert cdb_len is valid in megasas_handle_scsi()
Philippe Mathieu-Daudé 于2020年12月2日周三 上午3:13写道：
>
> cdb_len can not be zero... (or less than 6) here, else we have a
> out-of-bound read first in scsi_cdb_length():
>
> 71 int scsi_cdb_length(uint8_t *buf)
> 72 {
> 73 int cdb_len;
> 74
> 75 switch (buf[0] >> 5) {
Hi Philippe,
Here I not read the spec. Just guest from your patch, this 'buf[0]>>5'
indicates/related with the cdb length, right?
So here(this patch) you just want to ensure the 'buf[0]>>5' and the
'cdb_len' is consistent.
But I don't think here is a 'out-of-bound' read issue.
The 'buf' is from here 'cdb'.
static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
int frame_cmd)
{
cdb = cmd->frame->pass.cdb;
'cmd->frame->pass.cdb' is an array in heap and its data is mmaped
from the guest.
The guest can put any data in 'pass.cdb' which 'buf[0]>>5' can be 0 or
less than 6.
So every read of this 'pass.cdb'[0~15] is valid. Not an oob.
> 76 case 0:
> 77 cdb_len = 6;
> 78 break;
>
> Then another out-of-bound read when the size returned by
> scsi_cdb_length() is used.
Where is this?
So I think your intention is to ensure 'cdb_len' is consistent with
'cdb[0]>>5'.
Please correct me if I'm wrong.
Thanks,
Li Qiang
>
> Figured out after reviewing:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg757937.html
>
> And reproduced fuzzing:
>
> qemu-fuzz-i386: hw/scsi/megasas.c:1679: int
> megasas_handle_scsi(MegasasState *, MegasasCmd *, int):
> Assertion `len > 0 && cdb_len >= len' failed.
> ==1689590== ERROR: libFuzzer: deadly signal
> #8 0x7f7a5d918e75 in __assert_fail (/lib64/libc.so.6+0x34e75)
> #9 0x55a1b95cf6d4 in megasas_handle_scsi hw/scsi/megasas.c:1679:5
> #10 0x55a1b95cf6d4 in megasas_handle_frame hw/scsi/megasas.c:1975:24
> #11 0x55a1b95cf6d4 in megasas_mmio_write hw/scsi/megasas.c:2132:9
> #12 0x55a1b981972e in memory_region_write_accessor
> softmmu/memory.c:491:5
> #13 0x55a1b981972e in access_with_adjusted_size softmmu/memory.c:552:18
> #14 0x55a1b981972e in memory_region_dispatch_write
> softmmu/memory.c:1501:16
> #15 0x55a1b97f0ab0 in flatview_write_continue softmmu/physmem.c:2759:23
> #16 0x55a1b97ec3f2 in flatview_write softmmu/physmem.c:2799:14
> #17 0x55a1b97ec3f2 in address_space_write softmmu/physmem.c:2891:18
> #18 0x55a1b985c7cd in cpu_outw softmmu/ioport.c:70:5
> #19 0x55a1b99577ac in qtest_process_command softmmu/qtest.c:481:13
>
> Inspired-by: Daniele Buono
> Inspired-by: Alexander Bulekov
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/scsi/megasas.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 1a5fc5857db..f5ad4425b5b 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1667,6 +1667,7 @@ static int megasas_handle_scsi(MegasasState *s,
> MegasasCmd *cmd,
> {
> uint8_t *cdb;
> int target_id, lun_id, cdb_len;
> +int len = -1;
> bool is_write;
> struct SCSIDevice *sdev = NULL;
> bool is_logical = (frame_cmd == MFI_CMD_LD_SCSI_IO);
> @@ -1676,6 +1677,10 @@ static int megasas_handle_scsi(MegasasState *s,
> MegasasCmd *cmd,
> lun_id = cmd->frame->header.lun_id;
> cdb_len = cmd->frame->header.cdb_len;
>
> +if (cdb_len > 0) {
> +len = scsi_cdb_length(cdb);
> +}
> +assert(len > 0 && cdb_len >= len);
> if (is_logical) {
> if (target_id >= MFI_MAX_LD || lun_id != 0) {
> trace_megasas_scsi_target_not_present(
> --
> 2.26.2
>
>
Re: [PATCH v2 1/4] tests/qtest/fuzz-test: Quit test_lp1878642 once done
Philippe Mathieu-Daudé 于2020年12月2日周三 上午3:11写道：
>
> Missed in fd250172842 ("qtest: add a reproducer for LP#1878642").
>
> Reviewed-by: Thomas Huth
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> tests/qtest/fuzz-test.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
> index 9cb4c42bdea..87b72307a5b 100644
> --- a/tests/qtest/fuzz-test.c
> +++ b/tests/qtest/fuzz-test.c
> @@ -45,6 +45,7 @@ static void
> test_lp1878642_pci_bus_get_irq_level_assert(void)
> qtest_outl(s, 0xcf8, 0x8400f841);
> qtest_outl(s, 0xcfc, 0xebed205d);
> qtest_outl(s, 0x5d02, 0xebed205d);
> +qtest_quit(s);
> }
>
> int main(int argc, char **argv)
> --
> 2.26.2
>
>
Re: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
Laurent Vivier 于2020年10月12日周一 下午11:33写道：
>
> Le 10/10/2020 à 13:07, Chen Qun a écrit :
> > This if statement judgment is redundant and it will cause a warning:
> >
> > migration/block-dirty-bitmap.c:1090:13: warning: ‘bitmap_name’ may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
> > g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
> > ^~
> >
> > Reported-by: Euler Robot
> > Signed-off-by: Chen Qun
> > ---
> > migration/block-dirty-bitmap.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> > index 5bef793ac0..e09ea4f22b 100644
> > --- a/migration/block-dirty-bitmap.c
> > +++ b/migration/block-dirty-bitmap.c
> > @@ -1084,9 +1084,7 @@ static int dirty_bitmap_load_header(QEMUFile *f,
> > DBMLoadState *s,
> > } else {
> > bitmap_name = s->bitmap_alias;
> > }
> > -}
> >
> > -if (!s->cancelled) {
> > g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
> > s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
> >
> >
>
> I don't think it's correct as "cancel_incoming_locked(s)" can change the
> value of "s->cancelled".
>
Hi Laurent,
You're right. So I think this can simply assign 'bitmap_name' to NULL
to make compiler happy.
Thanks,
Li Qiang
> Thanks,
> Laurent
>
Re: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
Chen Qun 于2020年10月10日周六 下午7:08写道：
>
> This if statement judgment is redundant and it will cause a warning:
>
> migration/block-dirty-bitmap.c:1090:13: warning: ‘bitmap_name’ may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
> ^~
>
> Reported-by: Euler Robot
> Signed-off-by: Chen Qun
Reviewed-by: Li Qiang
> ---
> migration/block-dirty-bitmap.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 5bef793ac0..e09ea4f22b 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -1084,9 +1084,7 @@ static int dirty_bitmap_load_header(QEMUFile *f,
> DBMLoadState *s,
> } else {
> bitmap_name = s->bitmap_alias;
> }
> -}
>
> -if (!s->cancelled) {
> g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
> s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
>
> --
> 2.23.0
>
>
Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync
P J P 于2020年9月29日周二 下午2:22写道：
>
> Hello Li,
>
> +-- On Fri, 18 Sep 2020, Li Qiang wrote --+
> | P J P 于2020年9月18日周五 下午6:26写道：
> | > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+
> | > | Update v2: use an assert() call
> | > |
> ->https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html
> |
> | In 'ide_ioport_write' the guest can set 'bus->unit' to 0 or 1 by issue
> | 'ATA_IOPORT_WR_DEVICE_HEAD'. So this case the guest can set the active ifs.
> | If the guest set this to 1.
> |
> | Then in 'idebus_active_if' will return 'IDEBus.ifs[1]' and thus the 's->blk'
> | will be NULL.
>
> Right, guest does select the drive via
>
> portio_write
>->ide_ioport_write
> case ATA_IOPORT_WR_DEVICE_HEAD:
> /* FIXME: HOB readback uses bit 7 */
> bus->ifs[0].select = (val & ~0x10) | 0xa0;
> bus->ifs[1].select = (val | 0x10) | 0xa0;
> /* select drive */
> bus->unit = (val >> 4) & 1; <== set bus->unit=0x1
> break;
>
>
> | So from your (Peter's) saying, we need to check the value in
> | 'ATA_IOPORT_WR_DEVICE_HEAD' handler. To say if the guest
> | set a valid 'bus->unit'. This can also work I think.
>
> Yes, with the following fix, an assert(3) in ide_cancel_dma_sync fails.
>
> ===
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index f76f7e5234..cb55cc8b0f 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1300,7 +1300,11 @@ void ide_ioport_write(void *opaque, uint32_t addr,
> uint_)
> bus->ifs[0].select = (val & ~0x10) | 0xa0;
> bus->ifs[1].select = (val | 0x10) | 0xa0;
> /* select drive */
> +uint8_t bu = bus->unit;
> bus->unit = (val >> 4) & 1;
> +if (!bus->ifs[bus->unit].blk) {
> +bus->unit = bu;
> +}
> break;
> default:
>
> qemu-system-x86_64: ../hw/ide/core.c:724: ide_cancel_dma_sync: Assertion
> `s->bus->dma->aiocb == NULL' failed.
> Aborted (core dumped)
This is what I am worried, in the 'ide_ioport_write' set the
'bus->unit'. It also change the 'buf->ifs[0].select'.
Also there maybe some other corner case that causes some inconsistent.
And if we choice this method we need to deep into the more ahci-spec to
know how things really going.
> ===
>
> | As we the 'ide_exec_cmd' and other functions in 'hw/ide/core.c' check the
> | 's->blk' directly. I think we just check it in 'ide_cancel_dma_sync' is
> | enough and also this is more consistent with the other functions.
> | 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' which is one of
> | the 'ide_cmd_table' handler.
>
> Yes, I'm okay with either approach. Earlier patch v1 checks 's->blk' in
> ide_cancel_dma_sync().
I prefer the 'check the s->blk in the beginning of ide_cancel_dma_sync' method.
Some little different with your earlier patch.
Anyway, let the maintainer do the choices.
Thanks,
Li Qiang
>
> | BTW, where is the Peter's email saying this, just want to learn something,
> | :).
>
> -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg05820.html
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Re: [PATCH] hw: ide: check the pointer before do dma memory unmap
Philippe Mathieu-Daudé 于2020年9月22日周二 下午6:46写道：
>
> On 9/22/20 12:37 PM, Li Qiang wrote:
> > Philippe Mathieu-Daudé 于2020年9月22日周二 下午4:19写道：
> >>
> >> On 9/22/20 3:34 AM, Alexander Bulekov wrote:
> >>> On 200815 0020, Li Qiang wrote:
> >>>> In 'map_page' we need to check the return value of
> >>>> 'dma_memory_map' to ensure the we actully maped something.
> >>>> Otherwise, we will hit an assert in 'address_space_unmap'.
> >>>> This is because we can't find the MR with the NULL buffer.
> >>>> This is the LP#1884693:
> >>>>
> >>>> -->https://bugs.launchpad.net/qemu/+bug/1884693
> >>>>
> >>>> Reported-by: Alexander Bulekov
> >>>> Signed-off-by: Li Qiang
> >>>
> >>> I'm not very familiar with the IDE code, but this seems like a simple
> >>> null-ptr check, and Li has not received a response in over a month.
> >>
> >> Yeah well it is not an easy bug... I spent few hours but at some
> >> point it became too AHCI specific. I wanted to understand the bug
> >> to answer the "Why do we get there?" "Can we get there with real
> >> hardware?" questions, to be able to discern if this patch is OK,
> >> or if it is hiding bugs and what we really use here is an assert().
> >
> > Hi Philippe,
> > I think you have complicated this issue. The root cause is that
> > 'dma_memory_map' maybe fail.
> > The gpa is from guest and can be any value so this is expected.
> > It can return NULL pointer (no map) or it can be do partially
> > mapped(len < wanted).
> > Though in most situation the map result is 'ret == NULL and len <
> > wanted'. It may also has '
> > ret != NULL and len < wanted' I think.
>
> Then this form is easier to review to my taste:
>
> -- >8 --
> @@ -250,7 +250,7 @@ static void map_page(AddressSpace *as, uint8_t
> **ptr, uint64_t addr,
> }
>
> *ptr = dma_memory_map(as, addr, , DMA_DIRECTION_FROM_DEVICE);
> -if (len < wanted) {
> +if (*ptr && len < wanted) {
> dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
> *ptr = NULL;
> }
Yes, in this case your patch is more clean and less code change.
But in generic case, my origin patch is more easy to understand and also
It is easy to do resource clean or trace (such as 'ahci_populate_sglist').
Anyway just let John do the choice.
Thanks,
Li Qiang
> ---
>
> >
> > The assert is come from that we pass NULL to 'dma_memory_unmap'.
> >
> > So the standard usage of 'dma_memory_map' I think is first check if
> > the return value to ensure it is not NULL.
> > Then check whether it mapped the len as the caller expected.
> >
> > There are several places in the code base that doesn't following this
> > usage which I think it is wrong.
> >
> > Thanks,
> > Li Qiang
> >
> >>
> >>>
> >>> Reviewed-by: Alexander Bulekov
> >>>
> >>>> ---
> >>>> hw/ide/ahci.c | 5 +
> >>>> 1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> >>>> index 009120f88b..63e9fccdbe 100644
> >>>> --- a/hw/ide/ahci.c
> >>>> +++ b/hw/ide/ahci.c
> >>>> @@ -250,6 +250,11 @@ static void map_page(AddressSpace *as, uint8_t
> >>>> **ptr, uint64_t addr,
> >>>> }
> >>>>
> >>>> *ptr = dma_memory_map(as, addr, , DMA_DIRECTION_FROM_DEVICE);
> >>>> +
> >>>> +if (!*ptr) {
> >>>> +return;
> >>>> +}
> >>>> +
> >>>> if (len < wanted) {
> >>>> dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
> >>>> *ptr = NULL;
> >>>> --
> >>>> 2.17.1
> >>>>
> >>>
> >>
> >
>
Re: [PATCH v2] fdc: check null block pointer before r/w data transfer
P J P 于2020年9月22日周二 下午5:29写道：
>
> From: Prasad J Pandit
>
> While transferring data via fdctrl_read/write_data() routines,
> check that current drive does not have a null block pointer.
> Avoid null pointer dereference.
>
> -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Ffdc_nullptr1
> ==1658854==Hint: address points to the zero page.
> #0 blk_inc_in_flight block/block-backend.c:1327
> #1 blk_prw block/block-backend.c:1299
> #2 blk_pwrite block/block-backend.c:1464
> #3 fdctrl_write_data hw/block/fdc.c:2418
> #4 fdctrl_write hw/block/fdc.c:962
> #5 portio_write ioport.c:205
> #6 memory_region_write_accessor memory.c:483
> #7 access_with_adjusted_size memory.c:544
> #8 memory_region_dispatch_write memory.c:1476
>
> Reported-by: Ruhr-University
> Signed-off-by: Prasad J Pandit
Reviewed-by: Li Qiang
> ---
> hw/block/fdc.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> Update v2: treat NULL blk pointer as an error
> -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg06642.html
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 224bac504f..bf968dc66f 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1923,7 +1923,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
> fd_sector(cur_drv));
> return 0;
> }
> -if (blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> +if (!cur_drv->blk
> +|| blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
>BDRV_SECTOR_SIZE)
> < 0) {
> FLOPPY_DPRINTF("error getting sector %d\n",
> @@ -2427,7 +2428,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t
> value)
> if (pos == FD_SECTOR_LEN - 1 ||
> fdctrl->data_pos == fdctrl->data_len) {
> cur_drv = get_cur_drv(fdctrl);
> -if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> +if (!cur_drv->blk
> +|| blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> BDRV_SECTOR_SIZE, 0) < 0) {
> FLOPPY_DPRINTF("error writing sector %d\n",
> fd_sector(cur_drv));
> --
> 2.26.2
>
Re: [PATCH] hw: ide: check the pointer before do dma memory unmap
Philippe Mathieu-Daudé 于2020年9月22日周二 下午4:19写道：
>
> On 9/22/20 3:34 AM, Alexander Bulekov wrote:
> > On 200815 0020, Li Qiang wrote:
> >> In 'map_page' we need to check the return value of
> >> 'dma_memory_map' to ensure the we actully maped something.
> >> Otherwise, we will hit an assert in 'address_space_unmap'.
> >> This is because we can't find the MR with the NULL buffer.
> >> This is the LP#1884693:
> >>
> >> -->https://bugs.launchpad.net/qemu/+bug/1884693
> >>
> >> Reported-by: Alexander Bulekov
> >> Signed-off-by: Li Qiang
> >
> > I'm not very familiar with the IDE code, but this seems like a simple
> > null-ptr check, and Li has not received a response in over a month.
>
> Yeah well it is not an easy bug... I spent few hours but at some
> point it became too AHCI specific. I wanted to understand the bug
> to answer the "Why do we get there?" "Can we get there with real
> hardware?" questions, to be able to discern if this patch is OK,
> or if it is hiding bugs and what we really use here is an assert().
Hi Philippe,
I think you have complicated this issue. The root cause is that
'dma_memory_map' maybe fail.
The gpa is from guest and can be any value so this is expected.
It can return NULL pointer (no map) or it can be do partially
mapped(len < wanted).
Though in most situation the map result is 'ret == NULL and len <
wanted'. It may also has '
ret != NULL and len < wanted' I think.
The assert is come from that we pass NULL to 'dma_memory_unmap'.
So the standard usage of 'dma_memory_map' I think is first check if
the return value to ensure it is not NULL.
Then check whether it mapped the len as the caller expected.
There are several places in the code base that doesn't following this
usage which I think it is wrong.
Thanks,
Li Qiang
>
> >
> > Reviewed-by: Alexander Bulekov
> >
> >> ---
> >> hw/ide/ahci.c | 5 +
> >> 1 file changed, 5 insertions(+)
> >>
> >> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> >> index 009120f88b..63e9fccdbe 100644
> >> --- a/hw/ide/ahci.c
> >> +++ b/hw/ide/ahci.c
> >> @@ -250,6 +250,11 @@ static void map_page(AddressSpace *as, uint8_t **ptr,
> >> uint64_t addr,
> >> }
> >>
> >> *ptr = dma_memory_map(as, addr, , DMA_DIRECTION_FROM_DEVICE);
> >> +
> >> +if (!*ptr) {
> >> +return;
> >> +}
> >> +
> >> if (len < wanted) {
> >> dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
> >> *ptr = NULL;
> >> --
> >> 2.17.1
> >>
> >
>
Re: [PATCH] hw: ide: check the pointer before do dma memory unmap
Ping!!
Li Qiang 于2020年9月15日周二 下午9:38写道：
>
> ping!!
>
> Li Qiang 于2020年9月7日周一 上午9:39写道：
> >
> > Ping!
> >
> > Li Qiang 于2020年9月1日周二 下午6:34写道：
> > >
> > > Ping.
> > >
> > > Li Qiang 于2020年8月15日周六 下午3:21写道：
> > > >
> > > > In 'map_page' we need to check the return value of
> > > > 'dma_memory_map' to ensure the we actully maped something.
> > > > Otherwise, we will hit an assert in 'address_space_unmap'.
> > > > This is because we can't find the MR with the NULL buffer.
> > > > This is the LP#1884693:
> > > >
> > > > -->https://bugs.launchpad.net/qemu/+bug/1884693
> > > >
> > > > Reported-by: Alexander Bulekov
> > > > Signed-off-by: Li Qiang
> > > > ---
> > > > hw/ide/ahci.c | 5 +
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > > > index 009120f88b..63e9fccdbe 100644
> > > > --- a/hw/ide/ahci.c
> > > > +++ b/hw/ide/ahci.c
> > > > @@ -250,6 +250,11 @@ static void map_page(AddressSpace *as, uint8_t
> > > > **ptr, uint64_t addr,
> > > > }
> > > >
> > > > *ptr = dma_memory_map(as, addr, , DMA_DIRECTION_FROM_DEVICE);
> > > > +
> > > > +if (!*ptr) {
> > > > +return;
> > > > +}
> > > > +
> > > > if (len < wanted) {
> > > > dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE,
> > > > len);
> > > > *ptr = NULL;
> > > > --
> > > > 2.17.1
> > > >
Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync
P J P 于2020年9月18日周五 下午6:26写道： > > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+ > | Update v2: use an assert() call > | ->https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html > ... > | I think it is better to defer this check to 'ide_cancel_dma_sync'. > | 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' and all of the > | handlers of 'ide_cmd_table' will check whether the 's->blk' is NULL in the > | beginning of 'ide_exec_cmd'. > | > | So I think it is reasonable to check 's->blk' at the begining of > | 'ide_cancel_dma_sync'. > > * Yes, earlier patch v1 above does the same. > > * From Peter's reply in another thread of similar issue I gather, issue is > setting 'blk' to NULL, even erroneously. So (blk == NULL) check should be > done where 'blk' is set to null, rather than where it is dereferenced. > I don't find anywhere set the 'blk' to NULL. I think this NULL deference is caused by following: In 'pci_ide_create_devs' we call 'ide_create_drive', in the latter function it will create the 's->blk'. However it is not for every IDEBus. IDEBus[0]: ifs[2] IDEBus[1]: ifs[2] The 'ide_create_drive' will only initialize the 'IDEBus[0].ifs[0]' and 'IDEBus[1].ifs[0]' from the reproducer command line. So the 'IDEBus[0].ifs[1]' and 'IDEBus[1].ifs[1]' s blk will be NULL. In 'ide_ioport_write' the guest can set 'bus->unit' to 0 or 1 by issue 'ATA_IOPORT_WR_DEVICE_HEAD'. So this case the guest can set the active ifs. If the guest set this to 1. Then in 'idebus_active_if' will return 'IDEBus.ifs[1]' and thus the 's->blk' will be NULL. So from your (Peter's) saying, we need to check the value in 'ATA_IOPORT_WR_DEVICE_HEAD' handler. To say if the guest set a valid 'bus->unit'. This can also work I think. As we the 'ide_exec_cmd' and other functions in 'hw/ide/core.c' check the 's->blk' directly. I think we just check it in 'ide_cancel_dma_sync' is enough and also this is more consistent with the other functions. 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' which is one of the 'ide_cmd_table' handler. BTW, where is the Peter's email saying this, just want to learn something, :). Thanks, Li Qiang > * At the dereference point, assert(3) is good. > > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D >
Re: [PATCH] fdc: check null block pointer before blk_pwrite
P J P 于2020年8月27日周四 下午7:41写道：
>
> From: Prasad J Pandit
>
> While transferring data via fdctrl_write_data(), check that
> current drive does not have a null block pointer. Avoid
> null pointer dereference.
>
> -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Ffdc_nullptr1
> ==1658854==Hint: address points to the zero page.
> #0 blk_inc_in_flight block/block-backend.c:1327
> #1 blk_prw block/block-backend.c:1299
> #2 blk_pwrite block/block-backend.c:1464
> #3 fdctrl_write_data hw/block/fdc.c:2418
> #4 fdctrl_write hw/block/fdc.c:962
> #5 portio_write ioport.c:205
> #6 memory_region_write_accessor memory.c:483
> #7 access_with_adjusted_size memory.c:544
> #8 memory_region_dispatch_write memory.c:1476
>
> Reported-by: Ruhr-University
> Signed-off-by: Prasad J Pandit
> ---
> hw/block/fdc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index e9ed3eef45..dedadac68a 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2419,7 +2419,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t
> value)
> if (pos == FD_SECTOR_LEN - 1 ||
> fdctrl->data_pos == fdctrl->data_len) {
> cur_drv = get_cur_drv(fdctrl);
> -if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> +if (cur_drv->blk
> +&& blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> BDRV_SECTOR_SIZE, 0) < 0) {
> FLOPPY_DPRINTF("error writing sector %d\n",
> fd_sector(cur_drv));
> --
Hello Prasad,
There are several pattern to address this NULL check in fdc.c.
I think it is better to consider this as an error. Just like the check
in 'fdctrl_format_sector':
if (cur_drv->blk == NULL ||
blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
BDRV_SECTOR_SIZE, 0) < 0) {
FLOPPY_DPRINTF("error formatting sector %d\n", fd_sector(cur_drv));
fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
} else {
Also there seems exists the same issue in 'fdctrl_read_data'
Thanks,
Li Qiang
> 2.26.2
>
>
Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync
P J P 于2020年9月4日周五 上午2:34写道：
>
> From: Prasad J Pandit
>
> When cancelling an i/o operation via ide_cancel_dma_sync(),
> a block pointer may be null. Add check to avoid null pointer
> dereference.
>
> -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fide_nullptr1
> ==1803100==Hint: address points to the zero page.
> #0 blk_bs ../block/block-backend.c:714
> #1 blk_drain ../block/block-backend.c:1715
> #2 ide_cancel_dma_sync ../hw/ide/core.c:723
> #3 bmdma_cmd_writeb ../hw/ide/pci.c:298
> #4 bmdma_write ../hw/ide/piix.c:75
> #5 memory_region_write_accessor ../softmmu/memory.c:483
> #6 access_with_adjusted_size ../softmmu/memory.c:544
> #7 memory_region_dispatch_write ../softmmu/memory.c:1465
> #8 flatview_write_continue ../exec.c:3176
> ...
>
> Reported-by: Ruhr-University
> Signed-off-by: Prasad J Pandit
> ---
> hw/ide/core.c | 1 +
> hw/ide/pci.c | 5 -
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> Update v2: use an assert() call
> -> https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index f76f7e5234..8105187f49 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -718,6 +718,7 @@ void ide_cancel_dma_sync(IDEState *s)
> * whole DMA operation will be submitted to disk with a single
> * aio operation with preadv/pwritev.
> */
> +assert(s->blk);
> if (s->bus->dma->aiocb) {
> trace_ide_cancel_dma_sync_remaining();
> blk_drain(s->blk);
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index b50091b615..b47e675456 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -295,7 +295,10 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
> /* Ignore writes to SSBM if it keeps the old value */
> if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
> if (!(val & BM_CMD_START)) {
> -ide_cancel_dma_sync(idebus_active_if(bm->bus));
> +IDEState *s = idebus_active_if(bm->bus);
> +if (s->blk) {
> +ide_cancel_dma_sync(s);
> +}
> bm->status &= ~BM_STATUS_DMAING;
> } else {
> bm->cur_addr = bm->addr;
> --
Hello Prasad,
'bmdma_cmd_writeb' is in the bmdma layer, I think it is better to not
touch the IDE layer(check the 's->blk').
I think it is better to defer this check to 'ide_cancel_dma_sync'.
'ide_cancel_dma_sync' is also called by 'cmd_device_reset' and all of
the handlers of 'ide_cmd_table' will check
whether the 's->blk' is NULL in the beginning of 'ide_exec_cmd'.
So I think it is reasonable to check 's->blk' at the begining of
'ide_cancel_dma_sync'.
I'm not a blk expert, please correct me if I'm wrong.
Thanks,
Li Qiang
> 2.26.2
>
>
Re: [PATCH v2 2/3] virtio-blk: undo destructive iov_discard_*() operations
Stefan Hajnoczi 于2020年9月17日周四 下午5:47写道：
>
> Fuzzing discovered that virtqueue_unmap_sg() is being called on modified
> req->in/out_sg iovecs. This means dma_memory_map() and
> dma_memory_unmap() calls do not have matching memory addresses.
>
> Fuzzing discovered that non-RAM addresses trigger a bug:
>
> void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>bool is_write, hwaddr access_len)
> {
> if (buffer != bounce.buffer) {
> ^^^
>
> A modified iov->iov_base is no longer recognized as a bounce buffer and
> the wrong branch is taken.
>
> There are more potential bugs: dirty memory is not tracked correctly and
> MemoryRegion refcounts can be leaked.
>
> Use the new iov_discard_undo() API to restore elem->in/out_sg before
> virtqueue_push() is called.
>
> Reported-by: Alexander Bulekov
> Buglink: https://bugs.launchpad.net/qemu/+bug/1890360
> Fixes: 827805a2492c1bbf1c0712ed18ee069b4ebf3dd6 ("virtio-blk: Convert
> VirtIOBlockReq.out to structrue")
> Signed-off-by: Stefan Hajnoczi
Reviewed-by: Li Qiang
> ---
> include/hw/virtio/virtio-blk.h | 2 ++
> hw/block/virtio-blk.c | 11 +--
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 29c9f32353..df3876d49c 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -72,6 +72,8 @@ typedef struct VirtIOBlockReq {
> int64_t sector_num;
> VirtIOBlock *dev;
> VirtQueue *vq;
> +IOVDiscardUndo inhdr_undo;
> +IOVDiscardUndo outhdr_undo;
> struct virtio_blk_inhdr *in;
> struct virtio_blk_outhdr out;
> QEMUIOVector qiov;
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 2204ba149e..bac2d6fa2b 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -80,6 +80,8 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req,
> unsigned char status)
> trace_virtio_blk_req_complete(vdev, req, status);
>
> stb_p(>in->status, status);
> +iov_discard_undo(>inhdr_undo);
> +iov_discard_undo(>outhdr_undo);
> virtqueue_push(req->vq, >elem, req->in_len);
> if (s->dataplane_started && !s->dataplane_disabled) {
> virtio_blk_data_plane_notify(s->dataplane, req->vq);
> @@ -632,10 +634,12 @@ static int virtio_blk_handle_request(VirtIOBlockReq
> *req, MultiReqBuffer *mrb)
> return -1;
> }
>
> -iov_discard_front(_iov, _num, sizeof(req->out));
> +iov_discard_front_undoable(_iov, _num, sizeof(req->out),
> + >outhdr_undo);
>
> if (in_iov[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) {
> virtio_error(vdev, "virtio-blk request inhdr too short");
> +iov_discard_undo(>outhdr_undo);
> return -1;
> }
>
> @@ -644,7 +648,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
> MultiReqBuffer *mrb)
> req->in = (void *)in_iov[in_num - 1].iov_base
>+ in_iov[in_num - 1].iov_len
>- sizeof(struct virtio_blk_inhdr);
> -iov_discard_back(in_iov, _num, sizeof(struct virtio_blk_inhdr));
> +iov_discard_back_undoable(in_iov, _num, sizeof(struct
> virtio_blk_inhdr),
> + >inhdr_undo);
>
> type = virtio_ldl_p(vdev, >out.type);
>
> @@ -739,6 +744,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
> MultiReqBuffer *mrb)
>
> if (unlikely(iov_to_buf(out_iov, out_num, 0, _hdr,
> sizeof(dwz_hdr)) != sizeof(dwz_hdr))) {
> +iov_discard_undo(>inhdr_undo);
> +iov_discard_undo(>outhdr_undo);
> virtio_error(vdev, "virtio-blk discard/write_zeroes header"
> " too short");
> return -1;
> --
> 2.26.2
>
Re: [PATCH 2/3] virtio-blk: undo destructive iov_discard_*() operations
Stefan Hajnoczi 于2020年8月12日周三 下午6:51写道：
>
> Fuzzing discovered that virtqueue_unmap_sg() is being called on modified
> req->in/out_sg iovecs. This means dma_memory_map() and
> dma_memory_unmap() calls do not have matching memory addresses.
>
> Fuzzing discovered that non-RAM addresses trigger a bug:
>
> void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>bool is_write, hwaddr access_len)
> {
> if (buffer != bounce.buffer) {
> ^^^
>
> A modified iov->iov_base is no longer recognized as a bounce buffer and
> the wrong branch is taken.
>
> There are more potential bugs: dirty memory is not tracked correctly and
> MemoryRegion refcounts can be leaked.
>
> Use the new iov_discard_undo() API to restore elem->in/out_sg before
> virtqueue_push() is called.
>
> Reported-by: Alexander Bulekov
> Buglink: https://bugs.launchpad.net/qemu/+bug/1890360
> Fixes: 827805a2492c1bbf1c0712ed18ee069b4ebf3dd6 ("virtio-blk: Convert
> VirtIOBlockReq.out to structrue")
> Signed-off-by: Stefan Hajnoczi
> ---
> include/hw/virtio/virtio-blk.h | 2 ++
> hw/block/virtio-blk.c | 9 +++--
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index b1334c3904..0af654cace 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -68,6 +68,8 @@ typedef struct VirtIOBlockReq {
> int64_t sector_num;
> VirtIOBlock *dev;
> VirtQueue *vq;
> +IOVDiscardUndo inhdr_undo;
> +IOVDiscardUndo outhdr_undo;
> struct virtio_blk_inhdr *in;
> struct virtio_blk_outhdr out;
> QEMUIOVector qiov;
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 413783693c..2b7cc3e1c8 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -80,6 +80,8 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req,
> unsigned char status)
> trace_virtio_blk_req_complete(vdev, req, status);
>
> stb_p(>in->status, status);
> +iov_discard_undo(>inhdr_undo);
> +iov_discard_undo(>outhdr_undo);
> virtqueue_push(req->vq, >elem, req->in_len);
> if (s->dataplane_started && !s->dataplane_disabled) {
> virtio_blk_data_plane_notify(s->dataplane, req->vq);
> @@ -632,10 +634,12 @@ static int virtio_blk_handle_request(VirtIOBlockReq
> *req, MultiReqBuffer *mrb)
> return -1;
> }
>
> -iov_discard_front(_iov, _num, sizeof(req->out));
> +iov_discard_front_undoable(_iov, _num, sizeof(req->out),
> + >outhdr_undo);
>
> if (in_iov[in_num - 1].iov_len < sizeof(struct virtio_blk_inhdr)) {
> virtio_error(vdev, "virtio-blk request inhdr too short");
> +iov_discard_undo(>outhdr_undo);
> return -1;
> }
>
> @@ -644,7 +648,8 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req,
> MultiReqBuffer *mrb)
> req->in = (void *)in_iov[in_num - 1].iov_base
>+ in_iov[in_num - 1].iov_len
>- sizeof(struct virtio_blk_inhdr);
> -iov_discard_back(in_iov, _num, sizeof(struct virtio_blk_inhdr));
> + iov_discard_back_undoable(in_iov, _num, sizeof(struct
> virtio_blk_inhdr),
> + >inhdr_undo);
>
> type = virtio_ldl_p(vdev, >out.type);
>
It seems there is another error path need to do the undo operations.
case VIRTIO_BLK_T_WRITE_ZEROS & ~VIRTIO_BLK_T_OUT
handler has an error path.
Thanks,
Li Qiang
> --
> 2.26.2
>
Re: [PATCH 1/3] util/iov: add iov_discard_undo()
Stefan Hajnoczi 于2020年9月16日周三 下午6:09写道：
>
> On Sun, Aug 16, 2020 at 04:26:45PM +0800, Li Qiang wrote:
> > Stefan Hajnoczi 于2020年8月12日周三 下午6:52写道：
>
> Thanks for your review!
>
> > > +/* Discard more bytes than vector size */
> > > +iov_random(, _cnt);
> > > +iov_orig = g_memdup(iov, sizeof(iov[0]) * iov_cnt);
> > > +iov_tmp = iov;
> > > +iov_cnt_tmp = iov_cnt;
> > > +size = iov_size(iov, iov_cnt);
> > > +iov_discard_front_undoable(_tmp, _cnt_tmp, size + 1, );
> > > +iov_discard_undo();
> > > +assert(iov_equals(iov, iov_orig, iov_cnt));
> > >
> >
> > The 'iov_discard_front_undoable' will change the 'iov_tmp' it will not
> > touch 'iov_orig'.
> > So the test will be always passed. The actually function will not be tested.
>
> The test verifies that the iovec elements are restored to their previous
> state by iov_discard_undo().
>
> I think you are saying you'd like iov_discard_undo() to reset the
> iov_tmp pointer? Currently that is not how the API works. The caller is
> assumed to have the original pointer (e.g. virtio-blk has
> req->elem.in/out_sg) and therefore it is not necessary to reset iov_tmp.
>
Hi Stefan,
You're right, I just have the idea in my mind the the 'discard'
function change the *iov, but the caller have the origin pointer.
So
Reviewed-by: Li Qiang
> > Also, maybe we could abstract a function to do these discard test?
>
> The structure of the test cases is similar but they vary in different
> places. I'm not sure if this can be abstracted nicely.
>
> > > diff --git a/util/iov.c b/util/iov.c
> > > index 45ef3043ee..efcf04b445 100644
> > > --- a/util/iov.c
> > > +++ b/util/iov.c
> > > @@ -636,14 +636,33 @@ void qemu_iovec_clone(QEMUIOVector *dest, const
> > > QEMUIOVector *src, void *buf)
> > > }
> > > }
> > >
> > > -size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
> > > - size_t bytes)
> > > +void iov_discard_undo(IOVDiscardUndo *undo)
> > > +{
> > > +/* Restore original iovec if it was modified */
> > > +if (undo->modified_iov) {
> > > +*undo->modified_iov = undo->orig;
> > > +}
> > > +}
> > > +
> > > +size_t iov_discard_front_undoable(struct iovec **iov,
> > > + unsigned int *iov_cnt,
> > > + size_t bytes,
> > > + IOVDiscardUndo *undo)
> > > {
> > > size_t total = 0;
> > > struct iovec *cur;
> > >
> > > +if (undo) {
> > > +undo->modified_iov = NULL;
> > > +}
> > > +
> > > for (cur = *iov; *iov_cnt > 0; cur++) {
> > > if (cur->iov_len > bytes) {
> > > +if (undo) {
> > > +undo->modified_iov = cur;
> > > +undo->orig = *cur;
> > > +}
> > > +
> > >
> >
> > Why here we remember the 'cur'? 'cur' is the some of the 'iov'.
> > Maybe we remember the 'iov'. I think we need the undo structure like this:
> >
> > struct IOVUndo {
> > iov **modified_iov;
> > iov *orig;
> > };
> >
> > Then we can remeber the origin 'iov'.
>
> Yes, this could be done but it's not needed (yet?). VirtQueueElement has
> the original struct iovec pointers so adding this would be redundant.
Re: [PATCH] hw: ide: check the pointer before do dma memory unmap
ping!!
Li Qiang 于2020年9月7日周一 上午9:39写道：
>
> Ping!
>
> Li Qiang 于2020年9月1日周二 下午6:34写道：
> >
> > Ping.
> >
> > Li Qiang 于2020年8月15日周六 下午3:21写道：
> > >
> > > In 'map_page' we need to check the return value of
> > > 'dma_memory_map' to ensure the we actully maped something.
> > > Otherwise, we will hit an assert in 'address_space_unmap'.
> > > This is because we can't find the MR with the NULL buffer.
> > > This is the LP#1884693:
> > >
> > > -->https://bugs.launchpad.net/qemu/+bug/1884693
> > >
> > > Reported-by: Alexander Bulekov
> > > Signed-off-by: Li Qiang
> > > ---
> > > hw/ide/ahci.c | 5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > > index 009120f88b..63e9fccdbe 100644
> > > --- a/hw/ide/ahci.c
> > > +++ b/hw/ide/ahci.c
> > > @@ -250,6 +250,11 @@ static void map_page(AddressSpace *as, uint8_t
> > > **ptr, uint64_t addr,
> > > }
> > >
> > > *ptr = dma_memory_map(as, addr, , DMA_DIRECTION_FROM_DEVICE);
> > > +
> > > +if (!*ptr) {
> > > +return;
> > > +}
> > > +
> > > if (len < wanted) {
> > > dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
> > > *ptr = NULL;
> > > --
> > > 2.17.1
> > >
Re: [PATCH] hw: ide: check the pointer before do dma memory unmap
Ping!
Li Qiang 于2020年9月1日周二 下午6:34写道：
>
> Ping.
>
> Li Qiang 于2020年8月15日周六 下午3:21写道：
> >
> > In 'map_page' we need to check the return value of
> > 'dma_memory_map' to ensure the we actully maped something.
> > Otherwise, we will hit an assert in 'address_space_unmap'.
> > This is because we can't find the MR with the NULL buffer.
> > This is the LP#1884693:
> >
> > -->https://bugs.launchpad.net/qemu/+bug/1884693
> >
> > Reported-by: Alexander Bulekov
> > Signed-off-by: Li Qiang
> > ---
> > hw/ide/ahci.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > index 009120f88b..63e9fccdbe 100644
> > --- a/hw/ide/ahci.c
> > +++ b/hw/ide/ahci.c
> > @@ -250,6 +250,11 @@ static void map_page(AddressSpace *as, uint8_t **ptr,
> > uint64_t addr,
> > }
> >
> > *ptr = dma_memory_map(as, addr, , DMA_DIRECTION_FROM_DEVICE);
> > +
> > +if (!*ptr) {
> > +return;
> > +}
> > +
> > if (len < wanted) {
> > dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
> > *ptr = NULL;
> > --
> > 2.17.1
> >
Re: [PATCH 12/13] dma: Let dma_memory_read/write() take MemTxAttrs argument
Philippe Mathieu-Daudé 于2020年9月4日周五 下午11:52写道：
>
> Let devices specify transaction attributes when calling
> dma_memory_read() or dma_memory_write().
>
> Patch created mechanically using spatch with this script:
>
> @@
> expression E1, E2, E3, E4;
> @@
> (
> - dma_memory_read(E1, E2, E3, E4)
> + dma_memory_read(E1, E2, E3, E4, MEMTXATTRS_UNSPECIFIED)
> |
> - dma_memory_write(E1, E2, E3, E4)
> + dma_memory_write(E1, E2, E3, E4, MEMTXATTRS_UNSPECIFIED)
> )
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> include/hw/ppc/spapr_vio.h| 6 --
> include/sysemu/dma.h | 20
> hw/arm/musicpal.c | 13 +++--
> hw/arm/smmu-common.c | 3 ++-
> hw/arm/smmuv3.c | 14 +-
> hw/core/generic-loader.c | 3 ++-
> hw/dma/pl330.c| 12
> hw/dma/sparc32_dma.c | 16 ++--
> hw/dma/xlnx-zynq-devcfg.c | 6 --
> hw/dma/xlnx_dpdma.c | 10 ++
> hw/i386/amd_iommu.c | 16 +---
> hw/i386/intel_iommu.c | 28 +---
> hw/ide/macio.c| 2 +-
> hw/intc/xive.c| 7 ---
> hw/misc/bcm2835_property.c| 3 ++-
> hw/misc/macio/mac_dbdma.c | 10 ++
> hw/net/allwinner-sun8i-emac.c | 21 ++---
> hw/net/ftgmac100.c| 25 -
> hw/net/imx_fec.c | 32
> hw/nvram/fw_cfg.c | 9 ++---
> hw/pci-host/pnv_phb3.c| 5 +++--
> hw/pci-host/pnv_phb3_msi.c| 9 ++---
> hw/pci-host/pnv_phb4.c| 7 ---
> hw/sd/allwinner-sdhost.c | 14 --
> hw/sd/sdhci.c | 35 ++-
> hw/usb/hcd-dwc2.c | 8
> hw/usb/hcd-ehci.c | 6 --
> hw/usb/hcd-ohci.c | 18 +++---
> 28 files changed, 221 insertions(+), 137 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index 6e5c0840248..8168f4fc5a5 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -102,14 +102,16 @@ static inline bool spapr_vio_dma_valid(SpaprVioDevice
> *dev, uint64_t taddr,
> static inline int spapr_vio_dma_read(SpaprVioDevice *dev, uint64_t taddr,
> void *buf, uint32_t size)
> {
> -return (dma_memory_read(>as, taddr, buf, size) != 0) ?
> +return (dma_memory_read(>as, taddr,
> +buf, size, MEMTXATTRS_UNSPECIFIED) != 0) ?
> H_DEST_PARM : H_SUCCESS;
> }
>
> static inline int spapr_vio_dma_write(SpaprVioDevice *dev, uint64_t taddr,
>const void *buf, uint32_t size)
> {
> -return (dma_memory_write(>as, taddr, buf, size) != 0) ?
> +return (dma_memory_write(>as, taddr,
> + buf, size, MEMTXATTRS_UNSPECIFIED) != 0) ?
> H_DEST_PARM : H_SUCCESS;
> }
>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 0695d430119..b9cb9c8944b 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -143,12 +143,14 @@ static inline MemTxResult dma_memory_rw(AddressSpace
> *as, dma_addr_t addr,
> * @addr: address within that address space
> * @buf: buffer with the data transferred
> * @len: length of the data transferred
> + * @attrs: memory transaction attributes
> */
> static inline MemTxResult dma_memory_read(AddressSpace *as, dma_addr_t addr,
> - void *buf, dma_addr_t len)
> + void *buf, dma_addr_t len,
> + MemTxAttrs attrs)
> {
> return dma_memory_rw(as, addr, buf, len,
> - DMA_DIRECTION_TO_DEVICE, MEMTXATTRS_UNSPECIFIED);
> + DMA_DIRECTION_TO_DEVICE, attrs);
> }
>
> /**
> @@ -162,12 +164,14 @@ static inline MemTxResult dma_memory_read(AddressSpace
> *as, dma_addr_t addr,
> * @addr: address within that address space
> * @buf: buffer with the data transferred
> * @len: the number of bytes to write
> + * @attrs: memory transaction attributes
> */
> static inline MemTxResult dma_memory_write(AddressSpace *as, dma_addr_t addr,
> - const void *buf, dma_addr_t len)
> + const void *buf, dma_addr_t len,
> + MemTxAttrs attrs)
> {
> return dma_memory_r
Re: [PATCH 13/13] dma: Let dma_memory_map() take MemTxAttrs argument
Philippe Mathieu-Daudé 于2020年9月4日周五 下午11:52写道：
>
> Let devices specify transaction attributes when calling
> dma_memory_map().
>
> Patch created mechanically using spatch with this script:
>
> @@
> expression E1, E2, E3, E4;
> @@
> - dma_memory_map(E1, E2, E3, E4)
> + dma_memory_map(E1, E2, E3, E4, MEMTXATTRS_UNSPECIFIED)
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> include/hw/pci/pci.h| 3 ++-
> include/sysemu/dma.h| 5 +++--
> dma-helpers.c | 3 ++-
> hw/display/virtio-gpu.c | 8 ++--
> hw/hyperv/vmbus.c | 8 +---
> hw/ide/ahci.c | 9 ++---
> hw/usb/libhw.c | 3 ++-
> hw/virtio/virtio.c | 6 --
> 8 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 0c3217e019c..a221dfb3b08 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -831,7 +831,8 @@ static inline void *pci_dma_map(PCIDevice *dev,
> dma_addr_t addr,
> {
> void *buf;
>
> -buf = dma_memory_map(pci_get_address_space(dev), addr, plen, dir);
> +buf = dma_memory_map(pci_get_address_space(dev), addr, plen, dir,
> + MEMTXATTRS_UNSPECIFIED);
> return buf;
> }
>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index b9cb9c8944b..bb8b0a059f5 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -203,16 +203,17 @@ MemTxResult dma_memory_set(AddressSpace *as, dma_addr_t
> addr,
> * @addr: address within that address space
> * @len: pointer to length of buffer; updated on return
> * @dir: indicates the transfer direction
> + * @attrs: memory attributes
> */
> static inline void *dma_memory_map(AddressSpace *as,
> dma_addr_t addr, dma_addr_t *len,
> - DMADirection dir)
> + DMADirection dir, MemTxAttrs attrs)
> {
> hwaddr xlen = *len;
> void *p;
>
> p = address_space_map(as, addr, , dir == DMA_DIRECTION_FROM_DEVICE,
> - MEMTXATTRS_UNSPECIFIED);
> + attrs);
> *len = xlen;
> return p;
> }
> diff --git a/dma-helpers.c b/dma-helpers.c
> index 6c3b2200f16..0507a6f95b9 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -143,7 +143,8 @@ static void dma_blk_cb(void *opaque, int ret)
> while (dbs->sg_cur_index < dbs->sg->nsg) {
> cur_addr = dbs->sg->sg[dbs->sg_cur_index].base + dbs->sg_cur_byte;
> cur_len = dbs->sg->sg[dbs->sg_cur_index].len - dbs->sg_cur_byte;
> -mem = dma_memory_map(dbs->sg->as, cur_addr, _len, dbs->dir);
> +mem = dma_memory_map(dbs->sg->as, cur_addr, _len, dbs->dir,
> + MEMTXATTRS_UNSPECIFIED);
> /*
> * Make reads deterministic in icount mode. Windows sometimes issues
> * disk read requests with overlapping SGs. It leads
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 5f0dd7c1500..be7f5cdee46 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -648,7 +648,9 @@ int virtio_gpu_create_mapping_iov(VirtIOGPU *g,
> hwaddr len = l;
> (*iov)[i].iov_len = l;
> (*iov)[i].iov_base = dma_memory_map(VIRTIO_DEVICE(g)->dma_as,
> -a, ,
> DMA_DIRECTION_TO_DEVICE);
> +a, ,
> +DMA_DIRECTION_TO_DEVICE,
> +MEMTXATTRS_UNSPECIFIED);
> if (addr) {
> (*addr)[i] = a;
> }
> @@ -1049,7 +1051,9 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque,
> size_t size,
> hwaddr len = res->iov[i].iov_len;
> res->iov[i].iov_base =
> dma_memory_map(VIRTIO_DEVICE(g)->dma_as,
> - res->addrs[i], , DMA_DIRECTION_TO_DEVICE);
> + res->addrs[i], ,
> + DMA_DIRECTION_TO_DEVICE,
> + MEMTXATTRS_UNSPECIFIED);
>
> if (!res->iov[i].iov_base || len != res->iov[i].iov_len) {
> /* Clean up the half-a-mapping we just created... */
> diff --git a/hw/hyperv/vmbus.c b/hw/hyperv/vmbus.c
> index 75af6b83dde..56621d72e5b 100644
> --- a/hw/hyperv/vmbus.c
> +++ b/hw/hyperv/vmbus.c
> @@ -372,7 +372,8 @@ static ssize_t gpadl_iter_io(GpadlIter *iter, void *buf,
> uint32_t len)
Re: [PATCH 11/13] dma: Let dma_memory_rw() take MemTxAttrs argument
Philippe Mathieu-Daudé 于2020年9月4日周五 下午11:53写道：
>
> Let devices specify transaction attributes when calling
> dma_memory_rw().
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> include/hw/pci/pci.h | 3 ++-
> include/sysemu/dma.h | 11 ++-
> dma-helpers.c| 3 ++-
> hw/intc/spapr_xive.c | 3 ++-
> hw/usb/hcd-ohci.c| 10 ++
> 5 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 896cef9ad47..0c3217e019c 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -788,7 +788,8 @@ static inline AddressSpace
> *pci_get_address_space(PCIDevice *dev)
> static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
> void *buf, dma_addr_t len, DMADirection dir)
> {
> -return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> +return dma_memory_rw(pci_get_address_space(dev), addr, buf, len,
> + dir, MEMTXATTRS_UNSPECIFIED);
> }
>
> static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr,
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 59331ec0bd3..0695d430119 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -121,15 +121,15 @@ static inline MemTxResult
> dma_memory_write_relaxed(AddressSpace *as,
> * @buf: buffer with the data transferred
> * @len: the number of bytes to read or write
> * @dir: indicates the transfer direction
> + * @attrs: memory transaction attributes
> */
> static inline MemTxResult dma_memory_rw(AddressSpace *as, dma_addr_t addr,
> void *buf, dma_addr_t len,
> -DMADirection dir)
> +DMADirection dir, MemTxAttrs attrs)
> {
> dma_barrier(as, dir);
>
> -return dma_memory_rw_relaxed(as, addr, buf, len, dir,
> - MEMTXATTRS_UNSPECIFIED);
> +return dma_memory_rw_relaxed(as, addr, buf, len, dir, attrs);
> }
>
> /**
> @@ -147,7 +147,8 @@ static inline MemTxResult dma_memory_rw(AddressSpace *as,
> dma_addr_t addr,
> static inline MemTxResult dma_memory_read(AddressSpace *as, dma_addr_t addr,
>void *buf, dma_addr_t len)
> {
> -return dma_memory_rw(as, addr, buf, len, DMA_DIRECTION_TO_DEVICE);
> +return dma_memory_rw(as, addr, buf, len,
> + DMA_DIRECTION_TO_DEVICE, MEMTXATTRS_UNSPECIFIED);
> }
>
> /**
> @@ -166,7 +167,7 @@ static inline MemTxResult dma_memory_write(AddressSpace
> *as, dma_addr_t addr,
> const void *buf, dma_addr_t len)
> {
> return dma_memory_rw(as, addr, (void *)buf, len,
> - DMA_DIRECTION_FROM_DEVICE);
> + DMA_DIRECTION_FROM_DEVICE, MEMTXATTRS_UNSPECIFIED);
> }
>
> /**
> diff --git a/dma-helpers.c b/dma-helpers.c
> index 6a9735386dc..6c3b2200f16 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -305,7 +305,8 @@ static uint64_t dma_buf_rw(uint8_t *ptr, int32_t len,
> QEMUSGList *sg,
> while (len > 0) {
> ScatterGatherEntry entry = sg->sg[sg_cur_index++];
> int32_t xfer = MIN(len, entry.len);
> -dma_memory_rw(sg->as, entry.base, ptr, xfer, dir);
> +dma_memory_rw(sg->as, entry.base, ptr, xfer, dir,
> + MEMTXATTRS_UNSPECIFIED);
> ptr += xfer;
> len -= xfer;
> resid -= xfer;
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 4bd0d606ba1..dbf73a8bf47 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -1666,7 +1666,8 @@ static target_ulong h_int_esb(PowerPCCPU *cpu,
> mmio_addr = xive->vc_base + xive_source_esb_mgmt(xsrc, lisn) +
> offset;
>
> if (dma_memory_rw(_space_memory, mmio_addr, , 8,
> - (flags & SPAPR_XIVE_ESB_STORE))) {
> + (flags & SPAPR_XIVE_ESB_STORE),
> + MEMTXATTRS_UNSPECIFIED)) {
> qemu_log_mask(LOG_GUEST_ERROR, "XIVE: failed to access ESB @0x%"
>HWADDR_PRIx "\n", mmio_addr);
> return H_HARDWARE;
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 1e6e85e86a8..bac1adf439c 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -586,7 +586,8 @@ static int ohci_copy_td(OHCIState *ohci, struct ohci_td
> *td,
> if (n > len)
> n = len;
>
> -if (dma_memory_rw(ohci->as, ptr + ohci->
Re: [PATCH 10/13] dma: Let dma_memory_rw_relaxed() take MemTxAttrs argument
Philippe Mathieu-Daudé 于2020年9月4日周五 下午11:53写道：
>
> We will add the MemTxAttrs argument to dma_memory_rw() in
> the next commit. Since dma_memory_rw_relaxed() is only used
> by dma_memory_rw(), modify it first in a separate commit to
> keep the next commit easier to review.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> include/sysemu/dma.h | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index d0381f9ae9b..59331ec0bd3 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -83,9 +83,10 @@ static inline bool dma_memory_valid(AddressSpace *as,
> static inline MemTxResult dma_memory_rw_relaxed(AddressSpace *as,
> dma_addr_t addr,
> void *buf, dma_addr_t len,
> -DMADirection dir)
> +DMADirection dir,
> +MemTxAttrs attrs)
> {
> -return address_space_rw(as, addr, MEMTXATTRS_UNSPECIFIED,
> +return address_space_rw(as, addr, attrs,
> buf, len, dir == DMA_DIRECTION_FROM_DEVICE);
> }
>
> @@ -93,7 +94,9 @@ static inline MemTxResult
> dma_memory_read_relaxed(AddressSpace *as,
>dma_addr_t addr,
>void *buf, dma_addr_t len)
> {
> -return dma_memory_rw_relaxed(as, addr, buf, len,
> DMA_DIRECTION_TO_DEVICE);
> +return dma_memory_rw_relaxed(as, addr, buf, len,
> + DMA_DIRECTION_TO_DEVICE,
> + MEMTXATTRS_UNSPECIFIED);
> }
>
> static inline MemTxResult dma_memory_write_relaxed(AddressSpace *as,
> @@ -102,7 +105,8 @@ static inline MemTxResult
> dma_memory_write_relaxed(AddressSpace *as,
> dma_addr_t len)
> {
> return dma_memory_rw_relaxed(as, addr, (void *)buf, len,
> - DMA_DIRECTION_FROM_DEVICE);
> + DMA_DIRECTION_FROM_DEVICE,
> + MEMTXATTRS_UNSPECIFIED);
> }
>
> /**
> @@ -124,7 +128,8 @@ static inline MemTxResult dma_memory_rw(AddressSpace *as,
> dma_addr_t addr,
> {
> dma_barrier(as, dir);
>
> -return dma_memory_rw_relaxed(as, addr, buf, len, dir);
> +return dma_memory_rw_relaxed(as, addr, buf, len, dir,
> + MEMTXATTRS_UNSPECIFIED);
> }
>
> /**
> --
> 2.26.2
>
>
Re: [PATCH 09/13] dma: Let dma_memory_set() take MemTxAttrs argument
Philippe Mathieu-Daudé 于2020年9月4日周五 下午11:49写道：
>
> Let devices specify transaction attributes when calling
> dma_memory_set().
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> include/hw/ppc/spapr_vio.h | 3 ++-
> include/sysemu/dma.h | 3 ++-
> dma-helpers.c | 5 ++---
> hw/nvram/fw_cfg.c | 3 ++-
> 4 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index f134f6cf574..6e5c0840248 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -116,7 +116,8 @@ static inline int spapr_vio_dma_write(SpaprVioDevice
> *dev, uint64_t taddr,
> static inline int spapr_vio_dma_set(SpaprVioDevice *dev, uint64_t taddr,
> uint8_t c, uint32_t size)
> {
> -return (dma_memory_set(>as, taddr, c, size) != 0) ?
> +return (dma_memory_set(>as, taddr,
> + c, size, MEMTXATTRS_UNSPECIFIED) != 0) ?
> H_DEST_PARM : H_SUCCESS;
> }
>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index b322aa5947b..d0381f9ae9b 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -175,9 +175,10 @@ static inline MemTxResult dma_memory_write(AddressSpace
> *as, dma_addr_t addr,
> * @addr: address within that address space
> * @c: constant byte to fill the memory
> * @len: the number of bytes to fill with the constant byte
> + * @attrs: memory transaction attributes
> */
> MemTxResult dma_memory_set(AddressSpace *as, dma_addr_t addr,
> - uint8_t c, dma_addr_t len);
> + uint8_t c, dma_addr_t len, MemTxAttrs attrs);
>
> /**
> * address_space_map: Map a physical memory region into a DMA controller
> diff --git a/dma-helpers.c b/dma-helpers.c
> index 4a9e37d6d06..6a9735386dc 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -19,7 +19,7 @@
> /* #define DEBUG_IOMMU */
>
> MemTxResult dma_memory_set(AddressSpace *as, dma_addr_t addr,
> - uint8_t c, dma_addr_t len)
> + uint8_t c, dma_addr_t len, MemTxAttrs attrs)
> {
> dma_barrier(as, DMA_DIRECTION_FROM_DEVICE);
>
> @@ -31,8 +31,7 @@ MemTxResult dma_memory_set(AddressSpace *as, dma_addr_t
> addr,
> memset(fillbuf, c, FILLBUF_SIZE);
> while (len > 0) {
> l = len < FILLBUF_SIZE ? len : FILLBUF_SIZE;
> -error |= address_space_write(as, addr, MEMTXATTRS_UNSPECIFIED,
> - fillbuf, l);
> +error |= address_space_write(as, addr, attrs, fillbuf, l);
> len -= l;
> addr += l;
> }
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index f3a4728288e..a15de06a10c 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -397,7 +397,8 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
> * tested before.
> */
> if (read) {
> -if (dma_memory_set(s->dma_as, dma.address, 0, len)) {
> +if (dma_memory_set(s->dma_as, dma.address, 0, len,
> + MEMTXATTRS_UNSPECIFIED)) {
> dma.control |= FW_CFG_DMA_CTL_ERROR;
> }
> }
> --
> 2.26.2
>
>
Re: [PATCH 08/13] dma: Let dma_memory_valid() take MemTxAttrs argument
Philippe Mathieu-Daudé 于2020年9月4日周五 下午11:48写道：
>
> Let devices specify transaction attributes when calling
> dma_memory_valid().
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> include/hw/ppc/spapr_vio.h | 2 +-
> include/sysemu/dma.h | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index bed7df60e35..f134f6cf574 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -96,7 +96,7 @@ static inline void spapr_vio_irq_pulse(SpaprVioDevice *dev)
> static inline bool spapr_vio_dma_valid(SpaprVioDevice *dev, uint64_t taddr,
> uint32_t size, DMADirection dir)
> {
> -return dma_memory_valid(>as, taddr, size, dir);
> +return dma_memory_valid(>as, taddr, size, dir,
> MEMTXATTRS_UNSPECIFIED);
> }
>
> static inline int spapr_vio_dma_read(SpaprVioDevice *dev, uint64_t taddr,
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index f4ade067a46..b322aa5947b 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -73,11 +73,11 @@ static inline void dma_barrier(AddressSpace *as,
> DMADirection dir)
> * dma_memory_{read,write}() and check for errors */
> static inline bool dma_memory_valid(AddressSpace *as,
> dma_addr_t addr, dma_addr_t len,
> -DMADirection dir)
> +DMADirection dir, MemTxAttrs attrs)
> {
> return address_space_access_valid(as, addr, len,
>dir == DMA_DIRECTION_FROM_DEVICE,
> - MEMTXATTRS_UNSPECIFIED);
> + attrs);
> }
>
> static inline MemTxResult dma_memory_rw_relaxed(AddressSpace *as,
> --
> 2.26.2
>
>
Re: [PATCH 06/13] dma: Let dma_memory_read() propagate MemTxResult
Philippe Mathieu-Daudé 于2020年9月4日周五 下午11:50写道：
>
> dma_memory_rw_relaxed() returns a MemTxResult type.
> Do not discard it, return it to the caller.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> include/sysemu/dma.h | 21 +
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 661d7d0ca88..2961a96ad67 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -89,8 +89,9 @@ static inline MemTxResult
> dma_memory_rw_relaxed(AddressSpace *as,
> buf, len, dir == DMA_DIRECTION_FROM_DEVICE);
> }
>
> -static inline int dma_memory_read_relaxed(AddressSpace *as, dma_addr_t addr,
> - void *buf, dma_addr_t len)
> +static inline MemTxResult dma_memory_read_relaxed(AddressSpace *as,
> + dma_addr_t addr,
> + void *buf, dma_addr_t len)
> {
> return dma_memory_rw_relaxed(as, addr, buf, len,
> DMA_DIRECTION_TO_DEVICE);
> }
> @@ -124,8 +125,20 @@ static inline MemTxResult dma_memory_rw(AddressSpace
> *as, dma_addr_t addr,
> return dma_memory_rw_relaxed(as, addr, buf, len, dir);
> }
>
> -static inline int dma_memory_read(AddressSpace *as, dma_addr_t addr,
> - void *buf, dma_addr_t len)
> +/**
> + * dma_memory_read: Read from an address space from DMA controller.
> + *
> + * Return a MemTxResult indicating whether the operation succeeded
> + * or failed (eg unassigned memory, device rejected the transaction,
> + * IOMMU fault). Called within RCU critical section.
> + *
> + * @as: #AddressSpace to be accessed
> + * @addr: address within that address space
> + * @buf: buffer with the data transferred
> + * @len: length of the data transferred
> + */
> +static inline MemTxResult dma_memory_read(AddressSpace *as, dma_addr_t addr,
> + void *buf, dma_addr_t len)
> {
> return dma_memory_rw(as, addr, buf, len, DMA_DIRECTION_TO_DEVICE);
> }
> --
> 2.26.2
>
>
Re: [PATCH 07/13] dma: Let dma_memory_write() propagate MemTxResult
Philippe Mathieu-Daudé 于2020年9月4日周五 下午11:46写道：
>
> dma_memory_rw_relaxed() returns a MemTxResult type.
> Do not discard it, return it to the caller.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> include/sysemu/dma.h | 22 ++
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 2961a96ad67..f4ade067a46 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -96,8 +96,10 @@ static inline MemTxResult
> dma_memory_read_relaxed(AddressSpace *as,
> return dma_memory_rw_relaxed(as, addr, buf, len,
> DMA_DIRECTION_TO_DEVICE);
> }
>
> -static inline int dma_memory_write_relaxed(AddressSpace *as, dma_addr_t addr,
> - const void *buf, dma_addr_t len)
> +static inline MemTxResult dma_memory_write_relaxed(AddressSpace *as,
> + dma_addr_t addr,
> + const void *buf,
> + dma_addr_t len)
> {
> return dma_memory_rw_relaxed(as, addr, (void *)buf, len,
> DMA_DIRECTION_FROM_DEVICE);
> @@ -143,8 +145,20 @@ static inline MemTxResult dma_memory_read(AddressSpace
> *as, dma_addr_t addr,
> return dma_memory_rw(as, addr, buf, len, DMA_DIRECTION_TO_DEVICE);
> }
>
> -static inline int dma_memory_write(AddressSpace *as, dma_addr_t addr,
> - const void *buf, dma_addr_t len)
> +/**
> + * address_space_write: Write to address space from DMA controller.
> + *
> + * Return a MemTxResult indicating whether the operation succeeded
> + * or failed (eg unassigned memory, device rejected the transaction,
> + * IOMMU fault).
> + *
> + * @as: #AddressSpace to be accessed
> + * @addr: address within that address space
> + * @buf: buffer with the data transferred
> + * @len: the number of bytes to write
> + */
> +static inline MemTxResult dma_memory_write(AddressSpace *as, dma_addr_t addr,
> + const void *buf, dma_addr_t len)
> {
> return dma_memory_rw(as, addr, (void *)buf, len,
> DMA_DIRECTION_FROM_DEVICE);
> --
> 2.26.2
>
>
Re: [PATCH 05/13] dma: Let dma_memory_rw() propagate MemTxResult
Philippe Mathieu-Daudé 于2020年9月4日周五 下午11:46写道：
>
> address_space_rw() returns a MemTxResult type.
> Do not discard it, return it to the caller.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> include/sysemu/dma.h | 30 ++
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index ad8a3f82f47..661d7d0ca88 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -80,12 +80,13 @@ static inline bool dma_memory_valid(AddressSpace *as,
>MEMTXATTRS_UNSPECIFIED);
> }
>
> -static inline int dma_memory_rw_relaxed(AddressSpace *as, dma_addr_t addr,
> -void *buf, dma_addr_t len,
> -DMADirection dir)
> +static inline MemTxResult dma_memory_rw_relaxed(AddressSpace *as,
> +dma_addr_t addr,
> +void *buf, dma_addr_t len,
> +DMADirection dir)
> {
> -return (bool)address_space_rw(as, addr, MEMTXATTRS_UNSPECIFIED,
> - buf, len, dir ==
> DMA_DIRECTION_FROM_DEVICE);
> +return address_space_rw(as, addr, MEMTXATTRS_UNSPECIFIED,
> +buf, len, dir == DMA_DIRECTION_FROM_DEVICE);
> }
>
> static inline int dma_memory_read_relaxed(AddressSpace *as, dma_addr_t addr,
> @@ -101,9 +102,22 @@ static inline int dma_memory_write_relaxed(AddressSpace
> *as, dma_addr_t addr,
> DMA_DIRECTION_FROM_DEVICE);
> }
>
> -static inline int dma_memory_rw(AddressSpace *as, dma_addr_t addr,
> -void *buf, dma_addr_t len,
> -DMADirection dir)
> +/**
> + * dma_memory_rw: Read from or write to an address space from DMA controller.
> + *
> + * Return a MemTxResult indicating whether the operation succeeded
> + * or failed (eg unassigned memory, device rejected the transaction,
> + * IOMMU fault).
> + *
> + * @as: #AddressSpace to be accessed
> + * @addr: address within that address space
> + * @buf: buffer with the data transferred
> + * @len: the number of bytes to read or write
> + * @dir: indicates the transfer direction
> + */
> +static inline MemTxResult dma_memory_rw(AddressSpace *as, dma_addr_t addr,
> +void *buf, dma_addr_t len,
> +DMADirection dir)
> {
> dma_barrier(as, dir);
>
> --
> 2.26.2
>
>
Re: [PATCH 04/13] dma: Let dma_memory_set() propagate MemTxResult
Philippe Mathieu-Daudé 于2020年9月4日周五 下午11:47写道：
>
> address_space_write() returns a MemTxResult type.
> Do not discard it, return it to the caller.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> include/sysemu/dma.h | 15 ++-
> dma-helpers.c| 7 ---
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 19bc9ad1b69..ad8a3f82f47 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -123,7 +123,20 @@ static inline int dma_memory_write(AddressSpace *as,
> dma_addr_t addr,
> DMA_DIRECTION_FROM_DEVICE);
> }
>
> -int dma_memory_set(AddressSpace *as, dma_addr_t addr, uint8_t c, dma_addr_t
> len);
> +/**
> + * dma_memory_set: Fill memory with a constant byte from DMA controller.
> + *
> + * Return a MemTxResult indicating whether the operation succeeded
> + * or failed (eg unassigned memory, device rejected the transaction,
> + * IOMMU fault).
> + *
> + * @as: #AddressSpace to be accessed
> + * @addr: address within that address space
> + * @c: constant byte to fill the memory
> + * @len: the number of bytes to fill with the constant byte
> + */
> +MemTxResult dma_memory_set(AddressSpace *as, dma_addr_t addr,
> + uint8_t c, dma_addr_t len);
>
> /**
> * address_space_map: Map a physical memory region into a DMA controller
> diff --git a/dma-helpers.c b/dma-helpers.c
> index 41ef24a63b6..4a9e37d6d06 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -1,7 +1,7 @@
> /*
> * DMA helper functions
> *
> - * Copyright (c) 2009 Red Hat
> + * Copyright (c) 2009,2020 Red Hat
> *
> * This work is licensed under the terms of the GNU General Public License
> * (GNU GPL), version 2 or later.
> @@ -18,14 +18,15 @@
>
> /* #define DEBUG_IOMMU */
>
> -int dma_memory_set(AddressSpace *as, dma_addr_t addr, uint8_t c, dma_addr_t
> len)
> +MemTxResult dma_memory_set(AddressSpace *as, dma_addr_t addr,
> + uint8_t c, dma_addr_t len)
> {
> dma_barrier(as, DMA_DIRECTION_FROM_DEVICE);
>
> #define FILLBUF_SIZE 512
> uint8_t fillbuf[FILLBUF_SIZE];
> int l;
> -bool error = false;
> +MemTxResult error = MEMTX_OK;
>
> memset(fillbuf, c, FILLBUF_SIZE);
> while (len > 0) {
> --
> 2.26.2
>
>
Re: [PATCH 01/13] pci: pass along the return value of dma_memory_rw
Philippe Mathieu-Daudé 于2020年9月4日周五 下午11:47写道：
>
> From: Klaus Jensen
>
> Some might actually care about the return value of dma_memory_rw. So
> let us pass it along instead of ignoring it.
>
> There are no existing users of the return value, so this patch should be
> safe.
>
> Signed-off-by: Klaus Jensen
> Reviewed-by: Philippe Mathieu-Daudé
> Reviewed-by: Michael S. Tsirkin
> Acked-by: Keith Busch
> Message-Id: <20191011070141.188713-2-...@irrelevant.dk>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> include/hw/pci/pci.h | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 4ca7258b5b7..896cef9ad47 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -788,8 +788,7 @@ static inline AddressSpace
> *pci_get_address_space(PCIDevice *dev)
> static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
> void *buf, dma_addr_t len, DMADirection dir)
> {
> -dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> -return 0;
> +return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> }
>
> static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr,
> --
> 2.26.2
>
>
Re: [RFC PATCH 00/12] hw: Forbid DMA write accesses to MMIO regions
Philippe Mathieu-Daudé 于2020年9月3日周四 下午7:09写道： > > Hi, > > I'm not suppose to work on this but I couldn't sleep so kept > wondering about this problem the whole night and eventually > woke up to write this quickly, so comments are scarce, sorry. > > The first part is obvious anyway, simply pass MemTxAttrs argument. > > The main patch is: > "exec/memattrs: Introduce MemTxAttrs::direct_access field". > This way we can restrict accesses to ROM/RAM by setting the > 'direct_access' field. Illegal accesses return MEMTX_BUS_ERROR. > > Next patch restrict PCI DMA accesses by setting the direct_access > field. > > Finally we add an assertion for any DMA write access to indirect > memory to kill a class of bug recently found by Alexander while > fuzzing. > Hi Philippe, I have reviewed your patches. Your patch just deny the DMA write to MMIO for PCI device. 1. The DMA write to MMIO is allowed for P2P. Unconditionally deny is not right I think. Maybe we can add some flag for the device as property so the device can indicate whether it supports DMA to MMIO. But this method needs define we should apply the restrict to DMA to MMIO initiator or target. If the target, we need to find the target PCI device. 2. I think the MMIO read maybe also suffers the reentrant issue If the MMIO read handler does complicated work. 3. As your patch just consider the PCI case. This reentrant is quite complicated if we consider the no-PCI the qemu_irq cases. I agree to address the PCI cases first. Thanks, Li Qiang > Regards, > > Phil. > > Klaus Jensen (1): > pci: pass along the return value of dma_memory_rw > > Philippe Mathieu-Daudé (11): > dma: Let dma_memory_valid() take MemTxAttrs argument > dma: Let dma_memory_set() take MemTxAttrs argument > dma: Let dma_memory_rw_relaxed() take MemTxAttrs argument > dma: Let dma_memory_rw() take MemTxAttrs argument > dma: Let dma_memory_read/write() take MemTxAttrs argument > dma: Let dma_memory_map() take MemTxAttrs argument > docs/devel/loads-stores: Add regexp for DMA functions > dma: Let load/store DMA functions take MemTxAttrs argument > exec/memattrs: Introduce MemTxAttrs::direct_access field > hw/pci: Only allow PCI slave devices to write to direct memory > dma: Assert when device writes to indirect memory (such MMIO regions) > > docs/devel/loads-stores.rst | 2 ++ > include/exec/memattrs.h | 3 ++ > include/hw/pci/pci.h | 21 ++--- > include/hw/ppc/spapr_vio.h| 26 +-- > include/sysemu/dma.h | 59 +-- > dma-helpers.c | 12 --- > exec.c| 8 + > hw/arm/musicpal.c | 13 > hw/arm/smmu-common.c | 3 +- > hw/arm/smmuv3.c | 14 ++--- > hw/core/generic-loader.c | 3 +- > hw/display/virtio-gpu.c | 8 +++-- > hw/dma/pl330.c| 12 --- > hw/dma/sparc32_dma.c | 16 ++ > hw/dma/xlnx-zynq-devcfg.c | 6 ++-- > hw/dma/xlnx_dpdma.c | 10 +++--- > hw/hyperv/vmbus.c | 8 +++-- > hw/i386/amd_iommu.c | 16 +- > hw/i386/intel_iommu.c | 28 ++--- > hw/ide/ahci.c | 9 -- > hw/ide/macio.c| 2 +- > hw/intc/pnv_xive.c| 7 +++-- > hw/intc/spapr_xive.c | 3 +- > hw/intc/xive.c| 7 +++-- > hw/misc/bcm2835_property.c| 3 +- > hw/misc/macio/mac_dbdma.c | 10 +++--- > hw/net/allwinner-sun8i-emac.c | 21 - > hw/net/ftgmac100.c| 25 +-- > hw/net/imx_fec.c | 32 --- > hw/nvram/fw_cfg.c | 16 ++ > hw/pci-host/pnv_phb3.c| 5 +-- > hw/pci-host/pnv_phb3_msi.c| 9 -- > hw/pci-host/pnv_phb4.c| 7 +++-- > hw/sd/allwinner-sdhost.c | 14 + > hw/sd/sdhci.c | 35 + > hw/usb/hcd-dwc2.c | 8 ++--- > hw/usb/hcd-ehci.c | 6 ++-- > hw/usb/hcd-ohci.c | 28 ++--- > hw/usb/libhw.c| 3 +- > hw/virtio/virtio.c| 6 ++-- > trace-events | 1 + > 41 files changed, 334 insertions(+), 191 deletions(-) > > -- > 2.26.2 > >
Re: [PATCH v2 09/10] block/file-posix: fix a possible undefined behavior
Pan Nengyuan 于2020年8月31日周一 下午3:21写道：
>
> local_err is not initialized to NULL, it will cause a assert error as below:
> qemu/util/error.c:59: error_setv: Assertion `*errp == NULL' failed.
>
> Fixes: c6447510690
> Reported-by: Euler Robot
> Signed-off-by: Pan Nengyuan
> Reviewed-by: Stefano Garzarella
Reviewed-by: Li Qiang
> ---
> Cc: Kevin Wolf
> Cc: Max Reitz
> Cc: Aarushi Mehta
> Cc: qemu-block@nongnu.org
> ---
> - V2: no changes in v2.
> ---
> block/file-posix.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 9a00d4190a..697a7d9eea 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2113,7 +2113,7 @@ static void raw_aio_attach_aio_context(BlockDriverState
> *bs,
> #endif
> #ifdef CONFIG_LINUX_IO_URING
> if (s->use_linux_io_uring) {
> -Error *local_err;
> +Error *local_err = NULL;
> if (!aio_setup_linux_io_uring(new_context, _err)) {
> error_reportf_err(local_err, "Unable to use linux io_uring, "
> "falling back to thread pool: ");
> --
> 2.18.2
>
>
Re: [PATCH] hw: ide: check the pointer before do dma memory unmap
Ping.
Li Qiang 于2020年8月15日周六 下午3:21写道：
>
> In 'map_page' we need to check the return value of
> 'dma_memory_map' to ensure the we actully maped something.
> Otherwise, we will hit an assert in 'address_space_unmap'.
> This is because we can't find the MR with the NULL buffer.
> This is the LP#1884693:
>
> -->https://bugs.launchpad.net/qemu/+bug/1884693
>
> Reported-by: Alexander Bulekov
> Signed-off-by: Li Qiang
> ---
> hw/ide/ahci.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 009120f88b..63e9fccdbe 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -250,6 +250,11 @@ static void map_page(AddressSpace *as, uint8_t **ptr,
> uint64_t addr,
> }
>
> *ptr = dma_memory_map(as, addr, , DMA_DIRECTION_FROM_DEVICE);
> +
> +if (!*ptr) {
> +return;
> +}
> +
> if (len < wanted) {
> dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
> *ptr = NULL;
> --
> 2.17.1
>
Re: [PATCH v2 1/2] util/hexdump: Convert to take a void pointer argument
Philippe Mathieu-Daudé 于2020年8月23日周日 上午2:11写道：
>
> Most uses of qemu_hexdump() do not take an array of char
> as input, forcing use of cast. Since we can use this
> helper to dump any kind of buffer, use a pointer to void
> argument instead.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> Since v1:
> - renamed argument 'bufptr' (Peter Maydell)
> ---
> include/qemu-common.h| 3 ++-
> hw/dma/xlnx_dpdma.c | 2 +-
> hw/net/fsl_etsec/etsec.c | 2 +-
> hw/sd/sd.c | 2 +-
> hw/usb/redirect.c| 2 +-
> net/colo-compare.c | 12 ++--
> net/net.c| 2 +-
> util/hexdump.c | 4 +++-
> 8 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index bb9496bd80f..6834883822f 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -138,7 +138,8 @@ int os_parse_cmd_args(int index, const char *optarg);
> * Hexdump a buffer to a file. An optional string prefix is added to every
> line
> */
>
> -void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t
> size);
> +void qemu_hexdump(const void *bufptr, FILE *fp,
> + const char *prefix, size_t size);
>
> /*
> * helper to parse debug environment variables
> diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
> index b40c897de2c..d75a8069426 100644
> --- a/hw/dma/xlnx_dpdma.c
> +++ b/hw/dma/xlnx_dpdma.c
> @@ -388,7 +388,7 @@ static void xlnx_dpdma_dump_descriptor(DPDMADescriptor
> *desc)
> {
> if (DEBUG_DPDMA) {
> qemu_log("DUMP DESCRIPTOR:\n");
> -qemu_hexdump((char *)desc, stdout, "", sizeof(DPDMADescriptor));
> +qemu_hexdump(desc, stdout, "", sizeof(DPDMADescriptor));
> }
> }
>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 7035cf4eb97..c817a28decd 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -357,7 +357,7 @@ static ssize_t etsec_receive(NetClientState *nc,
>
> #if defined(HEX_DUMP)
> fprintf(stderr, "%s receive size:%zd\n", nc->name, size);
> -qemu_hexdump((void *)buf, stderr, "", size);
> +qemu_hexdump(buf, stderr, "", size);
> #endif
> /* Flush is unnecessary as are already in receiving path */
> etsec->need_flush = false;
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index fad9cf1ee7a..190e4cf1232 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1781,7 +1781,7 @@ send_response:
> }
>
> #ifdef DEBUG_SD
> -qemu_hexdump((const char *)response, stderr, "Response", rsplen);
> +qemu_hexdump(response, stderr, "Response", rsplen);
> #endif
>
> return rsplen;
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 417a60a2e68..09edb0d81c0 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -240,7 +240,7 @@ static void usbredir_log_data(USBRedirDevice *dev, const
> char *desc,
> if (dev->debug < usbredirparser_debug_data) {
> return;
> }
> -qemu_hexdump((char *)data, stderr, desc, len);
> +qemu_hexdump(data, stderr, desc, len);
> }
>
> /*
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 2c20de1537d..550272b3baa 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -494,9 +494,9 @@ sec:
> g_queue_push_head(>secondary_list, spkt);
>
> if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> -qemu_hexdump((char *)ppkt->data, stderr,
> +qemu_hexdump(ppkt->data, stderr,
> "colo-compare ppkt", ppkt->size);
> -qemu_hexdump((char *)spkt->data, stderr,
> +qemu_hexdump(spkt->data, stderr,
> "colo-compare spkt", spkt->size);
> }
>
> @@ -535,9 +535,9 @@ static int colo_packet_compare_udp(Packet *spkt, Packet
> *ppkt)
> trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
> trace_colo_compare_udp_miscompare("Secondary pkt size", spkt->size);
> if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> -qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> +qemu_hexdump(ppkt->data, stderr, "colo-compare pri pkt",
> ppkt->size);
> -qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> +qemu_hexdump(spkt->data
Re: [PATCH 09/12] blockdev: Fix a memleak in drive_backup_prepare()
Pan Nengyuan 于2020年8月14日周五 下午6:54写道：
>
> 'local_err' seems forgot to propagate in error path, it'll cause
> a memleak. Fix it.
>
> Reported-by: Euler Robot
> Signed-off-by: Pan Nengyuan
Reviewed-by: Li Qiang
> ---
> Cc: Kevin Wolf
> Cc: Max Reitz
> Cc: Markus Armbruster
> Cc: qemu-block@nongnu.org
> ---
> blockdev.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index 3848a9c8ab..842ac289c1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1801,6 +1801,7 @@ static void drive_backup_prepare(BlkActionState
> *common, Error **errp)
> if (set_backing_hd) {
> bdrv_set_backing_hd(target_bs, source, _err);
> if (local_err) {
> +error_propagate(errp, local_err);
> goto unref;
> }
> }
> --
> 2.18.2
>
>
Re: [PATCH 10/12] block/file-posix: fix a possible undefined behavior
Pan Nengyuan 于2020年8月14日周五 下午6:32写道：
>
> local_err is not initialized to NULL, it will cause a assert error as below:
> qemu/util/error.c:59: error_setv: Assertion `*errp == NULL' failed.
>
> Fixes: c6447510690
> Reported-by: Euler Robot
> Signed-off-by: Pan Nengyuan
Reviewed-by: Li Qiang
> ---
> Cc: Kevin Wolf
> Cc: Max Reitz
> Cc: Aarushi Mehta
> Cc: qemu-block@nongnu.org
> ---
> block/file-posix.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 9a00d4190a..697a7d9eea 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2113,7 +2113,7 @@ static void raw_aio_attach_aio_context(BlockDriverState
> *bs,
> #endif
> #ifdef CONFIG_LINUX_IO_URING
> if (s->use_linux_io_uring) {
> -Error *local_err;
> +Error *local_err = NULL;
> if (!aio_setup_linux_io_uring(new_context, _err)) {
> error_reportf_err(local_err, "Unable to use linux io_uring, "
> "falling back to thread pool: ");
> --
> 2.18.2
>
>
Re: [PATCH 0/2] Fix the assert failure in scsi_dma_complete
Paolo Bonzini 于2020年8月18日周二 上午1:05写道： > On 15/08/20 16:19, Li Qiang wrote: > > Currently in 'megasas_map_sgl' when 'iov_count=0' will just return > > success however the 'cmd' doens't contain any iov. This will cause > > the assert in 'scsi_dma_complete' failed. This is because in > > 'dma_blk_cb' the 'dbs->sg_cur_index == dbs->sg->nsg' will be true > > and just call 'dma_complete'. However now there is no aiocb returned. > > > > This is the LP#1878263: > > > > -->https://bugs.launchpad.net/qemu/+bug/1878263 > > > > To solve this we will consider the 'iov_count=0' is an error. > > In the first patch, I uses -1 to indicate an error and in the second > > patch I consider 'iov_count=0' is an error. > > > > Li Qiang (2): > > hw: megasas: return -1 when 'megasas_map_sgl' fails > > hw: megasas: consider 'iov_count=0' is an error in megasas_map_sgl > > > > hw/scsi/megasas.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > Queued, thanks. But do you have a qtest for this? > Okay, I will cook a qtest for this recently. Thanks, Li Qiang > > Paolo > >
Re: [PATCH 3/3] virtio-crypto: don't modify elem->in/out_sg
Stefan Hajnoczi 于2020年8月12日周三 下午6:51写道：
> A number of iov_discard_front/back() operations are made by
> virtio-crypto. The elem->in/out_sg iovec arrays are modified by these
> operations, resulting virtqueue_unmap_sg() calls on different addresses
> than were originally mapped.
>
> This is problematic because dirty memory may not be logged correctly,
> MemoryRegion refcounts may be leaked, and the non-RAM bounce buffer can
> be leaked.
>
> Take a copy of the elem->in/out_sg arrays so that the originals are
> preserved. The iov_discard_undo() API could be used instead (with better
> performance) but requires careful auditing of the code, so do the simple
> thing instead.
>
> Signed-off-by: Stefan Hajnoczi
>
virtio-net also uses this method.
Reviewed-by: Li Qiang
> ---
> hw/virtio/virtio-crypto.c | 17 ++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> index 6da12e315f..54f9bbb789 100644
> --- a/hw/virtio/virtio-crypto.c
> +++ b/hw/virtio/virtio-crypto.c
> @@ -228,6 +228,8 @@ static void virtio_crypto_handle_ctrl(VirtIODevice
> *vdev, VirtQueue *vq)
> size_t s;
>
> for (;;) {
> +g_autofree struct iovec *out_iov_copy = NULL;
> +
> elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> if (!elem) {
> break;
> @@ -240,9 +242,12 @@ static void virtio_crypto_handle_ctrl(VirtIODevice
> *vdev, VirtQueue *vq)
> }
>
> out_num = elem->out_num;
> -out_iov = elem->out_sg;
> +out_iov_copy = g_memdup(elem->out_sg, sizeof(out_iov[0]) *
> out_num);
> +out_iov = out_iov_copy;
> +
> in_num = elem->in_num;
> in_iov = elem->in_sg;
> +
> if (unlikely(iov_to_buf(out_iov, out_num, 0, , sizeof(ctrl))
> != sizeof(ctrl))) {
> virtio_error(vdev, "virtio-crypto request ctrl_hdr too
> short");
> @@ -582,6 +587,8 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
> int queue_index =
> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
> struct virtio_crypto_op_data_req req;
> int ret;
> +g_autofree struct iovec *in_iov_copy = NULL;
> +g_autofree struct iovec *out_iov_copy = NULL;
> struct iovec *in_iov;
> struct iovec *out_iov;
> unsigned in_num;
> @@ -598,9 +605,13 @@ virtio_crypto_handle_request(VirtIOCryptoReq *request)
> }
>
> out_num = elem->out_num;
> -out_iov = elem->out_sg;
> +out_iov_copy = g_memdup(elem->out_sg, sizeof(out_iov[0]) * out_num);
> +out_iov = out_iov_copy;
> +
> in_num = elem->in_num;
> -in_iov = elem->in_sg;
> +in_iov_copy = g_memdup(elem->in_sg, sizeof(in_iov[0]) * in_num);
> +in_iov = in_iov_copy;
> +
> if (unlikely(iov_to_buf(out_iov, out_num, 0, , sizeof(req))
> != sizeof(req))) {
> virtio_error(vdev, "virtio-crypto request outhdr too short");
> --
> 2.26.2
>
>
Re: [PATCH 1/3] util/iov: add iov_discard_undo()
gt; index 45ef3043ee..efcf04b445 100644
> --- a/util/iov.c
> +++ b/util/iov.c
> @@ -636,14 +636,33 @@ void qemu_iovec_clone(QEMUIOVector *dest, const
> QEMUIOVector *src, void *buf)
> }
> }
>
> -size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
> - size_t bytes)
> +void iov_discard_undo(IOVDiscardUndo *undo)
> +{
> +/* Restore original iovec if it was modified */
> +if (undo->modified_iov) {
> + *undo->modified_iov = undo->orig;
> +}
> +}
> +
> +size_t iov_discard_front_undoable(struct iovec **iov,
> + unsigned int *iov_cnt,
> + size_t bytes,
> + IOVDiscardUndo *undo)
> {
> size_t total = 0;
> struct iovec *cur;
>
> +if (undo) {
> +undo->modified_iov = NULL;
> +}
> +
> for (cur = *iov; *iov_cnt > 0; cur++) {
> if (cur->iov_len > bytes) {
> +if (undo) {
> +undo->modified_iov = cur;
> +undo->orig = *cur;
> +}
> +
>
Why here we remember the 'cur'? 'cur' is the some of the 'iov'.
Maybe we remember the 'iov'. I think we need the undo structure like this:
struct IOVUndo {
iov **modified_iov;
iov *orig;
};
Then we can remeber the origin 'iov'.
Thanks,
Li Qiang
> cur->iov_base += bytes;
> cur->iov_len -= bytes;
> total += bytes;
> @@ -659,12 +678,24 @@ size_t iov_discard_front(struct iovec **iov,
> unsigned int *iov_cnt,
> return total;
> }
>
> -size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
> -size_t bytes)
> +size_t iov_discard_front(struct iovec **iov, unsigned int *iov_cnt,
> + size_t bytes)
> +{
> +return iov_discard_front_undoable(iov, iov_cnt, bytes, NULL);
> +}
> +
> +size_t iov_discard_back_undoable(struct iovec *iov,
> + unsigned int *iov_cnt,
> + size_t bytes,
> + IOVDiscardUndo *undo)
> {
> size_t total = 0;
> struct iovec *cur;
>
> +if (undo) {
> +undo->modified_iov = NULL;
> +}
> +
> if (*iov_cnt == 0) {
> return 0;
> }
> @@ -673,6 +704,11 @@ size_t iov_discard_back(struct iovec *iov, unsigned
> int *iov_cnt,
>
> while (*iov_cnt > 0) {
> if (cur->iov_len > bytes) {
> +if (undo) {
> +undo->modified_iov = cur;
> +undo->orig = *cur;
> +}
> +
> cur->iov_len -= bytes;
> total += bytes;
> break;
> @@ -687,6 +723,12 @@ size_t iov_discard_back(struct iovec *iov, unsigned
> int *iov_cnt,
> return total;
> }
>
> +size_t iov_discard_back(struct iovec *iov, unsigned int *iov_cnt,
> +size_t bytes)
> +{
> +return iov_discard_back_undoable(iov, iov_cnt, bytes, NULL);
> +}
> +
> void qemu_iovec_discard_back(QEMUIOVector *qiov, size_t bytes)
> {
> size_t total;
> --
> 2.26.2
>
>
Re: [PATCH 6/7] hw/ide/pci: Replace magic '512' value by BDRV_SECTOR_SIZE
Philippe Mathieu-Daudé 于2020年8月14日周五 下午4:33写道：
>
> Use self-explicit definitions instead of magic '512' value.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> hw/ide/pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 5e85c4ad17..b50091b615 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -138,7 +138,7 @@ static int32_t bmdma_prepare_buf(const IDEDMA *dma,
> int32_t limit)
> int l, len;
>
> pci_dma_sglist_init(>sg, pci_dev,
> -s->nsector / (BMDMA_PAGE_SIZE / 512) + 1);
> +s->nsector / (BMDMA_PAGE_SIZE / BDRV_SECTOR_SIZE) +
> 1);
> s->io_buffer_size = 0;
> for(;;) {
> if (bm->cur_prd_len == 0) {
> --
> 2.21.3
>
>
Re: [PATCH 1/7] block/null: Make more explicit the driver default size is 1GiB
Philippe Mathieu-Daudé 于2020年8月14日周五 下午4:29写道：
>
> As it is not obvious the default size for the null block driver
> is 1 GiB, replace the obfuscated '1 << 30' magic value by a
> definition using IEC binary prefixes.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> block/null.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/block/null.c b/block/null.c
> index 15e1d56746..8354def367 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -11,6 +11,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/units.h"
> #include "qapi/error.h"
> #include "qapi/qmp/qdict.h"
> #include "qapi/qmp/qstring.h"
> @@ -21,6 +22,7 @@
>
> #define NULL_OPT_LATENCY "latency-ns"
> #define NULL_OPT_ZEROES "read-zeroes"
> +#define NULL_OPT_SIZE(1 * GiB)
>
> typedef struct {
> int64_t length;
> @@ -86,7 +88,7 @@ static int null_file_open(BlockDriverState *bs, QDict
> *options, int flags,
> opts = qemu_opts_create(_opts, NULL, 0, _abort);
> qemu_opts_absorb_qdict(opts, options, _abort);
> s->length =
> -qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
> +qemu_opt_get_size(opts, BLOCK_OPT_SIZE, NULL_OPT_SIZE);
> s->latency_ns =
> qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0);
> if (s->latency_ns < 0) {
> --
> 2.21.3
>
>
Re: [PATCH 4/7] hw/ide/ahci: Replace magic '512' value by BDRV_SECTOR_SIZE
Philippe Mathieu-Daudé 于2020年8月14日周五 下午4:31写道：
>
> Use self-explicit definitions instead of magic '512' value.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> hw/ide/ahci.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 009120f88b..b696c6291a 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1151,7 +1151,7 @@ static void process_ncq_command(AHCIState *s, int port,
> uint8_t *cmd_fis,
> if (!ncq_tfs->sector_count) {
> ncq_tfs->sector_count = 0x1;
> }
> -size = ncq_tfs->sector_count * 512;
> +size = ncq_tfs->sector_count * BDRV_SECTOR_SIZE;
> ahci_populate_sglist(ad, _tfs->sglist, ncq_tfs->cmdh, size, 0);
>
> if (ncq_tfs->sglist.size < size) {
> @@ -1703,7 +1703,8 @@ static int ahci_state_post_load(void *opaque, int
> version_id)
> return -1;
> }
> ahci_populate_sglist(ncq_tfs->drive, _tfs->sglist,
> - ncq_tfs->cmdh, ncq_tfs->sector_count * 512,
> + ncq_tfs->cmdh,
> + ncq_tfs->sector_count * BDRV_SECTOR_SIZE,
> 0);
> if (ncq_tfs->sector_count != ncq_tfs->sglist.size >> 9) {
> return -1;
> --
> 2.21.3
>
>
Re: [PATCH 3/7] hw/ide/core: Replace magic '512' value by BDRV_SECTOR_SIZE
Philippe Mathieu-Daudé 于2020年8月14日周五 下午4:31写道：
>
> Use self-explicit definitions instead of magic '512' value.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> hw/ide/core.c | 23 ---
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index f76f7e5234..bcb2aa85fc 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -121,7 +121,7 @@ static void ide_identify(IDEState *s)
> put_le16(p + 0, 0x0040);
> put_le16(p + 1, s->cylinders);
> put_le16(p + 3, s->heads);
> -put_le16(p + 4, 512 * s->sectors); /* XXX: retired, remove ? */
> +put_le16(p + 4, BDRV_SECTOR_SIZE * s->sectors); /* XXX: retired, remove
> ? */
> put_le16(p + 5, 512); /* XXX: retired, remove ? */
> put_le16(p + 6, s->sectors);
> padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
> @@ -864,7 +864,7 @@ static void ide_dma_cb(void *opaque, int ret)
> }
> }
>
> -if (s->io_buffer_size > s->nsector * 512) {
> +if (s->io_buffer_size > s->nsector * BDRV_SECTOR_SIZE) {
> /*
> * The PRDs were longer than needed for this request.
> * The Active bit must remain set after the request completes.
> @@ -877,7 +877,7 @@ static void ide_dma_cb(void *opaque, int ret)
>
> sector_num = ide_get_sector(s);
> if (n > 0) {
> -assert(n * 512 == s->sg.size);
> +assert(n * BDRV_SECTOR_SIZE == s->sg.size);
> dma_buf_commit(s, s->sg.size);
> sector_num += n;
> ide_set_sector(s, sector_num);
> @@ -894,17 +894,17 @@ static void ide_dma_cb(void *opaque, int ret)
> /* launch next transfer */
> n = s->nsector;
> s->io_buffer_index = 0;
> -s->io_buffer_size = n * 512;
> +s->io_buffer_size = n * BDRV_SECTOR_SIZE;
> prep_size = s->bus->dma->ops->prepare_buf(s->bus->dma,
> s->io_buffer_size);
> /* prepare_buf() must succeed and respect the limit */
> -assert(prep_size >= 0 && prep_size <= n * 512);
> +assert(prep_size >= 0 && prep_size <= n * BDRV_SECTOR_SIZE);
>
> /*
> * Now prep_size stores the number of bytes in the sglist, and
> * s->io_buffer_size stores the number of bytes described by the PRDs.
> */
>
> -if (prep_size < n * 512) {
> +if (prep_size < n * BDRV_SECTOR_SIZE) {
> /*
> * The PRDs are too short for this request. Error condition!
> * Reset the Active bit and don't raise the interrupt.
> @@ -1412,7 +1412,8 @@ static bool cmd_identify(IDEState *s, uint8_t cmd)
> ide_cfata_identify(s);
> }
> s->status = READY_STAT | SEEK_STAT;
> -ide_transfer_start(s, s->io_buffer, 512, ide_transfer_stop);
> +ide_transfer_start(s, s->io_buffer, BDRV_SECTOR_SIZE,
> + ide_transfer_stop);
> ide_set_irq(s->bus);
> return false;
> } else {
> @@ -1482,7 +1483,7 @@ static bool cmd_write_multiple(IDEState *s, uint8_t cmd)
> n = MIN(s->nsector, s->req_nb_sectors);
>
> s->status = SEEK_STAT | READY_STAT;
> -ide_transfer_start(s, s->io_buffer, 512 * n, ide_sector_write);
> +ide_transfer_start(s, s->io_buffer, BDRV_SECTOR_SIZE * n,
> ide_sector_write);
>
> s->media_changed = 1;
>
> @@ -1524,7 +1525,7 @@ static bool cmd_write_pio(IDEState *s, uint8_t cmd)
>
> s->req_nb_sectors = 1;
> s->status = SEEK_STAT | READY_STAT;
> -ide_transfer_start(s, s->io_buffer, 512, ide_sector_write);
> +ide_transfer_start(s, s->io_buffer, BDRV_SECTOR_SIZE, ide_sector_write);
>
> s->media_changed = 1;
>
> @@ -1678,7 +1679,7 @@ static bool cmd_identify_packet(IDEState *s, uint8_t
> cmd)
> {
> ide_atapi_identify(s);
> s->status = READY_STAT | SEEK_STAT;
> -ide_transfer_start(s, s->io_buffer, 512, ide_transfer_stop);
> +ide_transfer_start(s, s->io_buffer, BDRV_SECTOR_SIZE, ide_transfer_stop);
> ide_set_irq(s->bus);
> return false;
> }
> @@ -2559,7 +2560,7 @@ static void ide_init1(IDEBus *bus, int unit)
> s->unit = unit;
> s->drive_serial = drive_serial++;
> /* we need at least 2k alignment for accessing CDROMs using O_DIRECT */
> -s->io_buffer_total_len = IDE_DMA_BUF_SECTORS*512 + 4;
> +s->io_buffer_total_len = IDE_DMA_BUF_SECTORS * BDRV_SECTOR_SIZE + 4;
> s->io_buffer = qemu_memalign(2048, s->io_buffer_total_len);
> memset(s->io_buffer, 0, s->io_buffer_total_len);
>
> --
> 2.21.3
>
>
Re: [PATCH 7/7] hw/scsi/scsi-disk: Replace magic '512' value by BDRV_SECTOR_SIZE
Philippe Mathieu-Daudé 于2020年8月14日周五 下午4:34写道：
>
> Use self-explicit definitions instead of magic '512' value.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> hw/scsi/scsi-disk.c | 44 +++-
> 1 file changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 8ce68a9dd6..7612035a4e 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -71,7 +71,7 @@ typedef struct SCSIDiskClass {
>
> typedef struct SCSIDiskReq {
> SCSIRequest req;
> -/* Both sector and sector_count are in terms of qemu 512 byte blocks. */
> +/* Both sector and sector_count are in terms of BDRV_SECTOR_SIZE bytes.
> */
> uint64_t sector;
> uint32_t sector_count;
> uint32_t buflen;
> @@ -141,7 +141,7 @@ static void scsi_init_iovec(SCSIDiskReq *r, size_t size)
> r->buflen = size;
> r->iov.iov_base = blk_blockalign(s->qdev.conf.blk, r->buflen);
> }
> -r->iov.iov_len = MIN(r->sector_count * 512, r->buflen);
> +r->iov.iov_len = MIN(r->sector_count * BDRV_SECTOR_SIZE, r->buflen);
> qemu_iovec_init_external(>qiov, >iov, 1);
> }
>
> @@ -311,7 +311,7 @@ static void scsi_read_complete_noio(SCSIDiskReq *r, int
> ret)
> goto done;
> }
>
> -n = r->qiov.size / 512;
> +n = r->qiov.size / BDRV_SECTOR_SIZE;
> r->sector += n;
> r->sector_count -= n;
> scsi_req_data(>req, r->qiov.size);
> @@ -505,7 +505,7 @@ static void scsi_write_complete_noio(SCSIDiskReq *r, int
> ret)
> goto done;
> }
>
> -n = r->qiov.size / 512;
> +n = r->qiov.size / BDRV_SECTOR_SIZE;
> r->sector += n;
> r->sector_count -= n;
> if (r->sector_count == 0) {
> @@ -1284,7 +1284,7 @@ static int scsi_disk_emulate_mode_sense(SCSIDiskReq *r,
> uint8_t *outbuf)
> } else { /* MODE_SENSE_10 */
> outbuf[7] = 8; /* Block descriptor length */
> }
> -nb_sectors /= (s->qdev.blocksize / 512);
> +nb_sectors /= (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> if (nb_sectors > 0xff) {
> nb_sectors = 0;
> }
> @@ -1342,7 +1342,7 @@ static int scsi_disk_emulate_read_toc(SCSIRequest *req,
> uint8_t *outbuf)
> start_track = req->cmd.buf[6];
> blk_get_geometry(s->qdev.conf.blk, _sectors);
> trace_scsi_disk_emulate_read_toc(start_track, format, msf >> 1);
> -nb_sectors /= s->qdev.blocksize / 512;
> +nb_sectors /= s->qdev.blocksize / BDRV_SECTOR_SIZE;
> switch (format) {
> case 0:
> toclen = cdrom_read_toc(nb_sectors, outbuf, msf, start_track);
> @@ -1738,9 +1738,10 @@ static void scsi_write_same_complete(void *opaque, int
> ret)
>
> block_acct_done(blk_get_stats(s->qdev.conf.blk), >acct);
>
> -data->nb_sectors -= data->iov.iov_len / 512;
> -data->sector += data->iov.iov_len / 512;
> -data->iov.iov_len = MIN(data->nb_sectors * 512, data->iov.iov_len);
> +data->nb_sectors -= data->iov.iov_len / BDRV_SECTOR_SIZE;
> +data->sector += data->iov.iov_len / BDRV_SECTOR_SIZE;
> +data->iov.iov_len = MIN(data->nb_sectors * BDRV_SECTOR_SIZE,
> +data->iov.iov_len);
> if (data->iov.iov_len) {
> block_acct_start(blk_get_stats(s->qdev.conf.blk), >acct,
> data->iov.iov_len, BLOCK_ACCT_WRITE);
> @@ -1805,9 +1806,10 @@ static void scsi_disk_emulate_write_same(SCSIDiskReq
> *r, uint8_t *inbuf)
>
> data = g_new0(WriteSameCBData, 1);
> data->r = r;
> -data->sector = r->req.cmd.lba * (s->qdev.blocksize / 512);
> -data->nb_sectors = nb_sectors * (s->qdev.blocksize / 512);
> -data->iov.iov_len = MIN(data->nb_sectors * 512, SCSI_WRITE_SAME_MAX);
> +data->sector = r->req.cmd.lba * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> +data->nb_sectors = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE);
> +data->iov.iov_len = MIN(data->nb_sectors * BDRV_SECTOR_SIZE,
> +SCSI_WRITE_SAME_MAX);
> data->iov.iov_base = buf = blk_blockalign(s->qdev.conf.blk,
>data->iov.iov_len);
> qemu_iovec_init_external(>qiov, >iov, 1);
> @@ -1980,7 +1982,7 @@ static int32_t scsi_disk_emulate_command(SCSIRequest
> *req, uint8_t *buf)
> if ((req->cmd.buf[8] & 1) == 0 && req->cmd.lba) {
> goto illegal_requ
Re: [PATCH 5/7] hw/ide/atapi: Replace magic '512' value by BDRV_SECTOR_SIZE
Philippe Mathieu-Daudé 于2020年8月14日周五 下午4:30写道：
>
> Use self-explicit definitions instead of magic '512' value.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> hw/ide/atapi.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 17a9d635d8..14a2b0bb2f 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -824,9 +824,9 @@ static void cmd_get_configuration(IDEState *s, uint8_t
> *buf)
> *
> * Only a problem if the feature/profiles grow.
> */
> -if (max_len > 512) {
> +if (max_len > BDRV_SECTOR_SIZE) {
> /* XXX: assume 1 sector */
> -max_len = 512;
> +max_len = BDRV_SECTOR_SIZE;
> }
>
> memset(buf, 0, max_len);
> @@ -1186,8 +1186,8 @@ static void cmd_read_dvd_structure(IDEState *s,
> uint8_t* buf)
> }
> }
>
> -memset(buf, 0, max_len > IDE_DMA_BUF_SECTORS * 512 + 4 ?
> - IDE_DMA_BUF_SECTORS * 512 + 4 : max_len);
> +memset(buf, 0, max_len > IDE_DMA_BUF_SECTORS * BDRV_SECTOR_SIZE + 4 ?
> + IDE_DMA_BUF_SECTORS * BDRV_SECTOR_SIZE + 4 : max_len);
>
> switch (format) {
> case 0x00 ... 0x7f:
> --
> 2.21.3
>
>
[PATCH 0/2] Fix the assert failure in scsi_dma_complete
Currently in 'megasas_map_sgl' when 'iov_count=0' will just return success however the 'cmd' doens't contain any iov. This will cause the assert in 'scsi_dma_complete' failed. This is because in 'dma_blk_cb' the 'dbs->sg_cur_index == dbs->sg->nsg' will be true and just call 'dma_complete'. However now there is no aiocb returned. This is the LP#1878263: -->https://bugs.launchpad.net/qemu/+bug/1878263 To solve this we will consider the 'iov_count=0' is an error. In the first patch, I uses -1 to indicate an error and in the second patch I consider 'iov_count=0' is an error. Li Qiang (2): hw: megasas: return -1 when 'megasas_map_sgl' fails hw: megasas: consider 'iov_count=0' is an error in megasas_map_sgl hw/scsi/megasas.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.17.1
[PATCH 2/2] hw: megasas: consider 'iov_count=0' is an error in megasas_map_sgl
Currently in 'megasas_map_sgl' when 'iov_count=0' will just return
success however the 'cmd' doens't contain any iov. This will cause
the assert in 'scsi_dma_complete' failed. This is because in
'dma_blk_cb' the 'dbs->sg_cur_index == dbs->sg->nsg' will be true
and just call 'dma_complete'. However now there is no aiocb returned.
This fixes the LP#1878263:
-->https://bugs.launchpad.net/qemu/+bug/1878263
Reported-by: Alexander Bulekov
Signed-off-by: Li Qiang
---
hw/scsi/megasas.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index d6c9680c36..9562c58a2d 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -278,7 +278,7 @@ static int megasas_map_sgl(MegasasState *s, MegasasCmd
*cmd, union mfi_sgl *sgl)
cmd->flags = le16_to_cpu(cmd->frame->header.flags);
iov_count = cmd->frame->header.sge_count;
-if (iov_count > MEGASAS_MAX_SGE) {
+if (!iov_count || iov_count > MEGASAS_MAX_SGE) {
trace_megasas_iovec_sgl_overflow(cmd->index, iov_count,
MEGASAS_MAX_SGE);
return -1;
--
2.17.1
[PATCH 1/2] hw: megasas: return -1 when 'megasas_map_sgl' fails
The caller of 'megasas_map_sgl' will only check if the return
is zero or not. If it return 0 it means success, as in the next
patch we will consider 'iov_count=0' is an error, so let's
return -1 to indicate a failure.
Signed-off-by: Li Qiang
---
hw/scsi/megasas.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 5cfd1bf22e..d6c9680c36 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -281,7 +281,7 @@ static int megasas_map_sgl(MegasasState *s, MegasasCmd
*cmd, union mfi_sgl *sgl)
if (iov_count > MEGASAS_MAX_SGE) {
trace_megasas_iovec_sgl_overflow(cmd->index, iov_count,
MEGASAS_MAX_SGE);
-return iov_count;
+return -1;
}
pci_dma_sglist_init(>qsg, PCI_DEVICE(s), iov_count);
for (i = 0; i < iov_count; i++) {
@@ -311,7 +311,7 @@ static int megasas_map_sgl(MegasasState *s, MegasasCmd
*cmd, union mfi_sgl *sgl)
return 0;
unmap:
qemu_sglist_destroy(>qsg);
-return iov_count - i;
+return -1;
}
/*
--
2.17.1
Re: [PATCH 2/2] hw: megasas: consider 'iov_count=0' is an error in megasas_map_sgl
Oh, sorry to forget to CC Alexander Bulekov.
Thanks,
Li Qiang
Li Qiang 于2020年8月15日周六 下午10:20写道：
>
> Currently in 'megasas_map_sgl' when 'iov_count=0' will just return
> success however the 'cmd' doens't contain any iov. This will cause
> the assert in 'scsi_dma_complete' failed. This is because in
> 'dma_blk_cb' the 'dbs->sg_cur_index == dbs->sg->nsg' will be true
> and just call 'dma_complete'. However now there is no aiocb returned.
>
> This fixes the LP#1878263:
>
> -->https://bugs.launchpad.net/qemu/+bug/1878263
>
> Reported-by: Alexander Bulekov
> Signed-off-by: Li Qiang
> ---
> hw/scsi/megasas.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index d6c9680c36..9562c58a2d 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -278,7 +278,7 @@ static int megasas_map_sgl(MegasasState *s, MegasasCmd
> *cmd, union mfi_sgl *sgl)
>
> cmd->flags = le16_to_cpu(cmd->frame->header.flags);
> iov_count = cmd->frame->header.sge_count;
> -if (iov_count > MEGASAS_MAX_SGE) {
> +if (!iov_count || iov_count > MEGASAS_MAX_SGE) {
> trace_megasas_iovec_sgl_overflow(cmd->index, iov_count,
> MEGASAS_MAX_SGE);
> return -1;
> --
> 2.17.1
>
[PATCH] hw: ide: check the pointer before do dma memory unmap
In 'map_page' we need to check the return value of
'dma_memory_map' to ensure the we actully maped something.
Otherwise, we will hit an assert in 'address_space_unmap'.
This is because we can't find the MR with the NULL buffer.
This is the LP#1884693:
-->https://bugs.launchpad.net/qemu/+bug/1884693
Reported-by: Alexander Bulekov
Signed-off-by: Li Qiang
---
hw/ide/ahci.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 009120f88b..63e9fccdbe 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -250,6 +250,11 @@ static void map_page(AddressSpace *as, uint8_t **ptr,
uint64_t addr,
}
*ptr = dma_memory_map(as, addr, , DMA_DIRECTION_FROM_DEVICE);
+
+if (!*ptr) {
+return;
+}
+
if (len < wanted) {
dma_memory_unmap(as, *ptr, len, DMA_DIRECTION_FROM_DEVICE, len);
*ptr = NULL;
--
2.17.1
Re: [PATCH 2/7] hw/ide/core: Trivial typo fix
Philippe Mathieu-Daudé 于2020年8月14日周五 下午4:29写道： > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Li Qiang > --- > hw/ide/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index d997a78e47..f76f7e5234 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -709,7 +709,7 @@ void ide_cancel_dma_sync(IDEState *s) > /* > * We can't cancel Scatter Gather DMA in the middle of the > * operation or a partial (not full) DMA transfer would reach > - * the storage so we wait for completion instead (we beahve > + * the storage so we wait for completion instead (we behave > * like if the DMA was completed by the time the guest trying > * to cancel dma with bmdma_cmd_writeb with BM_CMD_START not > * set). > -- > 2.21.3 > >
Re: [PATCH v3 3/8] hw/ide/piix: Convert reset handler to DeviceReset
Philippe Mathieu-Daudé 于2019年10月10日周四 下午9:16写道：
> The PIIX/IDE is a PCI device within a PIIX chipset, it will be reset
> when the PCI bus it stands on is reset.
>
> Convert its reset handler into a proper Device reset method.
>
> Signed-off-by: Philippe Mathieu-Daudé
>
Reviewed-by: Li Qiang
> ---
> v3: Also convert PIIX4 (Li Qiang)
> ---
> hw/ide/piix.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index fba6bc8bff..db313dd3b1 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -30,7 +30,6 @@
> #include "sysemu/block-backend.h"
> #include "sysemu/blockdev.h"
> #include "sysemu/dma.h"
> -#include "sysemu/reset.h"
>
> #include "hw/ide/pci.h"
> #include "trace.h"
> @@ -103,9 +102,9 @@ static void bmdma_setup_bar(PCIIDEState *d)
> }
> }
>
> -static void piix3_reset(void *opaque)
> +static void piix_ide_reset(DeviceState *dev)
> {
> -PCIIDEState *d = opaque;
> +PCIIDEState *d = PCI_IDE(dev);
> PCIDevice *pd = PCI_DEVICE(d);
> uint8_t *pci_conf = pd->config;
> int i;
> @@ -154,8 +153,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
> **errp)
>
> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>
> -qemu_register_reset(piix3_reset, d);
> -
> bmdma_setup_bar(d);
> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
>
> @@ -247,6 +244,7 @@ static void piix3_ide_class_init(ObjectClass *klass,
> void *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> +dc->reset = piix_ide_reset;
> k->realize = pci_piix_ide_realize;
> k->exit = pci_piix_ide_exitfn;
> k->vendor_id = PCI_VENDOR_ID_INTEL;
> @@ -273,6 +271,7 @@ static void piix4_ide_class_init(ObjectClass *klass,
> void *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> +dc->reset = piix_ide_reset;
> k->realize = pci_piix_ide_realize;
> k->exit = pci_piix_ide_exitfn;
> k->vendor_id = PCI_VENDOR_ID_INTEL;
> --
> 2.21.0
>
>
Re: [PATCH v2 7/8] hw/input/lm832x: Convert reset handler to DeviceReset
Philippe Mathieu-Daudé 于2019年10月8日周二 下午10:38写道：
> The LM8323 key-scan controller is a I2C device, it will be reset
> when the I2C bus it stands on is reset.
>
> Convert its reset handler into a proper Device reset method.
>
> Signed-off-by: Philippe Mathieu-Daudé
>
Reviewed-by: Li Qiang
> ---
> hw/input/lm832x.c | 12 +---
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
> index a37eb854b9..aa629ddbf1 100644
> --- a/hw/input/lm832x.c
> +++ b/hw/input/lm832x.c
> @@ -24,7 +24,6 @@
> #include "migration/vmstate.h"
> #include "qemu/module.h"
> #include "qemu/timer.h"
> -#include "sysemu/reset.h"
> #include "ui/console.h"
>
> #define TYPE_LM8323 "lm8323"
> @@ -94,8 +93,10 @@ static void lm_kbd_gpio_update(LM823KbdState *s)
> {
> }
>
> -static void lm_kbd_reset(LM823KbdState *s)
> +static void lm_kbd_reset(DeviceState *dev)
> {
> +LM823KbdState *s = LM8323(dev);
> +
> s->config = 0x80;
> s->status = INT_NOINIT;
> s->acttime = 125;
> @@ -273,7 +274,7 @@ static void lm_kbd_write(LM823KbdState *s, int reg,
> int byte, uint8_t value)
>
> case LM832x_CMD_RESET:
> if (value == 0xaa)
> -lm_kbd_reset(s);
> +lm_kbd_reset(DEVICE(s));
> else
> lm_kbd_error(s, ERR_BADPAR);
> s->reg = LM832x_GENERAL_ERROR;
> @@ -476,10 +477,6 @@ static void lm8323_realize(DeviceState *dev, Error
> **errp)
> s->pwm.tm[1] = timer_new_ns(QEMU_CLOCK_VIRTUAL, lm_kbd_pwm1_tick, s);
> s->pwm.tm[2] = timer_new_ns(QEMU_CLOCK_VIRTUAL, lm_kbd_pwm2_tick, s);
> qdev_init_gpio_out(dev, >nirq, 1);
> -
> -lm_kbd_reset(s);
> -
> -qemu_register_reset((void *) lm_kbd_reset, s);
> }
>
> void lm832x_key_event(DeviceState *dev, int key, int state)
> @@ -507,6 +504,7 @@ static void lm8323_class_init(ObjectClass *klass, void
> *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
> I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
>
> +dc->reset = lm_kbd_reset;
> dc->realize = lm8323_realize;
> k->event = lm_i2c_event;
> k->recv = lm_i2c_rx;
> --
> 2.21.0
>
>
>
Re: [PATCH v2 6/8] hw/isa/vt82c686: Convert reset handler to DeviceReset
Philippe Mathieu-Daudé 于2019年10月8日周二 下午10:39写道：
> The VIA VT82C686 Southbridge is a PCI device, it will be reset
> when the PCI bus it stands on is reset.
>
> Convert its reset handler into a proper Device reset method.
>
> Signed-off-by: Philippe Mathieu-Daudé
>
Reviewed-by: Li Qiang
> ---
> hw/isa/vt82c686.c | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 50bd28fa82..616f67f347 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -23,7 +23,6 @@
> #include "hw/isa/apm.h"
> #include "hw/acpi/acpi.h"
> #include "hw/i2c/pm_smbus.h"
> -#include "sysemu/reset.h"
> #include "qemu/module.h"
> #include "qemu/timer.h"
> #include "exec/address-spaces.h"
> @@ -116,11 +115,10 @@ static const MemoryRegionOps superio_ops = {
> },
> };
>
> -static void vt82c686b_reset(void * opaque)
> +static void vt82c686b_isa_reset(DeviceState *dev)
> {
> -PCIDevice *d = opaque;
> -uint8_t *pci_conf = d->config;
> -VT82C686BState *vt82c = VT82C686B_DEVICE(d);
> +VT82C686BState *vt82c = VT82C686B_DEVICE(dev);
> +uint8_t *pci_conf = vt82c->dev.config;
>
> pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x00c0);
> pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO |
> PCI_COMMAND_MEMORY |
> @@ -476,8 +474,6 @@ static void vt82c686b_realize(PCIDevice *d, Error
> **errp)
> * But we do not emulate a floppy, so just set it here. */
> memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
> >superio);
> -
> -qemu_register_reset(vt82c686b_reset, d);
> }
>
> ISABus *vt82c686b_isa_init(PCIBus *bus, int devfn)
> @@ -501,6 +497,7 @@ static void via_class_init(ObjectClass *klass, void
> *data)
> k->device_id = PCI_DEVICE_ID_VIA_ISA_BRIDGE;
> k->class_id = PCI_CLASS_BRIDGE_ISA;
> k->revision = 0x40;
> +dc->reset = vt82c686b_isa_reset;
> dc->desc = "ISA bridge";
> dc->vmsd = _via;
> /*
> --
> 2.21.0
>
>
>
Re: [PATCH v2 5/8] hw/ide/via82c: Convert reset handler to DeviceReset
Philippe Mathieu-Daudé 于2019年10月8日周二 下午10:36写道：
> The VIA82C686B IDE controller is a PCI device, it will be reset
> when the PCI bus it stands on is reset.
>
> Convert its reset handler into a proper Device reset method.
>
> Signed-off-by: Philippe Mathieu-Daudé
>
Reviewed-by: Li Qiang
> ---
> hw/ide/via.c | 10 --
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 7087dc676e..053622bd82 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -29,7 +29,6 @@
> #include "migration/vmstate.h"
> #include "qemu/module.h"
> #include "sysemu/dma.h"
> -#include "sysemu/reset.h"
>
> #include "hw/ide/pci.h"
> #include "trace.h"
> @@ -120,10 +119,10 @@ static void via_ide_set_irq(void *opaque, int n, int
> level)
> }
> }
>
> -static void via_ide_reset(void *opaque)
> +static void via_ide_reset(DeviceState *dev)
> {
> -PCIIDEState *d = opaque;
> -PCIDevice *pd = PCI_DEVICE(d);
> +PCIIDEState *d = PCI_IDE(dev);
> +PCIDevice *pd = PCI_DEVICE(dev);
> uint8_t *pci_conf = pd->config;
> int i;
>
> @@ -172,8 +171,6 @@ static void via_ide_realize(PCIDevice *dev, Error
> **errp)
> pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x00c0);
> dev->wmask[PCI_INTERRUPT_LINE] = 0xf;
>
> -qemu_register_reset(via_ide_reset, d);
> -
> memory_region_init_io(>data_bar[0], OBJECT(d),
> _ide_data_le_ops,
>>bus[0], "via-ide0-data", 8);
> pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, >data_bar[0]);
> @@ -229,6 +226,7 @@ static void via_ide_class_init(ObjectClass *klass,
> void *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> +dc->reset = via_ide_reset;
> k->realize = via_ide_realize;
> k->exit = via_ide_exitfn;
> k->vendor_id = PCI_VENDOR_ID_VIA;
> --
> 2.21.0
>
>
>
Re: [PATCH v2 4/8] hw/ide/sii3112: Convert reset handler to DeviceReset
Philippe Mathieu-Daudé 于2019年10月8日周二 下午10:32写道：
> The SiI3112A SATA controller is a PCI device, it will be reset
> when the PCI bus it stands on is reset.
>
> Convert its reset handler into a proper Device reset method.
>
> Signed-off-by: Philippe Mathieu-Daudé
>
Reviewed-by: Li Qiang
> ---
> hw/ide/sii3112.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
> index 2181260531..06605d7af2 100644
> --- a/hw/ide/sii3112.c
> +++ b/hw/ide/sii3112.c
> @@ -15,7 +15,6 @@
> #include "qemu/osdep.h"
> #include "hw/ide/pci.h"
> #include "qemu/module.h"
> -#include "sysemu/reset.h"
> #include "trace.h"
>
> #define TYPE_SII3112_PCI "sii3112"
> @@ -237,9 +236,9 @@ static void sii3112_set_irq(void *opaque, int channel,
> int level)
> sii3112_update_irq(s);
> }
>
> -static void sii3112_reset(void *opaque)
> +static void sii3112_reset(DeviceState *dev)
> {
> -SiI3112PCIState *s = opaque;
> +SiI3112PCIState *s = SII3112_PCI(dev);
> int i;
>
> for (i = 0; i < 2; i++) {
> @@ -290,7 +289,6 @@ static void sii3112_pci_realize(PCIDevice *dev, Error
> **errp)
> s->bmdma[i].bus = >bus[i];
> ide_register_restart_cb(>bus[i]);
> }
> -qemu_register_reset(sii3112_reset, s);
> }
>
> static void sii3112_pci_class_init(ObjectClass *klass, void *data)
> @@ -303,6 +301,7 @@ static void sii3112_pci_class_init(ObjectClass *klass,
> void *data)
> pd->class_id = PCI_CLASS_STORAGE_RAID;
> pd->revision = 1;
> pd->realize = sii3112_pci_realize;
> +dc->reset = sii3112_reset;
> dc->desc = "SiI3112A SATA controller";
> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> }
> --
> 2.21.0
>
>
>
Re: [PATCH v2 0/8] hw: Convert various reset() handler to DeviceReset
Philippe Mathieu-Daudé 于2019年10月10日周四 上午3:54写道： > Hi Li, > > On 10/9/19 4:28 AM, Li Qiang wrote: > > Philippe Mathieu-Daudé mailto:phi...@redhat.com>> 于 > > 2019年10月8日周二 下午10:47写道： > > > > Since v1: > > - Removed the pci-host devices > > > > > > Hello I want to know why remove this? > > I haven't removed the devices, I simply remove the patches converting > them to DeviceReset, Yes, I mean the patches. > basically because I've not enough time to check if > the are on a bus that would reset them. IIUC, they are right. > I added these devices on my TODO > list for later, so meanwhile the other devices can be easily reviewed > and merged. When few patches from a series are not reviewed or > incorrect, sometime the rest of the series is not merged, so I prefer to > split it and get these patches merged. As far as I can see, most of the devices' usage of qemu_register_reset function can be convert to 'dc->reset'. In the main function. qemu_register_reset(qbus_reset_all_fn, sysbus_get_default()); The 'qbus_reset_all_fn' calls 'qbus_reset_all' from the 'main-sys-bus'. Then 'qdev_reset_one' will call 'device_reset'. So IIUC every bus attached to 'main-sys-bus' can be reset through 'dc->reset' So I'm quite sure most of the cases that devices use 'qemu_register_reset' can be changed to 'dc->reset'. Seems you're busy, If you don't mind, I can do some of the work to convert 'reset' callback(not a patchset, one by one). Thanks, Li Qiang > > > > > - Removed the vmcoreinfo conversion (elmarco) but add a comment. > > - Added Igor's R-b tag. > > > > Following the thread discussion between Peter/Markus/Damien about > > reset handlers: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg617103.html > > I started to remove qemu_register_reset() calls from few qdevified > > devices (the trivial ones). > > > > Regards, > > > > Phil. > > > > v1: > https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg06367.html > > > > Philippe Mathieu-Daudé (8): > >hw/acpi/piix4: Convert reset handler to DeviceReset > >hw/isa/piix4: Convert reset handler to DeviceReset > >hw/ide/piix: Convert reset handler to DeviceReset > >hw/ide/sii3112: Convert reset handler to DeviceReset > >hw/ide/via82c: Convert reset handler to DeviceReset > >hw/isa/vt82c686: Convert reset handler to DeviceReset > >hw/input/lm832x: Convert reset handler to DeviceReset > >hw/misc/vmcoreinfo: Document its reset handler >
Re: [PATCH v2 0/8] hw: Convert various reset() handler to DeviceReset
Philippe Mathieu-Daudé 于2019年10月8日周二 下午10:47写道： > Since v1: > - Removed the pci-host devices > Hello I want to know why remove this? Thanks, Li Qiang > - Removed the vmcoreinfo conversion (elmarco) but add a comment. > - Added Igor's R-b tag. > > Following the thread discussion between Peter/Markus/Damien about > reset handlers: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg617103.html > I started to remove qemu_register_reset() calls from few qdevified > devices (the trivial ones). > > Regards, > > Phil. > > v1: https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg06367.html > > Philippe Mathieu-Daudé (8): > hw/acpi/piix4: Convert reset handler to DeviceReset > hw/isa/piix4: Convert reset handler to DeviceReset > hw/ide/piix: Convert reset handler to DeviceReset > hw/ide/sii3112: Convert reset handler to DeviceReset > hw/ide/via82c: Convert reset handler to DeviceReset > hw/isa/vt82c686: Convert reset handler to DeviceReset > hw/input/lm832x: Convert reset handler to DeviceReset > hw/misc/vmcoreinfo: Document its reset handler > > hw/acpi/piix4.c | 7 +++ > hw/ide/piix.c| 8 +++- > hw/ide/sii3112.c | 7 +++ > hw/ide/via.c | 10 -- > hw/input/lm832x.c| 12 +--- > hw/isa/piix4.c | 7 +++ > hw/isa/vt82c686.c| 11 --- > hw/misc/vmcoreinfo.c | 1 + > 8 files changed, 26 insertions(+), 37 deletions(-) > > -- > 2.21.0 > > >
Re: [PATCH v2 3/8] hw/ide/piix: Convert reset handler to DeviceReset
Philippe Mathieu-Daudé 于2019年10月8日周二 下午10:32写道：
> The PIIX3/IDE is a PCI device within the PIIX3 chipset, it will be reset
> when the PCI bus it stands on is reset.
>
> Convert its reset handler into a proper Device reset method.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/ide/piix.c | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index fba6bc8bff..18b2c3b722 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -30,7 +30,6 @@
> #include "sysemu/block-backend.h"
> #include "sysemu/blockdev.h"
> #include "sysemu/dma.h"
> -#include "sysemu/reset.h"
>
> #include "hw/ide/pci.h"
> #include "trace.h"
> @@ -103,9 +102,9 @@ static void bmdma_setup_bar(PCIIDEState *d)
> }
> }
>
> -static void piix3_reset(void *opaque)
> +static void piix3_ide_reset(DeviceState *dev)
> {
> -PCIIDEState *d = opaque;
> +PCIIDEState *d = PCI_IDE(dev);
> PCIDevice *pd = PCI_DEVICE(d);
> uint8_t *pci_conf = pd->config;
> int i;
> @@ -154,8 +153,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error
> **errp)
>
> pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode
>
> -qemu_register_reset(piix3_reset, d);
> -
> bmdma_setup_bar(d);
> pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
>
> @@ -247,6 +244,7 @@ static void piix3_ide_class_init(ObjectClass *klass,
> void *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> +dc->reset = piix3_ide_reset;
> k->realize = pci_piix_ide_realize;
> k->exit = pci_piix_ide_exitfn;
> k->vendor_id = PCI_VENDOR_ID_INTEL;
> --
>
Shouldn't we also add the reset callback for piix4 ide device?
Thanks,
Li Qiang
> 2.21.0
>
>
>
Re: [PATCH v2 2/8] hw/isa/piix4: Convert reset handler to DeviceReset
Philippe Mathieu-Daudé 于2019年10月8日周二 下午10:49写道：
> The PIIX4/ISA is a PCI device within the PIIX4 chipset, it will be reset
> when the PCI bus it stands on is reset.
>
> Convert its reset handler into a proper Device reset method.
>
> Signed-off-by: Philippe Mathieu-Daudé
>
Reviewed-by: Li Qiang
> ---
> hw/isa/piix4.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 3294056cd5..890d999abf 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -28,7 +28,6 @@
> #include "hw/isa/isa.h"
> #include "hw/sysbus.h"
> #include "migration/vmstate.h"
> -#include "sysemu/reset.h"
>
> PCIDevice *piix4_dev;
>
> @@ -40,9 +39,9 @@ typedef struct PIIX4State {
> #define PIIX4_PCI_DEVICE(obj) \
> OBJECT_CHECK(PIIX4State, (obj), TYPE_PIIX4_PCI_DEVICE)
>
> -static void piix4_reset(void *opaque)
> +static void piix4_isa_reset(DeviceState *dev)
> {
> -PIIX4State *d = opaque;
> +PIIX4State *d = PIIX4_PCI_DEVICE(dev);
> uint8_t *pci_conf = d->dev.config;
>
> pci_conf[0x04] = 0x07; // master, memory and I/O
> @@ -97,7 +96,6 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
> return;
> }
> piix4_dev = >dev;
> -qemu_register_reset(piix4_reset, d);
> }
>
> int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
> @@ -118,6 +116,7 @@ static void piix4_class_init(ObjectClass *klass, void
> *data)
> k->vendor_id = PCI_VENDOR_ID_INTEL;
> k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
> k->class_id = PCI_CLASS_BRIDGE_ISA;
> +dc->reset = piix4_isa_reset;
> dc->desc = "ISA bridge";
> dc->vmsd = _piix4;
> /*
> --
> 2.21.0
>
>
>
Re: [PATCH v2 1/8] hw/acpi/piix4: Convert reset handler to DeviceReset
Philippe Mathieu-Daudé 于2019年10月8日周二 下午10:28写道：
> The PIIX4/PM is a PCI device within the PIIX4 chipset, it will be reset
> when the PCI bus it stands on is reset.
>
> Convert its reset handler into a proper Device reset method.
>
> Reviewed-by: Igor Mammedov
> Signed-off-by: Philippe Mathieu-Daudé
>
Reviewed-by: Li Qiang
> ---
> hw/acpi/piix4.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 5742c3df87..4e079b39bd 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -27,7 +27,6 @@
> #include "hw/pci/pci.h"
> #include "hw/qdev-properties.h"
> #include "hw/acpi/acpi.h"
> -#include "sysemu/reset.h"
> #include "sysemu/runstate.h"
> #include "sysemu/sysemu.h"
> #include "qapi/error.h"
> @@ -344,9 +343,9 @@ static const VMStateDescription vmstate_acpi = {
> }
> };
>
> -static void piix4_reset(void *opaque)
> +static void piix4_pm_reset(DeviceState *dev)
> {
> -PIIX4PMState *s = opaque;
> +PIIX4PMState *s = PIIX4_PM(dev);
> PCIDevice *d = PCI_DEVICE(s);
> uint8_t *pci_conf = d->config;
>
> @@ -542,7 +541,6 @@ static void piix4_pm_realize(PCIDevice *dev, Error
> **errp)
>
> s->machine_ready.notify = piix4_pm_machine_ready;
> qemu_add_machine_init_done_notifier(>machine_ready);
> -qemu_register_reset(piix4_reset, s);
>
> piix4_acpi_system_hot_add_init(pci_address_space_io(dev),
> pci_get_bus(dev), s);
> @@ -692,6 +690,7 @@ static void piix4_pm_class_init(ObjectClass *klass,
> void *data)
> k->device_id = PCI_DEVICE_ID_INTEL_82371AB_3;
> k->revision = 0x03;
> k->class_id = PCI_CLASS_BRIDGE_OTHER;
> +dc->reset = piix4_pm_reset;
> dc->desc = "PM";
> dc->vmsd = _acpi;
> dc->props = piix4_pm_properties;
> --
> 2.21.0
>
>
>
[Qemu-block] [PATCH] block: qcow2: free 'refcount_table' in error path
Currently, when doing './check -qcow2 098'. We can get following
asan output:
qemu-img: Could not empty blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT:
Input/output error
+
+=
+==60365==ERROR: LeakSanitizer: detected memory leaks
+
+Direct leak of 65536 byte(s) in 1 object(s) allocated from:
+#0 0x7f3ed729fd38 in __interceptor_calloc
(/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded38)
+#1 0x56274517fe66 in make_completely_empty block/IMGFMT.c:4219
+#2 0x562745180e51 in IMGFMT_make_empty block/IMGFMT.c:4313
+#3 0x56274509b14e in img_commit /home/test/qemu5/qemu/qemu-img.c:1053
+#4 0x5627450b4b74 in main /home/test/qemu5/qemu/qemu-img.c:5097
+#5 0x7f3ed4f2fb96 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
This is because the logic of clean resource in 'make_completely_empty' is
wrong. The patch frees the 's->refcount_table' in error path.
Signed-off-by: Li Qiang
---
block/qcow2.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 7c5a4859f7..23fe713d4c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4243,7 +4243,7 @@ static int make_completely_empty(BlockDriverState *bs)
ret = bdrv_pwrite_sync(bs->file, s->cluster_size,
_entry, sizeof(rt_entry));
if (ret < 0) {
-goto fail_broken_refcounts;
+goto fail;
}
s->refcount_table[0] = 2 * s->cluster_size;
@@ -4252,7 +4252,7 @@ static int make_completely_empty(BlockDriverState *bs)
offset = qcow2_alloc_clusters(bs, 3 * s->cluster_size + l1_size2);
if (offset < 0) {
ret = offset;
-goto fail_broken_refcounts;
+goto fail;
} else if (offset > 0) {
error_report("First cluster in emptied image is in use");
abort();
@@ -4274,6 +4274,9 @@ static int make_completely_empty(BlockDriverState *bs)
return 0;
+fail:
+g_free(s->refcount_table);
+
fail_broken_refcounts:
/* The BDS is unusable at this point. If we wanted to make it usable, we
* would have to call qcow2_refcount_close(), qcow2_refcount_init(),
@@ -4283,8 +4286,6 @@ fail_broken_refcounts:
* that that sequence will fail as well. Therefore, just eject the BDS. */
bs->drv = NULL;
-fail:
-g_free(new_reftable);
return ret;
}
--
2.17.1
[Qemu-block] [PATCH 2/3] nvme: ensure the num_queues is not zero
When it is zero, it causes segv.
Using following command:
"-drive file=//home/test/test1.img,if=none,id=id0
-device nvme,drive=id0,serial=test,num_queues=0"
causes following Backtrack:
Thread 4 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffe9735700 (LWP 30952)]
0x55a7a77c in nvme_start_ctrl (n=0x577473f0) at hw/block/nvme.c:825
825 if (unlikely(n->cq[0])) {
(gdb) bt
0 0x55a7a77c in nvme_start_ctrl (n=0x577473f0)
at hw/block/nvme.c:825
1 0x55a7af7f in nvme_write_bar (n=0x577473f0, offset=20,
data=4587521, size=4) at hw/block/nvme.c:969
2 0x55a7b81a in nvme_mmio_write (opaque=0x577473f0, addr=20,
data=4587521, size=4) at hw/block/nvme.c:1163
3 0x55869236 in memory_region_write_accessor (mr=0x57747cd0,
addr=20, value=0x7fffe97320f8, size=4, shift=0, mask=4294967295, attrs=...)
at /home/test/qemu1/qemu/memory.c:502
4 0x55869446 in access_with_adjusted_size (addr=20,
value=0x7fffe97320f8, size=4, access_size_min=2, access_size_max=8,
access_fn=0x5586914d ,
mr=0x57747cd0, attrs=...) at /home/test/qemu1/qemu/memory.c:568
5 0x5586c479 in memory_region_dispatch_write (mr=0x57747cd0,
addr=20, data=4587521, size=4, attrs=...)
at /home/test/qemu1/qemu/memory.c:1499
6 0x558030af in flatview_write_continue (fv=0x7fffe0061130,
addr=4273930260, attrs=..., buf=0x77ff0028 "\001", len=4, addr1=20,
l=4, mr=0x57747cd0) at /home/test/qemu1/qemu/exec.c:3234
7 0x558031f9 in flatview_write (fv=0x7fffe0061130, addr=4273930260,
attrs=..., buf=0x77ff0028 "\001", len=4)
at /home/test/qemu1/qemu/exec.c:3273
8 0x558034ff in address_space_write (
---Type to continue, or q to quit---
as=0x56758480 , addr=4273930260, attrs=...,
buf=0x77ff0028 "\001", len=4) at /home/test/qemu1/qemu/exec.c:3363
9 0x55803550 in address_space_rw (
as=0x56758480 , addr=4273930260, attrs=...,
buf=0x77ff0028 "\001", len=4, is_write=true)
at /home/test/qemu1/qemu/exec.c:3374
10 0x558884a1 in kvm_cpu_exec (cpu=0x56920e40)
at /home/test/qemu1/qemu/accel/kvm/kvm-all.c:2031
11 0x5584cd9d in qemu_kvm_cpu_thread_fn (arg=0x56920e40)
at /home/test/qemu1/qemu/cpus.c:1281
12 0x55dbaf6d in qemu_thread_start (args=0x569438a0)
at util/qemu-thread-posix.c:502
13 0x75dc86db in start_thread (arg=0x7fffe9735700)
at pthread_create.c:463
14 0x75af188f in clone ()
at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
Signed-off-by: Li Qiang
Reviewed-by: Philippe Mathieu-Daud??
---
hw/block/nvme.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index f206391e8e..0b77b49b36 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1208,6 +1208,11 @@ static void nvme_realize(PCIDevice *pci_dev, Error
**errp)
int64_t bs_size;
uint8_t *pci_conf;
+if (!n->num_queues) {
+error_setg(errp, "num_queues can't be zero");
+return;
+}
+
if (!n->conf.blk) {
error_setg(errp, "drive property not set");
return;
--
2.17.1
[Qemu-block] [PATCH 3/3] nvme: use pci_dev directly in nvme_realize
There is no need to make another reference. Signed-off-by: Li Qiang Reviewed-by: Max Reitz Reviewed-by: Philippe Mathieu-Daud?? --- hw/block/nvme.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 0b77b49b36..8325b5e88a 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -1238,7 +1238,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) pci_conf[PCI_INTERRUPT_PIN] = 1; pci_config_set_prog_interface(pci_dev->config, 0x2); pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS); -pcie_endpoint_cap_init(>parent_obj, 0x80); +pcie_endpoint_cap_init(pci_dev, 0x80); n->num_namespaces = 1; n->reg_size = pow2ceil(0x1004 + 2 * (n->num_queues + 1) * 4); @@ -1250,10 +1250,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) memory_region_init_io(>iomem, OBJECT(n), _mmio_ops, n, "nvme", n->reg_size); -pci_register_bar(>parent_obj, 0, +pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64, >iomem); -msix_init_exclusive_bar(>parent_obj, n->num_queues, 4, NULL); +msix_init_exclusive_bar(pci_dev, n->num_queues, 4, NULL); id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID)); id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID)); @@ -1308,7 +1308,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz)); memory_region_init_io(>ctrl_mem, OBJECT(n), _cmb_ops, n, "nvme-cmb", NVME_CMBSZ_GETSIZE(n->bar.cmbsz)); -pci_register_bar(>parent_obj, NVME_CMBLOC_BIR(n->bar.cmbloc), +pci_register_bar(pci_dev, NVME_CMBLOC_BIR(n->bar.cmbloc), PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64 | PCI_BASE_ADDRESS_MEM_PREFETCH, >ctrl_mem); -- 2.17.1
[Qemu-block] [PATCH 1/3] nvme: use TYPE_NVME instead of constant string
Signed-off-by: Li Qiang
Reviewed-by: Max Reitz
Reviewed-by: Philippe Mathieu-Daud??
---
hw/block/nvme.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 7c8c63e8f5..f206391e8e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1381,7 +1381,7 @@ static void nvme_instance_init(Object *obj)
}
static const TypeInfo nvme_info = {
-.name = "nvme",
+.name = TYPE_NVME,
.parent= TYPE_PCI_DEVICE,
.instance_size = sizeof(NvmeCtrl),
.class_init= nvme_class_init,
--
2.17.1
[Qemu-block] [PATCH 0/3] nvme small fix
This patchset contains small fix. Change since v2: For patch 2: 1. add nvme command 2. check num_queues first Change since v1: 1. drop the patch of checking return value of msix_init_exclusive_bar 2. return when nvme's num_queues configuration is 0 Li Qiang (3): nvme: use TYPE_NVME instead of constant string nvme: ensure the num_queues is not zero nvme: use pci_dev directly in nvme_realize hw/block/nvme.c | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) -- 2.17.1
[Qemu-block] [PATCH v2 0/3] nvme small fix
This patchset contains small fix. Change since v1: 1. drop the patch of checking return value of msix_init_exclusive_bar 2. return when nvme's num_queues configuration is 0 Li Qiang (3): nvme: use TYPE_NVME instead of constant string nvme: ensure the num_queues is not zero nvme: use pci_dev directly in nvme_realize hw/block/nvme.c | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) -- 2.17.1
[Qemu-block] [PATCH v2 3/3] nvme: use pci_dev directly in nvme_realize
There is no need to make another reference. Signed-off-by: Li Qiang Reviewed-by: Max Reitz --- hw/block/nvme.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 0ded74fa9a..0a1da749fc 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -1238,7 +1238,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) pci_conf[PCI_INTERRUPT_PIN] = 1; pci_config_set_prog_interface(pci_dev->config, 0x2); pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS); -pcie_endpoint_cap_init(>parent_obj, 0x80); +pcie_endpoint_cap_init(pci_dev, 0x80); n->num_namespaces = 1; n->reg_size = pow2ceil(0x1004 + 2 * (n->num_queues + 1) * 4); @@ -1250,10 +1250,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) memory_region_init_io(>iomem, OBJECT(n), _mmio_ops, n, "nvme", n->reg_size); -pci_register_bar(>parent_obj, 0, +pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64, >iomem); -msix_init_exclusive_bar(>parent_obj, n->num_queues, 4, NULL); +msix_init_exclusive_bar(pci_dev, n->num_queues, 4, NULL); id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID)); id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID)); @@ -1308,7 +1308,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz)); memory_region_init_io(>ctrl_mem, OBJECT(n), _cmb_ops, n, "nvme-cmb", NVME_CMBSZ_GETSIZE(n->bar.cmbsz)); -pci_register_bar(>parent_obj, NVME_CMBLOC_BIR(n->bar.cmbloc), +pci_register_bar(pci_dev, NVME_CMBLOC_BIR(n->bar.cmbloc), PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64 | PCI_BASE_ADDRESS_MEM_PREFETCH, >ctrl_mem); -- 2.17.1
[Qemu-block] [PATCH v2 1/3] nvme: use TYPE_NVME instead of constant string
Signed-off-by: Li Qiang
Reviewed-by: Max Reitz
---
hw/block/nvme.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 7c8c63e8f5..f206391e8e 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1381,7 +1381,7 @@ static void nvme_instance_init(Object *obj)
}
static const TypeInfo nvme_info = {
-.name = "nvme",
+.name = TYPE_NVME,
.parent= TYPE_PCI_DEVICE,
.instance_size = sizeof(NvmeCtrl),
.class_init= nvme_class_init,
--
2.17.1
[Qemu-block] [PATCH v2 2/3] nvme: ensure the num_queues is not zero
When it is zero, it causes segv. Backtrack:
Thread 5 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffc6c17700 (LWP 51808)]
0x55acbd90 in nvme_start_ctrl (n=0x624c8100) at hw/block/nvme.c:820
warning: Source file is more recent than executable.
820 if (unlikely(n->cq[0])) {
(gdb) bt
0 0x55acbd90 in nvme_start_ctrl (n=0x624c8100) at
hw/block/nvme.c:820
1 0x55accdbc in nvme_write_bar (n=0x624c8100, offset=20,
data=4587521, size=4) at hw/block/nvme.c:964
2 0x55acdd2b in nvme_mmio_write (opaque=0x624c8100, addr=20,
data=4587521, size=4) at hw/block/nvme.c:1158
3 0x558973ed in memory_region_write_accessor (mr=0x624c89e0,
addr=20, value=0x7fffc6c14428, size=4, shift=0, mask=4294967295, attrs=...) at
/home/liqiang02/qemu-upstream/qemu/memory.c:500
4 0x55897600 in access_with_adjusted_size (addr=20,
value=0x7fffc6c14428, size=4, access_size_min=2, access_size_max=8,
access_fn=0x55897304 , mr=0x624c89e0,
attrs=...) at /home/liqiang02/qemu-upstream/qemu/memory.c:566
5 0x5589a200 in memory_region_dispatch_write (mr=0x624c89e0,
addr=20, data=4587521, size=4, attrs=...) at
/home/liqiang02/qemu-upstream/qemu/memory.c:1442
6 0x55835151 in flatview_write_continue (fv=0x606e6fc0,
addr=4273930260, attrs=..., buf=0x7fffc8a18028 "\001", len=4, addr1=20, l=4,
mr=0x624c89e0) at /home/liqiang02/qemu-upstream/qemu/exec.c:3233
7 0x5583529b in flatview_write (fv=0x606e6fc0, addr=4273930260,
attrs=..., buf=0x7fffc8a18028 "\001", len=4) at
/home/liqiang02/qemu-upstream/qemu/exec.c:3272
8 0x558355a1 in address_space_write (as=0x5683ade0
, addr=4273930260, attrs=..., buf=0x7fffc8a18028 "\001",
len=4) at /home/liqiang02/qemu-upstream/qemu/exec.c:3362
9 0x558355f2 in address_space_rw (as=0x5683ade0
, addr=4273930260, attrs=..., buf=0x7fffc8a18028 "\001",
len=4, is_write=true) at /home/liqiang02/qemu-upstream/qemu/exec.c:3373
10 0x558b66ac in kvm_cpu_exec (cpu=0x63114800) at
/home/liqiang02/qemu-upstream/qemu/accel/kvm/kvm-all.c:2031
11 0x5587c3ac in qemu_kvm_cpu_thread_fn (arg=0x63114800) at
/home/liqiang02/qemu-upstream/qemu/cpus.c:1277
12 0x55e54ae6 in qemu_thread_start (args=0x6032c170) at
util/qemu-thread-posix.c:504
13 0x7fffdadbd494 in start_thread () from
/lib/x86_64-linux-gnu/libpthread.so.0
14 0x7fffdaaffacf in clone () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) q
Signed-off-by: Li Qiang
---
hw/block/nvme.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index f206391e8e..0ded74fa9a 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1223,6 +1223,11 @@ static void nvme_realize(PCIDevice *pci_dev, Error
**errp)
error_setg(errp, "serial property not set");
return;
}
+
+if (!n->num_queues) {
+error_setg(errp, "num_queues can't be zero");
+return;
+}
blkconf_blocksizes(>conf);
if (!blkconf_apply_backend_options(>conf, blk_is_read_only(n->conf.blk),
false, errp)) {
--
2.17.1
Re: [Qemu-block] [PATCH 3/4] nvme: check msix_init_exclusive_bar return value
Max Reitz 于2019年1月9日周三 下午10:52写道：
> On 30.10.18 06:18, Li Qiang wrote:
> > As this function can fail.
> >
> > Signed-off-by: Li Qiang
> > ---
> > hw/block/nvme.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 72c9644..a406c23 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1250,7 +1250,10 @@ static void nvme_realize(PCIDevice *pci_dev,
> Error **errp)
> > pci_register_bar(>parent_obj, 0,
> > PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
> > >iomem);
> > -msix_init_exclusive_bar(>parent_obj, n->num_queues, 4, NULL);
> > +if (msix_init_exclusive_bar(>parent_obj, n->num_queues, 4,
> NULL)) {
> > +error_setg(errp, "msix_init_exclusive_bar failed");
> > +return;
> > +}
>
> Commit ee640c625e1 which last touched this function stated that creating
> this device won't fail if this function fails, so I suppose it's
> intential that the error isn't checked.
>
OK, got. I will drop this patch later.
Thanks,
Li Qiang
>
>
> But if you think the error should be checked and device creation should
> be aborted if this function failed, then I suppose there'd be some
> cleaning up to instead of just returning.
>
> Also, we should just pass errp to that function (as its last parameter,
> instead of NULL).
>
> Max
>
> >
> > id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
> > id->ssvid = cpu_to_le16(pci_get_word(pci_conf +
> PCI_SUBSYSTEM_VENDOR_ID));
> >
>
>
>
Re: [Qemu-block] [PATCH 2/4] nvme: ensure the num_queues is not zero
Max Reitz 于2019年1月9日周三 下午10:38写道：
> On 30.10.18 06:18, Li Qiang wrote:
> > When it is zero, it causes segv. Backtrack:
> > Thread 5 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> > [Switching to Thread 0x7fffc6c17700 (LWP 51808)]
> > 0x55acbd90 in nvme_start_ctrl (n=0x624c8100) at
> hw/block/nvme.c:820
> > warning: Source file is more recent than executable.
> > 820 if (unlikely(n->cq[0])) {
> > (gdb) bt
> > 0 0x55acbd90 in nvme_start_ctrl (n=0x624c8100) at
> hw/block/nvme.c:820
> > 1 0x55accdbc in nvme_write_bar (n=0x624c8100, offset=20,
> data=4587521, size=4) at hw/block/nvme.c:964
> > 2 0x55acdd2b in nvme_mmio_write (opaque=0x624c8100,
> addr=20, data=4587521, size=4) at hw/block/nvme.c:1158
> > 3 0x558973ed in memory_region_write_accessor
> (mr=0x624c89e0, addr=20, value=0x7fffc6c14428, size=4, shift=0,
> mask=4294967295, attrs=...) at
> /home/liqiang02/qemu-upstream/qemu/memory.c:500
> > 4 0x55897600 in access_with_adjusted_size (addr=20,
> value=0x7fffc6c14428, size=4, access_size_min=2, access_size_max=8,
> access_fn=0x55897304 , mr=0x624c89e0,
> attrs=...) at /home/liqiang02/qemu-upstream/qemu/memory.c:566
> > 5 0x5589a200 in memory_region_dispatch_write
> (mr=0x624c89e0, addr=20, data=4587521, size=4, attrs=...) at
> /home/liqiang02/qemu-upstream/qemu/memory.c:1442
> > 6 0x55835151 in flatview_write_continue (fv=0x606e6fc0,
> addr=4273930260, attrs=..., buf=0x7fffc8a18028 "\001", len=4, addr1=20,
> l=4, mr=0x624c89e0) at /home/liqiang02/qemu-upstream/qemu/exec.c:3233
> > 7 0x5583529b in flatview_write (fv=0x606e6fc0,
> addr=4273930260, attrs=..., buf=0x7fffc8a18028 "\001", len=4) at
> /home/liqiang02/qemu-upstream/qemu/exec.c:3272
> > 8 0x558355a1 in address_space_write (as=0x5683ade0
> , addr=4273930260, attrs=..., buf=0x7fffc8a18028
> "\001", len=4) at /home/liqiang02/qemu-upstream/qemu/exec.c:3362
> > 9 0x558355f2 in address_space_rw (as=0x5683ade0
> , addr=4273930260, attrs=..., buf=0x7fffc8a18028
> "\001", len=4, is_write=true) at
> /home/liqiang02/qemu-upstream/qemu/exec.c:3373
> > 10 0x558b66ac in kvm_cpu_exec (cpu=0x63114800) at
> /home/liqiang02/qemu-upstream/qemu/accel/kvm/kvm-all.c:2031
> > 11 0x5587c3ac in qemu_kvm_cpu_thread_fn (arg=0x63114800) at
> /home/liqiang02/qemu-upstream/qemu/cpus.c:1277
> > 12 0x55e54ae6 in qemu_thread_start (args=0x60300002c170) at
> util/qemu-thread-posix.c:504
> > 13 0x7fffdadbd494 in start_thread () from
> /lib/x86_64-linux-gnu/libpthread.so.0
> > 14 0x7fffdaaffacf in clone () from /lib/x86_64-linux-gnu/libc.so.6
> > (gdb) q
> >
> > Signed-off-by: Li Qiang
> > ---
> > hw/block/nvme.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 676cc48..72c9644 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1221,6 +1221,10 @@ static void nvme_realize(PCIDevice *pci_dev,
> Error **errp)
> > error_setg(errp, "serial property not set");
> > return;
> > }
> > +
> > +if (!n->num_queues) {
> > +error_setg(errp, "num_queues can't be zero");
>
> I think we should return here.
>
>
Ok, forget this. will make a revised version later.
Thanks,
Li Qiang
> (Sorry for the late review...)
>
> Max
>
> > +}
> > blkconf_blocksizes(>conf);
> > if (!blkconf_apply_backend_options(>conf,
> blk_is_read_only(n->conf.blk),
> > false, errp)) {
> >
>
>
>
Re: [Qemu-block] [PATCH for-3.1] nvme: fix out-of-bounds access to the CMB
Paolo Bonzini 于2018年11月16日周五 下午5:31写道：
> Because the CMB BAR has a min_access_size of 2, if you read the last
> byte it will try to memcpy *2* bytes from n->cmbuf, causing an off-by-one
> error. This is CVE-2018-16847.
>
> Another way to fix this might be to register the CMB as a RAM memory
> region, which would also be more efficient. However, that might be a
> change for big-endian machines; I didn't think this through and I don't
> know how real hardware works. Add a basic testcase for the CMB in case
> somebody does this change later on.
>
> Cc: Keith Busch
> Cc: qemu-block@nongnu.org
> Cc: Li Qiang
> Signed-off-by: Paolo Bonzini
---
> hw/block/nvme.c| 2 +-
> tests/Makefile.include | 2 +-
> tests/nvme-test.c | 58 +++---
> 3 files changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 09d7c90259..5d92794ef7 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
> .write = nvme_cmb_write,
> .endianness = DEVICE_LITTLE_ENDIAN,
> .impl = {
> -.min_access_size = 2,
> +.min_access_size = 1,
> .max_access_size = 8,
> },
> };
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 613242bc6e..fb0b449c02 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -730,7 +730,7 @@ tests/test-hmp$(EXESUF): tests/test-hmp.o
> tests/machine-none-test$(EXESUF): tests/machine-none-test.o
> tests/drive_del-test$(EXESUF): tests/drive_del-test.o
> $(libqos-virtio-obj-y)
> tests/qdev-monitor-test$(EXESUF): tests/qdev-monitor-test.o
> $(libqos-pc-obj-y)
> -tests/nvme-test$(EXESUF): tests/nvme-test.o
> +tests/nvme-test$(EXESUF): tests/nvme-test.o $(libqos-pc-obj-y)
> tests/pvpanic-test$(EXESUF): tests/pvpanic-test.o
> tests/i82801b11-test$(EXESUF): tests/i82801b11-test.o
> tests/ac97-test$(EXESUF): tests/ac97-test.o
> diff --git a/tests/nvme-test.c b/tests/nvme-test.c
> index 7674a446e4..2abb3b6d19 100644
> --- a/tests/nvme-test.c
> +++ b/tests/nvme-test.c
> @@ -8,11 +8,64 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/units.h"
> #include "libqtest.h"
> +#include "libqos/libqos-pc.h"
> +
> +static QOSState *qnvme_start(const char *extra_opts)
> +{
> +QOSState *qs;
> +const char *arch = qtest_get_arch();
> +const char *cmd = "-drive id=drv0,if=none,file=null-co://,format=raw "
> + "-device nvme,addr=0x4.0,serial=foo,drive=drv0 %s";
> +
> +if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +qs = qtest_pc_boot(cmd, extra_opts ? : "");
> +global_qtest = qs->qts;
> +return qs;
> +}
> +
> +g_printerr("nvme tests are only available on x86\n");
> +exit(EXIT_FAILURE);
> +}
> +
> +static void qnvme_stop(QOSState *qs)
> +{
> +qtest_shutdown(qs);
> +}
>
> -/* Tests only initialization so far. TODO: Replace with functional tests
> */
> static void nop(void)
> {
> +QOSState *qs;
> +
> +qs = qnvme_start(NULL);
> +qnvme_stop(qs);
> +}
> +
> +static void nvmetest_cmb_test(void)
> +{
> +const int cmb_bar_size = 2 * MiB;
> +QOSState *qs;
> +QPCIDevice *pdev;
> +QPCIBar bar;
> +
> +qs = qnvme_start("-global nvme.cmb_size_mb=2");
> +pdev = qpci_device_find(qs->pcibus, QPCI_DEVFN(4,0));
> +g_assert(pdev != NULL);
> +
> +qpci_device_enable(pdev);
> +bar = qpci_iomap(pdev, 2, NULL);
> +
> +qpci_io_writel(pdev, bar, 0, 0xccbbaa99);
> +g_assert_cmpint(qpci_io_readb(pdev, bar, 0), ==, 0x99);
> +g_assert_cmpint(qpci_io_readw(pdev, bar, 0), ==, 0xaa99);
> +
> +/* Test partially out-of-bounds accesses. */
> +qpci_io_writel(pdev, bar, cmb_bar_size - 1, 0x44332211);
> +g_assert_cmpint(qpci_io_readb(pdev, bar, cmb_bar_size - 1), ==, 0x11);
> +g_assert_cmpint(qpci_io_readw(pdev, bar, cmb_bar_size - 1), !=,
> 0x2211);
> +g_assert_cmpint(qpci_io_readl(pdev, bar, cmb_bar_size - 1), !=,
> 0x44332211);
>
Here seems need a g_free(pdev),
> +qnvme_stop(qs);
> }
>
> int main(int argc, char **argv)
> @@ -21,9 +74,8 @@ int main(int argc, char **argv)
>
> g_test_init(, , NULL);
> qtest_add_func("/nvme/nop", nop);
> +qtest_add_func("/nvme/cmb_test", nvmetest_cmb_test);
>
> -qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw "
> -"-device nvme,drive=drv0,serial=foo");
> ret = g_test_run();
>
> qtest_end();
>
There is no qtest_start(), this qtest_end() seems trigger an assert in glib.
(tests/nvme-test:44053): GLib-CRITICAL **: g_hook_destroy_link: assertion
'hook != NULL' failed
Otherwise
Reviewed-by: Li Qiang
Tested-by: Li Qiang
Thanks,
Li Qiang
> --
> 2.19.1
>
>
Re: [Qemu-block] [PATCH] nvme: fix oob access issue(CVE-2018-16847)
Paolo Bonzini 于2018年11月14日周三 下午11:44写道：
> On 14/11/2018 02:38, Li Qiang wrote:
> >
> >
> > Paolo Bonzini mailto:pbonz...@redhat.com>> 于2018
> > 年11月14日周三 上午2:27写道：
> >
> > On 13/11/2018 11:17, Kevin Wolf wrote:
> > > Am 13.11.2018 um 02:45 hat Li Qiang geschrieben:
> > >> Ping what't the status of this patch.
> > >>
> > >> I see Kevin's new pr doesn't contain this patch.
> > >
> > > Oh, I thought you said that you wanted to fix this at a higher
> > level so
> > > that the problem is caught before even getting into nvme code? If
> you
> > > don't, I can apply the patch for my next pull request.
> >
> > As far as I know the bug doesn't exist. Li Qiang, if you have a
> > reproducer please send it.
> >
> >
> > Hello Paolo,
> > Though I've send the debug information and ASAN output in the mail to
> > secal...@redhat.com <mailto:secal...@redhat.com>, I'm glad provide here.
> > This is for read, I think the write the same but as the PoC is in
> > userspace, the mmap can only map the exact size of the MMIO,
> > So we can only write within the area. But if we using a module we can
> > write the out of MMIO I think
> > The nvme device's parameter should set as 'cmb_size_mb=2' and the PCI
> > address may differ in your system.
>
> Ok, thanks. I've created a reproducer using qtest (though I have to run
> now and cannot post it properly).
>
> The patch for the fix is simply:
>
>
So do you send this or me?
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fc7dacb816..6385033af3 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1192,7 +1192,7 @@ static const MemoryRegionOps nvme_cmb_ops = {
> .write = nvme_cmb_write,
> .endianness = DEVICE_LITTLE_ENDIAN,
> .impl = {
> -.min_access_size = 2,
> +.min_access_size = 1,
> .max_access_size = 8,
> },
> };
>
>
> The memory subsystem _is_ recognizing the out-of-bounds 32-bit access,
>
Thanks, this strengthen my understanding of memory subsystem.
Thanks,
Li Qiang
> but because min_access_size=2 it sends down a write at offset 2097151
> and size 2.
> Paolo
>
Re: [Qemu-block] [PATCH] nvme: fix oob access issue(CVE-2018-16847)
Paolo Bonzini 于2018年11月14日周三 上午2:27写道：
> On 13/11/2018 11:17, Kevin Wolf wrote:
> > Am 13.11.2018 um 02:45 hat Li Qiang geschrieben:
> >> Ping what't the status of this patch.
> >>
> >> I see Kevin's new pr doesn't contain this patch.
> >
> > Oh, I thought you said that you wanted to fix this at a higher level so
> > that the problem is caught before even getting into nvme code? If you
> > don't, I can apply the patch for my next pull request.
>
> As far as I know the bug doesn't exist. Li Qiang, if you have a
> reproducer please send it.
>
>
Hello Paolo,
Though I've send the debug information and ASAN output in the mail to
secal...@redhat.com, I'm glad provide here.
This is for read, I think the write the same but as the PoC is in
userspace, the mmap can only map the exact size of the MMIO,
So we can only write within the area. But if we using a module we can write
the out of MMIO I think
The nvme device's parameter should set as 'cmb_size_mb=2' and the PCI
address may differ in your system.
Thanks,
Li Qiang
#include
#include
#include
#include
#include
int main(int argc, char **argv)
{
char *filename = "/sys/bus/pci/devices/:00:04.0/resource2";
uint32_t size = 2*1024*1024;
char *mmio = NULL;
int fd = open(filename, O_RDWR);
if (fd < 0) {
printf("open file error\n");
exit(1);
}
mmio = mmap(NULL, size, PROT_WRITE | PROT_READ, MAP_SHARED, fd, 0);
if (mmio == MAP_FAILED) {
printf("mmap error\n");
exit(1);
}
int x = *(uint64_t*)(mmio+size-1);
}
read:
[Switching to Thread 0x7fffc7326700 (LWP 52799)]
Thread 4 "qemu-system-x86" hit Breakpoint 1, nvme_cmb_read
(opaque=0x624b8100, addr=2097151, size=2) at hw/block/nvme.c:1182
1182 {
(gdb) p /x addr
$1 = 0x1f
(gdb) p /x addr+size
$2 = 0x21
(gdb) c
Continuing.
=
[Thread 0x7fff77efd700 (LWP 54057) exited]
==52793==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x7fff817ff800 at pc 0x76e9af7f bp 0x7fffc7322fc0 sp 0x7fffc7322770
READ of size 2 at 0x7fff817ff800 thread T3
[Thread 0x7fff7a183700 (LWP 53957) exited]
[Thread 0x7fff786fe700 (LWP 53953) exited]
[Thread 0x7fff70b21700 (LWP 53952) exited]
#0 0x76e9af7e (/usr/lib/x86_64-linux-gnu/libasan.so.3+0x5cf7e)
#1 0x562193b3 in nvme_cmb_read hw/block/nvme.c:1186
#2 0x55d630d0 in memory_region_read_accessor
/home/liqiang02/qemu-upstream/qemu/memory.c:440
#3 0x55d638da in access_with_adjusted_size
/home/liqiang02/qemu-upstream/qemu/memory.c:570
#4 0x55d690fd in memory_region_dispatch_read1
/home/liqiang02/qemu-upstream/qemu/memory.c:1375
#5 0x55d692b5 in memory_region_dispatch_read
/home/liqiang02/qemu-upstream/qemu/memory.c:1404
#6 0x55ca765b in flatview_read_continue
/home/liqiang02/qemu-upstream/qemu/exec.c:3294
#7 0x55ca790d in flatview_read
/home/liqiang02/qemu-upstream/qemu/exec.c:3332
#8 0x55ca79d3 in address_space_read_full
/home/liqiang02/qemu-upstream/qemu/exec.c:3345
#9 0x55ca7aaa in address_space_rw
/home/liqiang02/qemu-upstream/qemu/exec.c:3375
#10 0x55daadd9 in kvm_cpu_exec
/home/liqiang02/qemu-upstream/qemu/accel/kvm/kvm-all.c:2031
#11 0x55d2b2e5 in qemu_kvm_cpu_thread_fn
/home/liqiang02/qemu-upstream/qemu/cpus.c:1277
#12 0x56a037a0 in qemu_thread_start util/qemu-thread-posix.c:498
#13 0x7fffdadbd493 in start_thread
(/lib/x86_64-linux-gnu/libpthread.so.0+0x7493)
#14 0x7fffdaafface in __clone (/lib/x86_64-linux-gnu/libc.so.6+0xe8ace)
> Prasad, please revoke the CVE.
>
> Paolo
>
>
Re: [Qemu-block] [PATCH] nvme: fix oob access issue(CVE-2018-16847)
Kevin Wolf 于2018年11月13日周二 下午6:17写道： > Am 13.11.2018 um 02:45 hat Li Qiang geschrieben: > > Ping what't the status of this patch. > > > > I see Kevin's new pr doesn't contain this patch. > > Oh, I thought you said that you wanted to fix this at a higher level so > that the problem is caught before even getting into nvme code? The higher level fix may need a lot of another discussion, but this fix is just as the other things once we did. > If you > don't, I can apply the patch for my next pull request. > I think you can. Also, could you look at another patchset about nvme to fix some small issues. Seems they hangs a long time. Thanks, Li Qiang > > Kevin >
Re: [Qemu-block] [PATCH] nvme: fix oob access issue(CVE-2018-16847)
Ping what't the status of this patch.
I see Kevin's new pr doesn't contain this patch.
Thanks,
Li Qiang
Li Qiang 于2018年11月2日周五 上午9:22写道：
> Currently, the nvme_cmb_ops mr doesn't check the addr and size.
> This can lead an oob access issue. This is triggerable in the guest.
> Add check to avoid this issue.
>
> Fixes CVE-2018-16847.
>
> Reported-by: Li Qiang
> Reviewed-by: Paolo Bonzini
> Signed-off-by: Li Qiang
> ---
> hw/block/nvme.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fc7dacb..d097add 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque, hwaddr
> addr, uint64_t data,
> unsigned size)
> {
> NvmeCtrl *n = (NvmeCtrl *)opaque;
> +
> +if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
> +return;
> +}
> memcpy(>cmbuf[addr], , size);
> }
>
> @@ -1183,6 +1187,9 @@ static uint64_t nvme_cmb_read(void *opaque, hwaddr
> addr, unsigned size)
> uint64_t val;
> NvmeCtrl *n = (NvmeCtrl *)opaque;
>
> +if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
> +return 0;
> +}
> memcpy(, >cmbuf[addr], size);
> return val;
> }
> --
> 1.8.3.1
>
>
Re: [Qemu-block] [PATCH 1/2] nvme: don't unref ctrl_mem when device unrealized
Ping...
I think this is a serious issue, can go 3.1
Thanks,
Li Qiang
Li Qiang 于2018年10月29日周一 下午2:29写道：
> Currently, when hotplug/unhotplug nvme device, it will cause an
> assert in object.c. Following is the backtrack:
>
> ERROR:qom/object.c:981:object_unref: assertion failed: (obj->ref > 0)
>
> Thread 2 "qemu-system-x86" received signal SIGABRT, Aborted.
> [Switching to Thread 0x7fffcbd32700 (LWP 18844)]
> 0x7fffdb9e4fff in raise () from /lib/x86_64-linux-gnu/libc.so.6
> (gdb) bt
> /lib/x86_64-linux-gnu/libglib-2.0.so.0
> /lib/x86_64-linux-gnu/libglib-2.0.so.0
> qom/object.c:981
> /home/liqiang02/qemu-upstream/qemu/memory.c:1732
> /home/liqiang02/qemu-upstream/qemu/memory.c:285
> util/qemu-thread-posix.c:504
> /lib/x86_64-linux-gnu/libpthread.so.0
>
> This is caused by memory_region_unref in nvme_exit.
>
> Remove it to make the PCIdevice refcount correct.
>
> Signed-off-by: Li Qiang
> ---
> hw/block/nvme.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fc7dacb816..359a06d0ad 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1331,9 +1331,6 @@ static void nvme_exit(PCIDevice *pci_dev)
> g_free(n->namespaces);
> g_free(n->cq);
> g_free(n->sq);
> -if (n->cmbsz) {
> -memory_region_unref(>ctrl_mem);
> -}
>
> msix_uninit_exclusive_bar(pci_dev);
> }
> --
> 2.11.0
>
>
Re: [Qemu-block] [PATCH] nvme: fix oob access issue(CVE-2018-16847)
Keith Busch 于2018年11月2日周五 下午11:42写道：
> On Thu, Nov 01, 2018 at 06:22:43PM -0700, Li Qiang wrote:
> > Currently, the nvme_cmb_ops mr doesn't check the addr and size.
> > This can lead an oob access issue. This is triggerable in the guest.
> > Add check to avoid this issue.
> >
> > Fixes CVE-2018-16847.
> >
> > Reported-by: Li Qiang
> > Reviewed-by: Paolo Bonzini
> > Signed-off-by: Li Qiang
>
> Hey, so why is this memory region access even considered valid if the
> request is out of range from what NVMe had registered for its
> MemoryRegion? Wouldn't it be better to not call the mr->ops->read/write
> if it's out of bounds? Otherwise every MemoryRegion needs to duplicate
> the same check, right?
>
>
Yes, This seems a good idea. Once I also encounter one issue like this.
The read callback in MR will be called even it is NULL so cause a
NULL-deref.
discussed here:
-->https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg01391.html
This touchs the core design of memory subsystem I think, May Paolo or Peter
can answer this.
> Would something like the following work (minimally tested)?
>
> ---
> diff --git a/memory.c b/memory.c
> index 9b73892768..883fd818e6 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1369,6 +1369,9 @@ bool memory_region_access_valid(MemoryRegion *mr,
> access_size_max = 4;
> }
>
> +if (addr + size > mr->size)
> +return false;
> +
> access_size = MAX(MIN(size, access_size_max), access_size_min);
> for (i = 0; i < size; i += access_size) {
> if (!mr->ops->valid.accepts(mr->opaque, addr + i, access_size,
> --
>
Re: [Qemu-block] [PATCH] nvme: fix oob access issue(CVE-2018-16847)
Kevin Wolf 于2018年11月2日周五 下午11:42写道：
> Am 02.11.2018 um 16:22 hat Li Qiang geschrieben:
> > Hello Kevin,
> >
> > Kevin Wolf 于2018年11月2日周五 下午6:54写道：
> >
> > > Am 02.11.2018 um 02:22 hat Li Qiang geschrieben:
> > > > Currently, the nvme_cmb_ops mr doesn't check the addr and size.
> > > > This can lead an oob access issue. This is triggerable in the guest.
> > > > Add check to avoid this issue.
> > > >
> > > > Fixes CVE-2018-16847.
> > > >
> > > > Reported-by: Li Qiang
> > > > Reviewed-by: Paolo Bonzini
> > > > Signed-off-by: Li Qiang
> > > > ---
> > > > hw/block/nvme.c | 7 +++
> > > > 1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > > index fc7dacb..d097add 100644
> > > > --- a/hw/block/nvme.c
> > > > +++ b/hw/block/nvme.c
> > > > @@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque,
> hwaddr
> > > addr, uint64_t data,
> > > > unsigned size)
> > > > {
> > > > NvmeCtrl *n = (NvmeCtrl *)opaque;
> > > > +
> > > > +if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
> > >
> > > What prevents a guest from moving the device to the end of the address
> > > space and causing an integer overflow in addr + size?
> > >
> > >
> > This can't happen as the addr can't be any value, it just can be in the
> > Memory Region n->ctrl_mem defines.
>
> Yes, but can't the guest map that memory region whereever it wants?
>
>
I think it can't happen, as the 'addr' here is the relative offset in the
MR.
As we don't (can't) register such a MMIO region(the end of this region is
very big to
make addr+size overflow, size here can only be 1,2, 4, 8,). So the 'addr'
here can't be that large.
Thanks,
Li Qiang
> (As Keith confirmed, the integer overflow doesn't seem to have any bad
> consequences here, but anyway.)
>
> Kevin
>
Re: [Qemu-block] [PATCH] nvme: fix oob access issue(CVE-2018-16847)
Hello Kevin,
Kevin Wolf 于2018年11月2日周五 下午6:54写道：
> Am 02.11.2018 um 02:22 hat Li Qiang geschrieben:
> > Currently, the nvme_cmb_ops mr doesn't check the addr and size.
> > This can lead an oob access issue. This is triggerable in the guest.
> > Add check to avoid this issue.
> >
> > Fixes CVE-2018-16847.
> >
> > Reported-by: Li Qiang
> > Reviewed-by: Paolo Bonzini
> > Signed-off-by: Li Qiang
> > ---
> > hw/block/nvme.c | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index fc7dacb..d097add 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque, hwaddr
> addr, uint64_t data,
> > unsigned size)
> > {
> > NvmeCtrl *n = (NvmeCtrl *)opaque;
> > +
> > +if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
>
> What prevents a guest from moving the device to the end of the address
> space and causing an integer overflow in addr + size?
>
>
This can't happen as the addr can't be any value, it just can be in the
Memory Region n->ctrl_mem defines.
Thanks,
Li Qiang
> If this happens, we still have .max_access_size = 8. The next question is
> then, is NVME_CMBSZ_GETSIZE guaranteed to be at least 8? I suppose yes,
> but do we want to rely on this for security?
Kevin
>
[Qemu-block] [PATCH] nvme: fix oob access issue(CVE-2018-16847)
Currently, the nvme_cmb_ops mr doesn't check the addr and size.
This can lead an oob access issue. This is triggerable in the guest.
Add check to avoid this issue.
Fixes CVE-2018-16847.
Reported-by: Li Qiang
Reviewed-by: Paolo Bonzini
Signed-off-by: Li Qiang
---
hw/block/nvme.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fc7dacb..d097add 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1175,6 +1175,10 @@ static void nvme_cmb_write(void *opaque, hwaddr addr,
uint64_t data,
unsigned size)
{
NvmeCtrl *n = (NvmeCtrl *)opaque;
+
+if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
+return;
+}
memcpy(>cmbuf[addr], , size);
}
@@ -1183,6 +1187,9 @@ static uint64_t nvme_cmb_read(void *opaque, hwaddr addr,
unsigned size)
uint64_t val;
NvmeCtrl *n = (NvmeCtrl *)opaque;
+if (addr + size > NVME_CMBSZ_GETSIZE(n->bar.cmbsz)) {
+return 0;
+}
memcpy(, >cmbuf[addr], size);
return val;
}
--
1.8.3.1
[Qemu-block] [PATCH 4/4] nvme: use pci_dev directly in nvme_realize
There is no need to make another reference.
Signed-off-by: Li Qiang
---
hw/block/nvme.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index a406c23..f63e344 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1235,7 +1235,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
pci_conf[PCI_INTERRUPT_PIN] = 1;
pci_config_set_prog_interface(pci_dev->config, 0x2);
pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
-pcie_endpoint_cap_init(>parent_obj, 0x80);
+pcie_endpoint_cap_init(pci_dev, 0x80);
n->num_namespaces = 1;
n->reg_size = pow2ceil(0x1004 + 2 * (n->num_queues + 1) * 4);
@@ -1247,10 +1247,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error
**errp)
memory_region_init_io(>iomem, OBJECT(n), _mmio_ops, n,
"nvme", n->reg_size);
-pci_register_bar(>parent_obj, 0,
+pci_register_bar(pci_dev, 0,
PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
>iomem);
-if (msix_init_exclusive_bar(>parent_obj, n->num_queues, 4, NULL)) {
+if (msix_init_exclusive_bar(pci_dev, n->num_queues, 4, NULL)) {
error_setg(errp, "msix_init_exclusive_bar failed");
return;
}
@@ -1308,7 +1308,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
n->cmbuf = g_malloc0(NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
memory_region_init_io(>ctrl_mem, OBJECT(n), _cmb_ops, n,
"nvme-cmb", NVME_CMBSZ_GETSIZE(n->bar.cmbsz));
-pci_register_bar(>parent_obj, NVME_CMBLOC_BIR(n->bar.cmbloc),
+pci_register_bar(pci_dev, NVME_CMBLOC_BIR(n->bar.cmbloc),
PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64 |
PCI_BASE_ADDRESS_MEM_PREFETCH, >ctrl_mem);
--
1.8.3.1
[Qemu-block] [PATCH 3/4] nvme: check msix_init_exclusive_bar return value
As this function can fail.
Signed-off-by: Li Qiang
---
hw/block/nvme.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 72c9644..a406c23 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1250,7 +1250,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error
**errp)
pci_register_bar(>parent_obj, 0,
PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64,
>iomem);
-msix_init_exclusive_bar(>parent_obj, n->num_queues, 4, NULL);
+if (msix_init_exclusive_bar(>parent_obj, n->num_queues, 4, NULL)) {
+error_setg(errp, "msix_init_exclusive_bar failed");
+return;
+}
id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID));
id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID));
--
1.8.3.1
[Qemu-block] [PATCH 1/4] nvme: use TYPE_NVME instead of constant string
Signed-off-by: Li Qiang
---
hw/block/nvme.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fc7dacb..676cc48 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1379,7 +1379,7 @@ static void nvme_instance_init(Object *obj)
}
static const TypeInfo nvme_info = {
-.name = "nvme",
+.name = TYPE_NVME,
.parent= TYPE_PCI_DEVICE,
.instance_size = sizeof(NvmeCtrl),
.class_init= nvme_class_init,
--
1.8.3.1
[Qemu-block] [PATCH 2/4] nvme: ensure the num_queues is not zero
When it is zero, it causes segv. Backtrack:
Thread 5 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffc6c17700 (LWP 51808)]
0x55acbd90 in nvme_start_ctrl (n=0x624c8100) at hw/block/nvme.c:820
warning: Source file is more recent than executable.
820 if (unlikely(n->cq[0])) {
(gdb) bt
0 0x55acbd90 in nvme_start_ctrl (n=0x624c8100) at
hw/block/nvme.c:820
1 0x55accdbc in nvme_write_bar (n=0x624c8100, offset=20,
data=4587521, size=4) at hw/block/nvme.c:964
2 0x55acdd2b in nvme_mmio_write (opaque=0x624c8100, addr=20,
data=4587521, size=4) at hw/block/nvme.c:1158
3 0x558973ed in memory_region_write_accessor (mr=0x624c89e0,
addr=20, value=0x7fffc6c14428, size=4, shift=0, mask=4294967295, attrs=...) at
/home/liqiang02/qemu-upstream/qemu/memory.c:500
4 0x55897600 in access_with_adjusted_size (addr=20,
value=0x7fffc6c14428, size=4, access_size_min=2, access_size_max=8,
access_fn=0x55897304 , mr=0x624c89e0,
attrs=...) at /home/liqiang02/qemu-upstream/qemu/memory.c:566
5 0x5589a200 in memory_region_dispatch_write (mr=0x624c89e0,
addr=20, data=4587521, size=4, attrs=...) at
/home/liqiang02/qemu-upstream/qemu/memory.c:1442
6 0x55835151 in flatview_write_continue (fv=0x606e6fc0,
addr=4273930260, attrs=..., buf=0x7fffc8a18028 "\001", len=4, addr1=20, l=4,
mr=0x624c89e0) at /home/liqiang02/qemu-upstream/qemu/exec.c:3233
7 0x5583529b in flatview_write (fv=0x606e6fc0, addr=4273930260,
attrs=..., buf=0x7fffc8a18028 "\001", len=4) at
/home/liqiang02/qemu-upstream/qemu/exec.c:3272
8 0x558355a1 in address_space_write (as=0x5683ade0
, addr=4273930260, attrs=..., buf=0x7fffc8a18028 "\001",
len=4) at /home/liqiang02/qemu-upstream/qemu/exec.c:3362
9 0x558355f2 in address_space_rw (as=0x5683ade0
, addr=4273930260, attrs=..., buf=0x7fffc8a18028 "\001",
len=4, is_write=true) at /home/liqiang02/qemu-upstream/qemu/exec.c:3373
10 0x558b66ac in kvm_cpu_exec (cpu=0x63114800) at
/home/liqiang02/qemu-upstream/qemu/accel/kvm/kvm-all.c:2031
11 0x5587c3ac in qemu_kvm_cpu_thread_fn (arg=0x63114800) at
/home/liqiang02/qemu-upstream/qemu/cpus.c:1277
12 0x55e54ae6 in qemu_thread_start (args=0x6032c170) at
util/qemu-thread-posix.c:504
13 0x7fffdadbd494 in start_thread () from
/lib/x86_64-linux-gnu/libpthread.so.0
14 0x7fffdaaffacf in clone () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) q
Signed-off-by: Li Qiang
---
hw/block/nvme.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 676cc48..72c9644 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1221,6 +1221,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error
**errp)
error_setg(errp, "serial property not set");
return;
}
+
+if (!n->num_queues) {
+error_setg(errp, "num_queues can't be zero");
+}
blkconf_blocksizes(>conf);
if (!blkconf_apply_backend_options(>conf, blk_is_read_only(n->conf.blk),
false, errp)) {
--
1.8.3.1
[Qemu-block] [PATCH 0/4] nvme: some small fixes
Including sanity check and code cleanup. Li Qiang (4): nvme: use TYPE_NVME instead of constant string nvme: ensure the num_queues is not zero nvme: check msix_init_exclusive_bar return value nvme: use pci_dev directly in nvme_realize hw/block/nvme.c | 17 - 1 file changed, 12 insertions(+), 5 deletions(-) -- 1.8.3.1
[Qemu-block] [PATCH 2/2] nvme: free cmbuf in nvme_exit
This avoid a memory leak in unhotplug nvme device.
Signed-off-by: Li Qiang
---
hw/block/nvme.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 359a06d0ad..09d7c90259 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1332,6 +1332,9 @@ static void nvme_exit(PCIDevice *pci_dev)
g_free(n->cq);
g_free(n->sq);
+if (n->cmb_size_mb) {
+g_free(n->cmbuf);
+}
msix_uninit_exclusive_bar(pci_dev);
}
--
2.11.0
[Qemu-block] [PATCH 0/2] nvme: fix two issues in nvme unhotplug
The first corrent the refcount and second fix a memory leak. Li Qiang (2): nvme: don't unref ctrl_mem when device unrealized nvme: free cmbuf in nvme_exit hw/block/nvme.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) -- 2.11.0
[Qemu-block] [PATCH 1/2] nvme: don't unref ctrl_mem when device unrealized
Currently, when hotplug/unhotplug nvme device, it will cause an
assert in object.c. Following is the backtrack:
ERROR:qom/object.c:981:object_unref: assertion failed: (obj->ref > 0)
Thread 2 "qemu-system-x86" received signal SIGABRT, Aborted.
[Switching to Thread 0x7fffcbd32700 (LWP 18844)]
0x7fffdb9e4fff in raise () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) bt
/lib/x86_64-linux-gnu/libglib-2.0.so.0
/lib/x86_64-linux-gnu/libglib-2.0.so.0
qom/object.c:981
/home/liqiang02/qemu-upstream/qemu/memory.c:1732
/home/liqiang02/qemu-upstream/qemu/memory.c:285
util/qemu-thread-posix.c:504
/lib/x86_64-linux-gnu/libpthread.so.0
This is caused by memory_region_unref in nvme_exit.
Remove it to make the PCIdevice refcount correct.
Signed-off-by: Li Qiang
---
hw/block/nvme.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fc7dacb816..359a06d0ad 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1331,9 +1331,6 @@ static void nvme_exit(PCIDevice *pci_dev)
g_free(n->namespaces);
g_free(n->cq);
g_free(n->sq);
-if (n->cmbsz) {
-memory_region_unref(>ctrl_mem);
-}
msix_uninit_exclusive_bar(pci_dev);
}
--
2.11.0
[Qemu-block] [PATCH] block: change some function return type to bool
Signed-off-by: Li Qiang
---
block/block-backend.c | 8
include/sysemu/block-backend.h | 6 +++---
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index dc0cd57724..2a8f3b55f8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1708,7 +1708,7 @@ void blk_error_action(BlockBackend *blk, BlockErrorAction
action,
}
}
-int blk_is_read_only(BlockBackend *blk)
+bool blk_is_read_only(BlockBackend *blk)
{
BlockDriverState *bs = blk_bs(blk);
@@ -1719,18 +1719,18 @@ int blk_is_read_only(BlockBackend *blk)
}
}
-int blk_is_sg(BlockBackend *blk)
+bool blk_is_sg(BlockBackend *blk)
{
BlockDriverState *bs = blk_bs(blk);
if (!bs) {
-return 0;
+return false;
}
return bdrv_is_sg(bs);
}
-int blk_enable_write_cache(BlockBackend *blk)
+bool blk_enable_write_cache(BlockBackend *blk)
{
return blk->enable_write_cache;
}
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 830d873f24..c96bcdee14 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -166,9 +166,9 @@ BlockErrorAction blk_get_error_action(BlockBackend *blk,
bool is_read,
int error);
void blk_error_action(BlockBackend *blk, BlockErrorAction action,
bool is_read, int error);
-int blk_is_read_only(BlockBackend *blk);
-int blk_is_sg(BlockBackend *blk);
-int blk_enable_write_cache(BlockBackend *blk);
+bool blk_is_read_only(BlockBackend *blk);
+bool blk_is_sg(BlockBackend *blk);
+bool blk_enable_write_cache(BlockBackend *blk);
void blk_set_enable_write_cache(BlockBackend *blk, bool wce);
void blk_invalidate_cache(BlockBackend *blk, Error **errp);
bool blk_is_inserted(BlockBackend *blk);
--
2.17.1
[Qemu-block] [PATCH] ide: piix: convert constant device name to MACRO
Signed-off-by: Li Qiang
---
hw/ide/piix.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index a3afe1f..5f29cce 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -35,6 +35,10 @@
#include "hw/ide/pci.h"
#include "trace.h"
+#define TYPE_PIIX3_IDE "piix3-ide"
+#define TYPE_PIIX3_IDE_XEN "piix3-ide-xen"
+#define TYPE_PIIX4_IDE "piix4-ide"
+
static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
{
BMDMAState *bm = opaque;
@@ -204,7 +208,7 @@ PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo
**hd_table, int devfn)
{
PCIDevice *dev;
-dev = pci_create_simple(bus, devfn, "piix3-ide-xen");
+dev = pci_create_simple(bus, devfn, TYPE_PIIX3_IDE_XEN);
pci_ide_create_devs(dev, hd_table);
return dev;
}
@@ -226,7 +230,7 @@ PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo
**hd_table, int devfn)
{
PCIDevice *dev;
-dev = pci_create_simple(bus, devfn, "piix3-ide");
+dev = pci_create_simple(bus, devfn, TYPE_PIIX3_IDE);
pci_ide_create_devs(dev, hd_table);
return dev;
}
@@ -237,7 +241,7 @@ PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo
**hd_table, int devfn)
{
PCIDevice *dev;
-dev = pci_create_simple(bus, devfn, "piix4-ide");
+dev = pci_create_simple(bus, devfn, TYPE_PIIX4_IDE);
pci_ide_create_devs(dev, hd_table);
return dev;
}
@@ -257,13 +261,13 @@ static void piix3_ide_class_init(ObjectClass *klass, void
*data)
}
static const TypeInfo piix3_ide_info = {
-.name = "piix3-ide",
+.name = TYPE_PIIX3_IDE,
.parent= TYPE_PCI_IDE,
.class_init= piix3_ide_class_init,
};
static const TypeInfo piix3_ide_xen_info = {
-.name = "piix3-ide-xen",
+.name = TYPE_PIIX3_IDE_XEN,
.parent= TYPE_PCI_IDE,
.class_init= piix3_ide_class_init,
};
@@ -283,7 +287,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void
*data)
}
static const TypeInfo piix4_ide_info = {
-.name = "piix4-ide",
+.name = TYPE_PIIX4_IDE,
.parent= TYPE_PCI_IDE,
.class_init= piix4_ide_class_init,
};
--
1.8.3.1
