Re: [Qemu-devel] [Qemu-block] [RFC 1/1] ide: bug #1777315: io_buffer_size and sg.size can represent partial sector sizes
On Wed, Jun 20, 2018 at 06:23:19AM +0530, Amol Surati wrote: > On Tue, Jun 19, 2018 at 05:43:52PM -0400, John Snow wrote: > > > > > > On 06/19/2018 05:26 PM, Amol Surati wrote: > > > On Tue, Jun 19, 2018 at 08:04:03PM +0530, Amol Surati wrote: > > >> On Tue, Jun 19, 2018 at 09:45:15AM -0400, John Snow wrote: > > >>> > > >>> > > >>> On 06/19/2018 04:53 AM, Kevin Wolf wrote: > > Am 19.06.2018 um 06:01 hat Amol Surati geschrieben: > > > On Mon, Jun 18, 2018 at 08:14:10PM -0400, John Snow wrote: > > >> > > >> > > >> On 06/18/2018 02:02 PM, Amol Surati wrote: > > >>> On Mon, Jun 18, 2018 at 12:05:15AM +0530, Amol Surati wrote: > > This patch fixes the assumption that io_buffer_size is always a > > perfect > > multiple of the sector size. The assumption is the cause of the > > firing > > of 'assert(n * 512 == s->sg.size);'. > > > > Signed-off-by: Amol Surati > > --- > > >>> > > >>> The repository https://github.com/asurati/1777315 contains a module > > >>> for > > >>> QEMU's 8086:7010 ATA controller, which exercises the code path > > >>> described in [RFC 0/1] of this series. > > >>> > > >> > > >> Thanks, this made it easier to see what was happening. I was able to > > >> write an ide-test test case using this source as a guide, and > > >> reproduce > > >> the error. > > >> > > >> static void test_bmdma_partial_sector_short_prdt(void) > > >> { > > >> QPCIDevice *dev; > > >> QPCIBar bmdma_bar, ide_bar; > > >> uint8_t status; > > >> > > >> /* Read 2 sectors but only give 1 sector in PRDT */ > > >> PrdtEntry prdt[] = { > > >> { > > >> .addr = 0, > > >> .size = cpu_to_le32(0x200), > > >> }, > > >> { > > >> .addr = 512, > > >> .size = cpu_to_le32(0x44 | PRDT_EOT), > > >> } > > >> }; > > >> > > >> dev = get_pci_device(&bmdma_bar, &ide_bar); > > >> status = send_dma_request(CMD_READ_DMA, 0, 2, > > >> prdt, ARRAY_SIZE(prdt), NULL); > > >> g_assert_cmphex(status, ==, 0); > > >> assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | > > >> ERR); > > >> free_pci_device(dev); > > >> } > > >> > > >>> Loading the module reproduces the bug. Tested on the latest master > > >>> branch. > > >>> > > >>> Steps: > > >>> - Install a Linux distribution as a guest, ensuring that the boot > > >>> disk > > >>> resides on non-IDE controllers (such as virtio) > > >>> - Attach another disk as a master device on the primary > > >>> IDE controller (i.e. attach at -hda.) > > >>> - Blacklist ata_piix, pata_acpi and ata_generic modules, and reboot. > > >>> - Copy the source files into the guest and build the module. > > >>> - Load the module. QEMU process should die with the message: > > >>> qemu-system-x86_64: hw/ide/core.c:871: ide_dma_cb: > > >>> Assertion `n * 512 == s->sg.size' failed. > > >>> > > >>> > > >>> -Amol > > >>> > > >> > > >> I'm less sure of the fix -- certainly the assert is wrong, but just > > >> incrementing 'n' is wrong too -- we didn't copy (n+1) sectors, we > > >> copied > > >> (n) and a few extra bytes. > > > > > > That is true. > > > > > > There are (at least) two fields that represent the total size of a DMA > > > transfer - > > > (1) The size, as requested through the NSECTOR field. > > > (2) The size, as calculated through the length fields of the PRD > > > entries. > > > > > > It makes sense to consider the most restrictive of the sizes, as the > > > factor > > > which determines both the end of a successful DMA transfer and the > > > condition to assert. > > > > > >> > > >> The sector-based math here would need to be adjusted to be able to > > >> cope > > >> with partial sector reads... or we ought to avoid doing any partial > > >> sector transfers. > > >> > > >> > > >> I'm not sure which is more correct tonight, it depends: > > >> > > >> - If it's OK to transfer partial sectors before reporting overflow, > > >> adjusting the command loop to work with partial sectors is OK. > > >> > > >> - If it's NOT OK to do partial sector transfer, the sglist > > >> preparation > > >> phase needs to produce a truncated SGList that's some multiple of 512 > > >> bytes that leaves the excess bytes in a second sglist that we don't > > >> throw away and can use as a basis for building the next sglist. (Or > > >> the > > >> DMA helpers need to take a max_bytes parameter and return an sglist > > >> representing unused buffer space if the command underflowed.) > > > > > > Supp
Re: [Qemu-devel] [Qemu-block] [RFC 1/1] ide: bug #1777315: io_buffer_size and sg.size can represent partial sector sizes
On Tue, Jun 19, 2018 at 05:43:52PM -0400, John Snow wrote: > > > On 06/19/2018 05:26 PM, Amol Surati wrote: > > On Tue, Jun 19, 2018 at 08:04:03PM +0530, Amol Surati wrote: > >> On Tue, Jun 19, 2018 at 09:45:15AM -0400, John Snow wrote: > >>> > >>> > >>> On 06/19/2018 04:53 AM, Kevin Wolf wrote: > Am 19.06.2018 um 06:01 hat Amol Surati geschrieben: > > On Mon, Jun 18, 2018 at 08:14:10PM -0400, John Snow wrote: > >> > >> > >> On 06/18/2018 02:02 PM, Amol Surati wrote: > >>> On Mon, Jun 18, 2018 at 12:05:15AM +0530, Amol Surati wrote: > This patch fixes the assumption that io_buffer_size is always a > perfect > multiple of the sector size. The assumption is the cause of the > firing > of 'assert(n * 512 == s->sg.size);'. > > Signed-off-by: Amol Surati > --- > >>> > >>> The repository https://github.com/asurati/1777315 contains a module > >>> for > >>> QEMU's 8086:7010 ATA controller, which exercises the code path > >>> described in [RFC 0/1] of this series. > >>> > >> > >> Thanks, this made it easier to see what was happening. I was able to > >> write an ide-test test case using this source as a guide, and reproduce > >> the error. > >> > >> static void test_bmdma_partial_sector_short_prdt(void) > >> { > >> QPCIDevice *dev; > >> QPCIBar bmdma_bar, ide_bar; > >> uint8_t status; > >> > >> /* Read 2 sectors but only give 1 sector in PRDT */ > >> PrdtEntry prdt[] = { > >> { > >> .addr = 0, > >> .size = cpu_to_le32(0x200), > >> }, > >> { > >> .addr = 512, > >> .size = cpu_to_le32(0x44 | PRDT_EOT), > >> } > >> }; > >> > >> dev = get_pci_device(&bmdma_bar, &ide_bar); > >> status = send_dma_request(CMD_READ_DMA, 0, 2, > >> prdt, ARRAY_SIZE(prdt), NULL); > >> g_assert_cmphex(status, ==, 0); > >> assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | > >> ERR); > >> free_pci_device(dev); > >> } > >> > >>> Loading the module reproduces the bug. Tested on the latest master > >>> branch. > >>> > >>> Steps: > >>> - Install a Linux distribution as a guest, ensuring that the boot disk > >>> resides on non-IDE controllers (such as virtio) > >>> - Attach another disk as a master device on the primary > >>> IDE controller (i.e. attach at -hda.) > >>> - Blacklist ata_piix, pata_acpi and ata_generic modules, and reboot. > >>> - Copy the source files into the guest and build the module. > >>> - Load the module. QEMU process should die with the message: > >>> qemu-system-x86_64: hw/ide/core.c:871: ide_dma_cb: > >>> Assertion `n * 512 == s->sg.size' failed. > >>> > >>> > >>> -Amol > >>> > >> > >> I'm less sure of the fix -- certainly the assert is wrong, but just > >> incrementing 'n' is wrong too -- we didn't copy (n+1) sectors, we > >> copied > >> (n) and a few extra bytes. > > > > That is true. > > > > There are (at least) two fields that represent the total size of a DMA > > transfer - > > (1) The size, as requested through the NSECTOR field. > > (2) The size, as calculated through the length fields of the PRD > > entries. > > > > It makes sense to consider the most restrictive of the sizes, as the > > factor > > which determines both the end of a successful DMA transfer and the > > condition to assert. > > > >> > >> The sector-based math here would need to be adjusted to be able to cope > >> with partial sector reads... or we ought to avoid doing any partial > >> sector transfers. > >> > >> > >> I'm not sure which is more correct tonight, it depends: > >> > >> - If it's OK to transfer partial sectors before reporting overflow, > >> adjusting the command loop to work with partial sectors is OK. > >> > >> - If it's NOT OK to do partial sector transfer, the sglist preparation > >> phase needs to produce a truncated SGList that's some multiple of 512 > >> bytes that leaves the excess bytes in a second sglist that we don't > >> throw away and can use as a basis for building the next sglist. (Or the > >> DMA helpers need to take a max_bytes parameter and return an sglist > >> representing unused buffer space if the command underflowed.) > > > > Support for partial sector transfers is built into the DMA interface's > > PRD > > mechanism itself, because an entry is allowed to transfer in the units > > of > > even number of bytes. > > > > I think the controller's IO process runs in two parts (probably loops > > over > > for a single transfer): > > > > (1) T
Re: [Qemu-devel] [Qemu-block] [RFC 1/1] ide: bug #1777315: io_buffer_size and sg.size can represent partial sector sizes
On 06/19/2018 05:26 PM, Amol Surati wrote: > On Tue, Jun 19, 2018 at 08:04:03PM +0530, Amol Surati wrote: >> On Tue, Jun 19, 2018 at 09:45:15AM -0400, John Snow wrote: >>> >>> >>> On 06/19/2018 04:53 AM, Kevin Wolf wrote: Am 19.06.2018 um 06:01 hat Amol Surati geschrieben: > On Mon, Jun 18, 2018 at 08:14:10PM -0400, John Snow wrote: >> >> >> On 06/18/2018 02:02 PM, Amol Surati wrote: >>> On Mon, Jun 18, 2018 at 12:05:15AM +0530, Amol Surati wrote: This patch fixes the assumption that io_buffer_size is always a perfect multiple of the sector size. The assumption is the cause of the firing of 'assert(n * 512 == s->sg.size);'. Signed-off-by: Amol Surati --- >>> >>> The repository https://github.com/asurati/1777315 contains a module for >>> QEMU's 8086:7010 ATA controller, which exercises the code path >>> described in [RFC 0/1] of this series. >>> >> >> Thanks, this made it easier to see what was happening. I was able to >> write an ide-test test case using this source as a guide, and reproduce >> the error. >> >> static void test_bmdma_partial_sector_short_prdt(void) >> { >> QPCIDevice *dev; >> QPCIBar bmdma_bar, ide_bar; >> uint8_t status; >> >> /* Read 2 sectors but only give 1 sector in PRDT */ >> PrdtEntry prdt[] = { >> { >> .addr = 0, >> .size = cpu_to_le32(0x200), >> }, >> { >> .addr = 512, >> .size = cpu_to_le32(0x44 | PRDT_EOT), >> } >> }; >> >> dev = get_pci_device(&bmdma_bar, &ide_bar); >> status = send_dma_request(CMD_READ_DMA, 0, 2, >> prdt, ARRAY_SIZE(prdt), NULL); >> g_assert_cmphex(status, ==, 0); >> assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR); >> free_pci_device(dev); >> } >> >>> Loading the module reproduces the bug. Tested on the latest master >>> branch. >>> >>> Steps: >>> - Install a Linux distribution as a guest, ensuring that the boot disk >>> resides on non-IDE controllers (such as virtio) >>> - Attach another disk as a master device on the primary >>> IDE controller (i.e. attach at -hda.) >>> - Blacklist ata_piix, pata_acpi and ata_generic modules, and reboot. >>> - Copy the source files into the guest and build the module. >>> - Load the module. QEMU process should die with the message: >>> qemu-system-x86_64: hw/ide/core.c:871: ide_dma_cb: >>> Assertion `n * 512 == s->sg.size' failed. >>> >>> >>> -Amol >>> >> >> I'm less sure of the fix -- certainly the assert is wrong, but just >> incrementing 'n' is wrong too -- we didn't copy (n+1) sectors, we copied >> (n) and a few extra bytes. > > That is true. > > There are (at least) two fields that represent the total size of a DMA > transfer - > (1) The size, as requested through the NSECTOR field. > (2) The size, as calculated through the length fields of the PRD entries. > > It makes sense to consider the most restrictive of the sizes, as the > factor > which determines both the end of a successful DMA transfer and the > condition to assert. > >> >> The sector-based math here would need to be adjusted to be able to cope >> with partial sector reads... or we ought to avoid doing any partial >> sector transfers. >> >> >> I'm not sure which is more correct tonight, it depends: >> >> - If it's OK to transfer partial sectors before reporting overflow, >> adjusting the command loop to work with partial sectors is OK. >> >> - If it's NOT OK to do partial sector transfer, the sglist preparation >> phase needs to produce a truncated SGList that's some multiple of 512 >> bytes that leaves the excess bytes in a second sglist that we don't >> throw away and can use as a basis for building the next sglist. (Or the >> DMA helpers need to take a max_bytes parameter and return an sglist >> representing unused buffer space if the command underflowed.) > > Support for partial sector transfers is built into the DMA interface's PRD > mechanism itself, because an entry is allowed to transfer in the units of > even number of bytes. > > I think the controller's IO process runs in two parts (probably loops over > for a single transfer): > > (1) The controller's disk interface transfers between its internal buffer > and the disk storage. The transfers are likely to be in the > multiples of a sector. > (2) The controller's DMA interface transfers between its internal buffer > and the system memory. The transfers can be sub-sector in size(, and > are preserving of the areas, of the inter
Re: [Qemu-devel] [Qemu-block] [RFC 1/1] ide: bug #1777315: io_buffer_size and sg.size can represent partial sector sizes
On Tue, Jun 19, 2018 at 08:04:03PM +0530, Amol Surati wrote: > On Tue, Jun 19, 2018 at 09:45:15AM -0400, John Snow wrote: > > > > > > On 06/19/2018 04:53 AM, Kevin Wolf wrote: > > > Am 19.06.2018 um 06:01 hat Amol Surati geschrieben: > > >> On Mon, Jun 18, 2018 at 08:14:10PM -0400, John Snow wrote: > > >>> > > >>> > > >>> On 06/18/2018 02:02 PM, Amol Surati wrote: > > On Mon, Jun 18, 2018 at 12:05:15AM +0530, Amol Surati wrote: > > > This patch fixes the assumption that io_buffer_size is always a > > > perfect > > > multiple of the sector size. The assumption is the cause of the firing > > > of 'assert(n * 512 == s->sg.size);'. > > > > > > Signed-off-by: Amol Surati > > > --- > > > > The repository https://github.com/asurati/1777315 contains a module for > > QEMU's 8086:7010 ATA controller, which exercises the code path > > described in [RFC 0/1] of this series. > > > > >>> > > >>> Thanks, this made it easier to see what was happening. I was able to > > >>> write an ide-test test case using this source as a guide, and reproduce > > >>> the error. > > >>> > > >>> static void test_bmdma_partial_sector_short_prdt(void) > > >>> { > > >>> QPCIDevice *dev; > > >>> QPCIBar bmdma_bar, ide_bar; > > >>> uint8_t status; > > >>> > > >>> /* Read 2 sectors but only give 1 sector in PRDT */ > > >>> PrdtEntry prdt[] = { > > >>> { > > >>> .addr = 0, > > >>> .size = cpu_to_le32(0x200), > > >>> }, > > >>> { > > >>> .addr = 512, > > >>> .size = cpu_to_le32(0x44 | PRDT_EOT), > > >>> } > > >>> }; > > >>> > > >>> dev = get_pci_device(&bmdma_bar, &ide_bar); > > >>> status = send_dma_request(CMD_READ_DMA, 0, 2, > > >>> prdt, ARRAY_SIZE(prdt), NULL); > > >>> g_assert_cmphex(status, ==, 0); > > >>> assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR); > > >>> free_pci_device(dev); > > >>> } > > >>> > > Loading the module reproduces the bug. Tested on the latest master > > branch. > > > > Steps: > > - Install a Linux distribution as a guest, ensuring that the boot disk > > resides on non-IDE controllers (such as virtio) > > - Attach another disk as a master device on the primary > > IDE controller (i.e. attach at -hda.) > > - Blacklist ata_piix, pata_acpi and ata_generic modules, and reboot. > > - Copy the source files into the guest and build the module. > > - Load the module. QEMU process should die with the message: > > qemu-system-x86_64: hw/ide/core.c:871: ide_dma_cb: > > Assertion `n * 512 == s->sg.size' failed. > > > > > > -Amol > > > > >>> > > >>> I'm less sure of the fix -- certainly the assert is wrong, but just > > >>> incrementing 'n' is wrong too -- we didn't copy (n+1) sectors, we copied > > >>> (n) and a few extra bytes. > > >> > > >> That is true. > > >> > > >> There are (at least) two fields that represent the total size of a DMA > > >> transfer - > > >> (1) The size, as requested through the NSECTOR field. > > >> (2) The size, as calculated through the length fields of the PRD entries. > > >> > > >> It makes sense to consider the most restrictive of the sizes, as the > > >> factor > > >> which determines both the end of a successful DMA transfer and the > > >> condition to assert. > > >> > > >>> > > >>> The sector-based math here would need to be adjusted to be able to cope > > >>> with partial sector reads... or we ought to avoid doing any partial > > >>> sector transfers. > > >>> > > >>> > > >>> I'm not sure which is more correct tonight, it depends: > > >>> > > >>> - If it's OK to transfer partial sectors before reporting overflow, > > >>> adjusting the command loop to work with partial sectors is OK. > > >>> > > >>> - If it's NOT OK to do partial sector transfer, the sglist preparation > > >>> phase needs to produce a truncated SGList that's some multiple of 512 > > >>> bytes that leaves the excess bytes in a second sglist that we don't > > >>> throw away and can use as a basis for building the next sglist. (Or the > > >>> DMA helpers need to take a max_bytes parameter and return an sglist > > >>> representing unused buffer space if the command underflowed.) > > >> > > >> Support for partial sector transfers is built into the DMA interface's > > >> PRD > > >> mechanism itself, because an entry is allowed to transfer in the units of > > >> even number of bytes. > > >> > > >> I think the controller's IO process runs in two parts (probably loops > > >> over > > >> for a single transfer): > > >> > > >> (1) The controller's disk interface transfers between its internal buffer > > >> and the disk storage. The transfers are likely to be in the > > >> multiples of a sector. > > >> (2) The controller's DMA interface transfers between its internal buffer > > >> and the system memory. The t
Re: [Qemu-devel] [Qemu-block] [RFC 1/1] ide: bug #1777315: io_buffer_size and sg.size can represent partial sector sizes
On Tue, Jun 19, 2018 at 09:45:15AM -0400, John Snow wrote: > > > On 06/19/2018 04:53 AM, Kevin Wolf wrote: > > Am 19.06.2018 um 06:01 hat Amol Surati geschrieben: > >> On Mon, Jun 18, 2018 at 08:14:10PM -0400, John Snow wrote: > >>> > >>> > >>> On 06/18/2018 02:02 PM, Amol Surati wrote: > On Mon, Jun 18, 2018 at 12:05:15AM +0530, Amol Surati wrote: > > This patch fixes the assumption that io_buffer_size is always a perfect > > multiple of the sector size. The assumption is the cause of the firing > > of 'assert(n * 512 == s->sg.size);'. > > > > Signed-off-by: Amol Surati > > --- > > The repository https://github.com/asurati/1777315 contains a module for > QEMU's 8086:7010 ATA controller, which exercises the code path > described in [RFC 0/1] of this series. > > >>> > >>> Thanks, this made it easier to see what was happening. I was able to > >>> write an ide-test test case using this source as a guide, and reproduce > >>> the error. > >>> > >>> static void test_bmdma_partial_sector_short_prdt(void) > >>> { > >>> QPCIDevice *dev; > >>> QPCIBar bmdma_bar, ide_bar; > >>> uint8_t status; > >>> > >>> /* Read 2 sectors but only give 1 sector in PRDT */ > >>> PrdtEntry prdt[] = { > >>> { > >>> .addr = 0, > >>> .size = cpu_to_le32(0x200), > >>> }, > >>> { > >>> .addr = 512, > >>> .size = cpu_to_le32(0x44 | PRDT_EOT), > >>> } > >>> }; > >>> > >>> dev = get_pci_device(&bmdma_bar, &ide_bar); > >>> status = send_dma_request(CMD_READ_DMA, 0, 2, > >>> prdt, ARRAY_SIZE(prdt), NULL); > >>> g_assert_cmphex(status, ==, 0); > >>> assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR); > >>> free_pci_device(dev); > >>> } > >>> > Loading the module reproduces the bug. Tested on the latest master > branch. > > Steps: > - Install a Linux distribution as a guest, ensuring that the boot disk > resides on non-IDE controllers (such as virtio) > - Attach another disk as a master device on the primary > IDE controller (i.e. attach at -hda.) > - Blacklist ata_piix, pata_acpi and ata_generic modules, and reboot. > - Copy the source files into the guest and build the module. > - Load the module. QEMU process should die with the message: > qemu-system-x86_64: hw/ide/core.c:871: ide_dma_cb: > Assertion `n * 512 == s->sg.size' failed. > > > -Amol > > >>> > >>> I'm less sure of the fix -- certainly the assert is wrong, but just > >>> incrementing 'n' is wrong too -- we didn't copy (n+1) sectors, we copied > >>> (n) and a few extra bytes. > >> > >> That is true. > >> > >> There are (at least) two fields that represent the total size of a DMA > >> transfer - > >> (1) The size, as requested through the NSECTOR field. > >> (2) The size, as calculated through the length fields of the PRD entries. > >> > >> It makes sense to consider the most restrictive of the sizes, as the factor > >> which determines both the end of a successful DMA transfer and the > >> condition to assert. > >> > >>> > >>> The sector-based math here would need to be adjusted to be able to cope > >>> with partial sector reads... or we ought to avoid doing any partial > >>> sector transfers. > >>> > >>> > >>> I'm not sure which is more correct tonight, it depends: > >>> > >>> - If it's OK to transfer partial sectors before reporting overflow, > >>> adjusting the command loop to work with partial sectors is OK. > >>> > >>> - If it's NOT OK to do partial sector transfer, the sglist preparation > >>> phase needs to produce a truncated SGList that's some multiple of 512 > >>> bytes that leaves the excess bytes in a second sglist that we don't > >>> throw away and can use as a basis for building the next sglist. (Or the > >>> DMA helpers need to take a max_bytes parameter and return an sglist > >>> representing unused buffer space if the command underflowed.) > >> > >> Support for partial sector transfers is built into the DMA interface's PRD > >> mechanism itself, because an entry is allowed to transfer in the units of > >> even number of bytes. > >> > >> I think the controller's IO process runs in two parts (probably loops over > >> for a single transfer): > >> > >> (1) The controller's disk interface transfers between its internal buffer > >> and the disk storage. The transfers are likely to be in the > >> multiples of a sector. > >> (2) The controller's DMA interface transfers between its internal buffer > >> and the system memory. The transfers can be sub-sector in size(, and > >> are preserving of the areas, of the internal buffer, not subject to a > >> write.) > > > > The spec isn't clear about this (or at least I can't find anything where > > the exact behaviour is specified), but I agree that that's my mental > > model as well. So I would make
Re: [Qemu-devel] [Qemu-block] [RFC 1/1] ide: bug #1777315: io_buffer_size and sg.size can represent partial sector sizes
On 06/19/2018 04:53 AM, Kevin Wolf wrote: > Am 19.06.2018 um 06:01 hat Amol Surati geschrieben: >> On Mon, Jun 18, 2018 at 08:14:10PM -0400, John Snow wrote: >>> >>> >>> On 06/18/2018 02:02 PM, Amol Surati wrote: On Mon, Jun 18, 2018 at 12:05:15AM +0530, Amol Surati wrote: > This patch fixes the assumption that io_buffer_size is always a perfect > multiple of the sector size. The assumption is the cause of the firing > of 'assert(n * 512 == s->sg.size);'. > > Signed-off-by: Amol Surati > --- The repository https://github.com/asurati/1777315 contains a module for QEMU's 8086:7010 ATA controller, which exercises the code path described in [RFC 0/1] of this series. >>> >>> Thanks, this made it easier to see what was happening. I was able to >>> write an ide-test test case using this source as a guide, and reproduce >>> the error. >>> >>> static void test_bmdma_partial_sector_short_prdt(void) >>> { >>> QPCIDevice *dev; >>> QPCIBar bmdma_bar, ide_bar; >>> uint8_t status; >>> >>> /* Read 2 sectors but only give 1 sector in PRDT */ >>> PrdtEntry prdt[] = { >>> { >>> .addr = 0, >>> .size = cpu_to_le32(0x200), >>> }, >>> { >>> .addr = 512, >>> .size = cpu_to_le32(0x44 | PRDT_EOT), >>> } >>> }; >>> >>> dev = get_pci_device(&bmdma_bar, &ide_bar); >>> status = send_dma_request(CMD_READ_DMA, 0, 2, >>> prdt, ARRAY_SIZE(prdt), NULL); >>> g_assert_cmphex(status, ==, 0); >>> assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR); >>> free_pci_device(dev); >>> } >>> Loading the module reproduces the bug. Tested on the latest master branch. Steps: - Install a Linux distribution as a guest, ensuring that the boot disk resides on non-IDE controllers (such as virtio) - Attach another disk as a master device on the primary IDE controller (i.e. attach at -hda.) - Blacklist ata_piix, pata_acpi and ata_generic modules, and reboot. - Copy the source files into the guest and build the module. - Load the module. QEMU process should die with the message: qemu-system-x86_64: hw/ide/core.c:871: ide_dma_cb: Assertion `n * 512 == s->sg.size' failed. -Amol >>> >>> I'm less sure of the fix -- certainly the assert is wrong, but just >>> incrementing 'n' is wrong too -- we didn't copy (n+1) sectors, we copied >>> (n) and a few extra bytes. >> >> That is true. >> >> There are (at least) two fields that represent the total size of a DMA >> transfer - >> (1) The size, as requested through the NSECTOR field. >> (2) The size, as calculated through the length fields of the PRD entries. >> >> It makes sense to consider the most restrictive of the sizes, as the factor >> which determines both the end of a successful DMA transfer and the >> condition to assert. >> >>> >>> The sector-based math here would need to be adjusted to be able to cope >>> with partial sector reads... or we ought to avoid doing any partial >>> sector transfers. >>> >>> >>> I'm not sure which is more correct tonight, it depends: >>> >>> - If it's OK to transfer partial sectors before reporting overflow, >>> adjusting the command loop to work with partial sectors is OK. >>> >>> - If it's NOT OK to do partial sector transfer, the sglist preparation >>> phase needs to produce a truncated SGList that's some multiple of 512 >>> bytes that leaves the excess bytes in a second sglist that we don't >>> throw away and can use as a basis for building the next sglist. (Or the >>> DMA helpers need to take a max_bytes parameter and return an sglist >>> representing unused buffer space if the command underflowed.) >> >> Support for partial sector transfers is built into the DMA interface's PRD >> mechanism itself, because an entry is allowed to transfer in the units of >> even number of bytes. >> >> I think the controller's IO process runs in two parts (probably loops over >> for a single transfer): >> >> (1) The controller's disk interface transfers between its internal buffer >> and the disk storage. The transfers are likely to be in the >> multiples of a sector. >> (2) The controller's DMA interface transfers between its internal buffer >> and the system memory. The transfers can be sub-sector in size(, and >> are preserving of the areas, of the internal buffer, not subject to a >> write.) > > The spec isn't clear about this (or at least I can't find anything where > the exact behaviour is specified), but I agree that that's my mental > model as well. So I would make IDE send a byte-granularity request with > the final partial sector to the block layer, so that the data is > actually transferred up to that point. > > In practice it probably doesn't matter much because a too short PRDT > means that the request doesn't complete successfully (the condition is >
Re: [Qemu-devel] [Qemu-block] [RFC 1/1] ide: bug #1777315: io_buffer_size and sg.size can represent partial sector sizes
Am 19.06.2018 um 06:01 hat Amol Surati geschrieben: > On Mon, Jun 18, 2018 at 08:14:10PM -0400, John Snow wrote: > > > > > > On 06/18/2018 02:02 PM, Amol Surati wrote: > > > On Mon, Jun 18, 2018 at 12:05:15AM +0530, Amol Surati wrote: > > >> This patch fixes the assumption that io_buffer_size is always a perfect > > >> multiple of the sector size. The assumption is the cause of the firing > > >> of 'assert(n * 512 == s->sg.size);'. > > >> > > >> Signed-off-by: Amol Surati > > >> --- > > > > > > The repository https://github.com/asurati/1777315 contains a module for > > > QEMU's 8086:7010 ATA controller, which exercises the code path > > > described in [RFC 0/1] of this series. > > > > > > > Thanks, this made it easier to see what was happening. I was able to > > write an ide-test test case using this source as a guide, and reproduce > > the error. > > > > static void test_bmdma_partial_sector_short_prdt(void) > > { > > QPCIDevice *dev; > > QPCIBar bmdma_bar, ide_bar; > > uint8_t status; > > > > /* Read 2 sectors but only give 1 sector in PRDT */ > > PrdtEntry prdt[] = { > > { > > .addr = 0, > > .size = cpu_to_le32(0x200), > > }, > > { > > .addr = 512, > > .size = cpu_to_le32(0x44 | PRDT_EOT), > > } > > }; > > > > dev = get_pci_device(&bmdma_bar, &ide_bar); > > status = send_dma_request(CMD_READ_DMA, 0, 2, > > prdt, ARRAY_SIZE(prdt), NULL); > > g_assert_cmphex(status, ==, 0); > > assert_bit_clear(qpci_io_readb(dev, ide_bar, reg_status), DF | ERR); > > free_pci_device(dev); > > } > > > > > Loading the module reproduces the bug. Tested on the latest master > > > branch. > > > > > > Steps: > > > - Install a Linux distribution as a guest, ensuring that the boot disk > > > resides on non-IDE controllers (such as virtio) > > > - Attach another disk as a master device on the primary > > > IDE controller (i.e. attach at -hda.) > > > - Blacklist ata_piix, pata_acpi and ata_generic modules, and reboot. > > > - Copy the source files into the guest and build the module. > > > - Load the module. QEMU process should die with the message: > > > qemu-system-x86_64: hw/ide/core.c:871: ide_dma_cb: > > > Assertion `n * 512 == s->sg.size' failed. > > > > > > > > > -Amol > > > > > > > I'm less sure of the fix -- certainly the assert is wrong, but just > > incrementing 'n' is wrong too -- we didn't copy (n+1) sectors, we copied > > (n) and a few extra bytes. > > That is true. > > There are (at least) two fields that represent the total size of a DMA > transfer - > (1) The size, as requested through the NSECTOR field. > (2) The size, as calculated through the length fields of the PRD entries. > > It makes sense to consider the most restrictive of the sizes, as the factor > which determines both the end of a successful DMA transfer and the > condition to assert. > > > > > The sector-based math here would need to be adjusted to be able to cope > > with partial sector reads... or we ought to avoid doing any partial > > sector transfers. > > > > > > I'm not sure which is more correct tonight, it depends: > > > > - If it's OK to transfer partial sectors before reporting overflow, > > adjusting the command loop to work with partial sectors is OK. > > > > - If it's NOT OK to do partial sector transfer, the sglist preparation > > phase needs to produce a truncated SGList that's some multiple of 512 > > bytes that leaves the excess bytes in a second sglist that we don't > > throw away and can use as a basis for building the next sglist. (Or the > > DMA helpers need to take a max_bytes parameter and return an sglist > > representing unused buffer space if the command underflowed.) > > Support for partial sector transfers is built into the DMA interface's PRD > mechanism itself, because an entry is allowed to transfer in the units of > even number of bytes. > > I think the controller's IO process runs in two parts (probably loops over > for a single transfer): > > (1) The controller's disk interface transfers between its internal buffer > and the disk storage. The transfers are likely to be in the > multiples of a sector. > (2) The controller's DMA interface transfers between its internal buffer > and the system memory. The transfers can be sub-sector in size(, and > are preserving of the areas, of the internal buffer, not subject to a > write.) The spec isn't clear about this (or at least I can't find anything where the exact behaviour is specified), but I agree that that's my mental model as well. So I would make IDE send a byte-granularity request with the final partial sector to the block layer, so that the data is actually transferred up to that point. In practice it probably doesn't matter much because a too short PRDT means that the request doesn't complete successfully (the condition is indicated by clear Interrupt, Active an