Re: [Qemu-block] [Qemu-devel] reduce write bandwidth of qcow2 driver while allocating new cluster
On Mon, Aug 28, 2017 at 10:46:34AM -0500, Eric Blake wrote: > [adding qemu-block] > > On 08/28/2017 12:56 AM, Liu Qing wrote: > > Dear list, > > Recently I used fio to test qcow2 driver in the guest os, and found out > > that when a new cluster is allocated the 4K IO will occupy 64K(default > > cluster > > size) bandwith. > > From the code qcow2 driver will fill the unused part of new allocated > > cluster with 0 in perform_cow. These 0s are set in qcow2_co_readv when the > > read > > destination is not allocated and it has no backing file. Could I forbidden > > any > > further write in copy_sectors if the copy source is not allocated and it has > > no backing file? So only the requested data is written to the cluster. > > Function > > copy_sectors is only used by perform_cow in the master branch. > > There have already been discussions on optimizing COW writes in a manner > similar to what you are describing; for example, > > https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00109.html Thanks Eric, this is what I am looking for. The only concern I have is in patch '[Qemu-devel] [PATCH v4 12/15] qcow2: skip writing zero buffers to empty' it says: It can be detected that 1. COW alignment of a write request is zeroes 2. Respective areas on the underlying BDS already read as zeroes after being preallocated previously If both of these true, COW may be skipped Will writing zero be skipped if the disk is not preallocated? @Anton BTW: why the code in the patch is a little different than the latest master branch? For example I don't have the is_zero function but only get is_zero_sectors. Is there something wrong with my settings? My repo: # git remote -v origin git://git.qemu-project.org/qemu.git (fetch) origin git://git.qemu-project.org/qemu.git (push) Thanks. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org >
Re: [Qemu-block] [Qemu-devel] reduce write bandwidth of qcow2 driver while allocating new cluster
On Mon, Aug 28, 2017 at 05:40:48PM -0400, John Snow wrote: > > > On 08/28/2017 01:56 AM, Liu Qing wrote: > > Dear list, > > Recently I used fio to test qcow2 driver in the guest os, and found out > > that when a new cluster is allocated the 4K IO will occupy 64K(default > > cluster > > size) bandwith. > > From the code qcow2 driver will fill the unused part of new allocated > > cluster with 0 in perform_cow. These 0s are set in qcow2_co_readv when the > > read > > destination is not allocated and it has no backing file. Could I forbidden > > any > > further write in copy_sectors if the copy source is not allocated and it has > > no backing file? So only the requested data is written to the cluster. > > Function > > copy_sectors is only used by perform_cow in the master branch. > > Do you think this change is reasonable? Thanks. > > > > > > CCing qemu-block. > > If I am understanding you correctly, you're noticing an adverse > performance impact from initial writes to unallocated clusters through > the qcow2 driver, when qcow2 performs zero-fill of uninitiated clusters. Yes, that's it. > > First, can you look at sources for current master branch? it hasn't been > named "copy_sectors" since June 2016, and the function looks a bit > differently than it did in the version you're looking at, I think. I checkout the latest master branch and the code path has some changes, but the zero-fill process still exists. Eric has replied a patch from Anton which could improve the write performance. Thanks all. > > John >
Re: [Qemu-block] [Qemu-devel] [PATCH 4/5] pci: Add INTERFACE_LEGACY_PCI_DEVICE to legacy PCI devices
On Mon, Aug 28, 2017 at 06:58:37PM -0400, John Snow wrote: > > > On 08/25/2017 03:39 PM, Eduardo Habkost wrote: > > CCing maintainers of affected devices (sorry for not CCing you > > before). > > > > On Wed, Aug 23, 2017 at 07:14:44PM -0300, Eduardo Habkost wrote: > >> Add INTERFACE_LEGACY_PCI_DEVICE to all direct subtypes of > >> TYPE_PCI_DEVICE, except: > >> > >> 1) The ones that already have INTERFACE_PCIE_DEVICE set: > >> > >> * base-xhci > >> * e1000e > >> * nvme > >> * pvscsi > >> * vfio-pci > >> * virtio-pci > >> * vmxnet3 > >> > >> 2) base-pci-bridge > >> > >> Not all PCI bridges are legacy PCI devices, so > >> INTERFACE_LEGACY_PCI_DEVICE is added only to the subtypes that > >> are actually legacy PCI devices: > >> > >> * dec-21154-p2p-bridge > >> * i82801b11-bridge > >> * pbm-bridge > >> * pci-bridge > >> > >> The direct subtypes of base-pci-bridge not touched by this patch > >> are: > >> > >> * xilinx-pcie-root: Already marked as PCIe-only device. > >> * pcie-port: all non-abstract subtypes of pcie-port are already > >> marked as PCIe-only devices. > >> > >> 3) megasas-base > >> > >> Not all megasas devices are legacy PCI devices, so the interface > >> names are added to the subclasses registered by > >> megasas_register_types(), according to information in the > >> megasas_devices[] array. > >> > >> "megasas-gen2" already implements INTERFACE_PCIE_DEVICE, so add > >> INTERFACE_LEGACY_PCI_DEVICE only to "megasas". > >> > >> Signed-off-by: Eduardo Habkost > >> --- > > [...] > > >> hw/ide/ich.c| 4 > >> hw/ide/pci.c| 4 > > Acked-by: John Snow > > > (Random fly-by comment without looking at the other patches: I assume > there are reasons it's not appropriate or good to add a legacy PCI > device parent that we inherit from, and it's instead better to manually > add the property to all children?) Yes, the reason I'm using interfaces instead of regular inheritance is the existence of hybrid devices (see patch 2/5). -- Eduardo
Re: [Qemu-block] [Qemu-devel] Persistent bitmaps for non-qcow2 formats
On 08/25/2017 09:44 AM, Max Reitz wrote:
> On 2017-08-25 02:55, John Snow wrote:
>> Sorry in advance for :words: ...
>>
>> On 08/23/2017 02:04 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> 23.08.2017 11:59, Vladimir Sementsov-Ogievskiy wrote:
22.08.2017 22:07, John Snow wrote:
[snip]
Should there be some problems with internal snapshots and other things?
>
> I'd suspect you get exactly the same problems when using internal
> snapshots together with backing files. Imagine a newly created overlay
> file and taking a snapshot. This should give you exactly the same
> issue, doesn't it?
>
>>> Hm. looks like that this backing file should not only receive all reads
>>> and writes, but almost everything ->bdrv_ handlers, except bitmap
>>> related of course.
>
> How so? Shouldn't it just work like a backing file, except it also
> receives writes instead of just reads?
>
>>>This doesn't seems simple to implement. Especially if
>>> imaging some not-raw feature-full format under this thin qcow2 layer..
>>> Or we can restrict this RW backing file to be raw-only?
>>>
>>>
>>
>> The idea would really be to support any arbitrary data store, so I
>> wouldn't want to restrict it to just raw.
>>
>> You're right though, this might be a kind of messy approach.
>>
>> [From your other mail:]
>>
>>>
>>> So, anyway, I see only two differences (from the outside) between this
>>> approach and just a separate bitmap-only qcow2 without a data:
>>>
>>
>> It's very delicately similar, yes.
>>
>>> 1. in RW-backing approach qcow2-bitmap file has a link to data file (as a
>>> backing). It looks good.
>
> And this is rather important to me.
>
Good to know. Some good solid opinions to work around. ;)
>> Right. The information necessary to establish a link between the bitmap
>> data and the data being described is fully contained within a file fully
>> specified by the QCOW2 spec.
>>
>>> 2. in RW-backing approach qcow2-bitmap file is a top of the virtual disk,
>>> in separate-file approach it is an option of the real data drive. In my
>>> opinion the second is more clean for users ("to add this feature you should
>>> use other file as your disk" vs "to add this feature you should add an
>>> option to your disk description")
>
> I'd argue it's rather: "You cannot use this feature unless your format
> supports it. The only format supporting persistent bitmaps currently is
> qcow2. To use persistent bitmaps with other formats you can attach them
> as R/W backing files to an empty qcow2 file, though."
>
> So the difference is that you are saying it's a feature that is added to
> a non-qcow2 image whereas I'm saying it's a feature that only a qcow2
> image can provide (currently).
>
>> This puts us a little closer to the original idea that was rejected by
>> Kevin at the time. To recap:
>>
>> "1": Use qcow2 as a container. This was rejected because we didn't want
>> qcow2 containing data with no semantic relationship to the qcow2
>> container or to each other. The way it sounds like you're proposing it,
>> though, it would be one-qcow2-with-bitmaps-per-drive, so the data would
>> at least stay strictly related, but it would be meaningless outside of
>> QEMU itself. I think this is something that Kevin wanted to avoid, but I
>> can't speak for him.
>>
>> It's certainly not beyond the realm of management software to remember
>> to correlate a qcow2 metadata file alongside its actual data stores
>> whenever it needed to do so, but it does mean the introduction of a
>> feature that essentially requires the use of management software, which
>> sees resistance in the community at times.
>>
>> In this model, you'd probably have the raw drive at the top, with the
>> qcow2-with-bitmaps as a child node with some kind of new named child
>> relationship. All IO stays at the root node, but the bitmap method
>> handlers would know to look for this special bitmap-child. It shouldn't
>> be too hard to implement.
>
> I'd still like to throw in how much I dislike this approach, and I can't
> really think of a way to make it palatable to me. Not even "just write
> the file name of the image the bitmaps cover into the qcow2 file" sounds
> good to me, because then it still is basically unrelated data.
>
Understood. It's something I'd like to avoid too, but I have some real
concerns about implementation of that semantic link.
> The only approach that I might see myself liking is to indeed add a flag
> or whatever to say a qcow2 backing image is supposed to be R/W; and then
> (after somehow verifying that the qcow2 image itself is empty) just make
> qemu interpret this as "load the backing file as the real disk and
> attach the qcow2 image as a 'metadata' child" or whatever. But I fear
> this gets uglier and uglier because how qemu loads the files will then
> depend on whether the overlay is empty or not, and this may be very
> confusing.
>
Right, it makes opening a little more convoluted t
Re: [Qemu-block] [Qemu-devel] [PATCH 7/9] AHCI: Rework IRQ constants
On 08/25/2017 10:00 AM, Philippe Mathieu-Daudé wrote:
> Hi John,
>
> On 08/08/2017 03:33 PM, John Snow wrote:
>> Create a new enum so that we can name the IRQ bits, which will make
>> debugging
>> them a little nicer if we can print them out. Not handled in this
>> patch, but
>> this will make it possible to get a nice debug printf detailing
>> exactly which
>> status bits are set, as it can be multiple at any given time.
>>
>> As a consequence of this patch, it is no longer possible to set
>> multiple IRQ
>> codes at once, but nothing was utilizing this ability anyway.
>>
>> Signed-off-by: John Snow
>> ---
>> hw/ide/ahci.c | 49
>> ++---
>> hw/ide/ahci_internal.h | 44
>> +++-
>> hw/ide/trace-events| 2 +-
>> 3 files changed, 74 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index d5acceb..c5a9b69 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -56,6 +56,27 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
>> static void ahci_unmap_clb_address(AHCIDevice *ad);
>> static void ahci_unmap_fis_address(AHCIDevice *ad);
>> +static const char *AHCIPortIRQ_lookup[AHCI_PORT_IRQ__END] = {
>> +[AHCI_PORT_IRQ_BIT_DHRS] = "DHRS",
>> +[AHCI_PORT_IRQ_BIT_PSS] = "PSS",
>> +[AHCI_PORT_IRQ_BIT_DSS] = "DSS",
>> +[AHCI_PORT_IRQ_BIT_SDBS] = "SDBS",
>> +[AHCI_PORT_IRQ_BIT_UFS] = "UFS",
>> +[AHCI_PORT_IRQ_BIT_DPS] = "DPS",
>> +[AHCI_PORT_IRQ_BIT_PCS] = "PCS",
>> +[AHCI_PORT_IRQ_BIT_DMPS] = "DMPS",
>> +[8 ... 21] = "RESERVED",
>> +[AHCI_PORT_IRQ_BIT_PRCS] = "PRCS",
>> +[AHCI_PORT_IRQ_BIT_IPMS] = "IPMS",
>> +[AHCI_PORT_IRQ_BIT_OFS] = "OFS",
>> +[25] = "RESERVED",
>> +[AHCI_PORT_IRQ_BIT_INFS] = "INFS",
>> +[AHCI_PORT_IRQ_BIT_IFS] = "IFS",
>> +[AHCI_PORT_IRQ_BIT_HBDS] = "HBDS",
>> +[AHCI_PORT_IRQ_BIT_HBFS] = "HBFS",
>> +[AHCI_PORT_IRQ_BIT_TFES] = "TFES",
>> +[AHCI_PORT_IRQ_BIT_CPDS] = "CPDS"
>> +};
>> static uint32_t ahci_port_read(AHCIState *s, int port, int offset)
>> {
>> @@ -170,12 +191,18 @@ static void ahci_check_irq(AHCIState *s)
>> }
>> static void ahci_trigger_irq(AHCIState *s, AHCIDevice *d,
>> - int irq_type)
>> + enum AHCIPortIRQ irqbit)
>> {
>> -DPRINTF(d->port_no, "trigger irq %#x -> %x\n",
>> -irq_type, d->port_regs.irq_mask & irq_type);
>> +g_assert(irqbit >= 0 && irqbit < 32);
>
> Since you now use an enum this check is no more necessary.
>
You can actually still pass in a wrong value if you wanted to. This is
to prevent old-style IRQ masks from getting passed in by accident.
> However ...
>
>> +uint32_t irq = 1U << irqbit;
>> +uint32_t irqstat = d->port_regs.irq_stat | irq;
>> -d->port_regs.irq_stat |= irq_type;
>> +trace_ahci_trigger_irq(s, d->port_no,
>> + AHCIPortIRQ_lookup[irqbit], irq,
>> + d->port_regs.irq_stat, irqstat,
>> + irqstat & d->port_regs.irq_mask);
>> +
>
> Why not keep using masked irqs, and iterate over AHCI_PORT_IRQ__END
> bits? Something like:
>
> if (TRACE_AHCI_IRQ_ENABLED) {
> int irqbit;
> for (irqbit = 0; irqbit < AHCI_PORT_IRQ__END; irqbit++) {
> if (irqmask & BIT(irqbit)) {
> trace_ahci_trigger_irq(s, d->port_no, ...
>
Would rather not iterate like that for the IRQ path. Generally no place
in the codebase needs to raise more than one IRQ type at a time, so why
bother allowing it?
Re: [Qemu-block] [Qemu-devel] [PATCH 5/9] IDE: replace DEBUG_AIO with trace events
On 08/25/2017 09:46 AM, Philippe Mathieu-Daudé wrote:
> Hi John,
>
> On 08/08/2017 03:33 PM, John Snow wrote:
>> Signed-off-by: John Snow
>> ---
>> hw/ide/atapi.c| 5 +
>> hw/ide/core.c | 17 ++---
>> hw/ide/trace-events | 3 +++
>> include/hw/ide/internal.h | 7 +--
>> 4 files changed, 19 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index 37fa699..b8fc51e 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -416,10 +416,7 @@ static void ide_atapi_cmd_read_dma_cb(void
>> *opaque, int ret)
>> s->io_buffer_size = n * 2048;
>> data_offset = 0;
>> }
>> -#ifdef DEBUG_AIO
>> -printf("aio_read_cd: lba=%u n=%d\n", s->lba, n);
>> -#endif
>> -
>> +trace_ide_atapi_cmd_read_dma_cb_aio(s, s->lba, n);
>> s->bus->dma->iov.iov_base = (void *)(s->io_buffer + data_offset);
>> s->bus->dma->iov.iov_len = n * ATAPI_SECTOR_SIZE;
>> qemu_iovec_init_external(&s->bus->dma->qiov, &s->bus->dma->iov, 1);
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 29848ff..1358029 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -58,6 +58,13 @@ static const int smart_attributes[][12] = {
>> { 190, 0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00,
>> 0x00, 0x32},
>> };
>> +const char *IDE_DMA_CMD_lookup[IDE_DMA__END] = {
>> +[IDE_DMA_READ] = "DMA_READ",
>> +[IDE_DMA_WRITE] = "DMA_WRITE",
>> +[IDE_DMA_TRIM] = "DMA TRIM",
>> +[IDE_DMA_ATAPI] = "DMA ATAPI"
>> +};
>> +
>> static void ide_dummy_transfer_stop(IDEState *s);
>> static void padstr(char *str, const char *src, int len)
>> @@ -860,10 +867,8 @@ static void ide_dma_cb(void *opaque, int ret)
>> goto eot;
>> }
>> -#ifdef DEBUG_AIO
>> -printf("ide_dma_cb: sector_num=%" PRId64 " n=%d, cmd_cmd=%d\n",
>> - sector_num, n, s->dma_cmd);
>> -#endif
>> +trace_ide_dma_cb(s, sector_num, n,
>> + IDE_DMA_CMD_lookup[s->dma_cmd]);
>> if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd ==
>> IDE_DMA_WRITE) &&
>> !ide_sect_range_ok(s, sector_num, n)) {
>> @@ -2383,9 +2388,7 @@ void ide_bus_reset(IDEBus *bus)
>> /* pending async DMA */
>> if (bus->dma->aiocb) {
>> -#ifdef DEBUG_AIO
>> -printf("aio_cancel\n");
>> -#endif
>> +trace_ide_bus_reset_aio();
>> blk_aio_cancel(bus->dma->aiocb);
>> bus->dma->aiocb = NULL;
>> }
>> diff --git a/hw/ide/trace-events b/hw/ide/trace-events
>> index ade1e76..6e33cb3 100644
>> --- a/hw/ide/trace-events
>> +++ b/hw/ide/trace-events
>> @@ -17,6 +17,8 @@ ide_cancel_dma_sync_remaining(void) "draining all
>> remaining requests"
>> ide_sector_read(int64_t sector_num, int nsectors) "sector=%"PRId64"
>> nsectors=%d"
>> ide_sector_write(int64_t sector_num, int nsectors) "sector=%"PRId64"
>> nsectors=%d"
>> ide_reset(void *s) "IDEstate %p"
>> +ide_bus_reset_aio(void) "aio_cancel"
>> +ide_dma_cb(void *s, int64_t sector_num, int n, const char *dma)
>> "IDEState %p; sector_num=%"PRId64" n=%d cmd=%s"
>> # hw/ide/pci.c
>> bmdma_reset(void) ""
>> @@ -48,5 +50,6 @@ ide_atapi_cmd_reply_end_new(void *s, int status)
>> "IDEState: %p; new transfer sta
>> ide_atapi_cmd_check_status(void *s) "IDEState: %p"
>> ide_atapi_cmd_read(void *s, const char *method, int lba, int
>> nb_sectors) "IDEState: %p; read %s: LBA=%d nb_sectors=%d"
>> ide_atapi_cmd(void *s, uint8_t cmd) "IDEState: %p; cmd: 0x%02x"
>> +ide_atapi_cmd_read_dma_cb_aio(void *s, int lba, int n) "IDEState: %p;
>> aio read: lba=%d n=%d"
>> # Warning: Verbose
>> ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet)
>> "IDEState: %p; limit=0x%x packet: %s"
>> \ No newline at end of file
>> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
>> index 74efe8a..97e97a2 100644
>> --- a/include/hw/ide/internal.h
>> +++ b/include/hw/ide/internal.h
>> @@ -14,7 +14,6 @@
>> #include "block/scsi.h"
>> /* debug IDE devices */
>> -//#define DEBUG_AIO
>> #define USE_DMA_CDROM
>> typedef struct IDEBus IDEBus;
>> @@ -333,12 +332,16 @@ struct unreported_events {
>> };
>> enum ide_dma_cmd {
>> -IDE_DMA_READ,
>> +IDE_DMA__BEGIN = 0,
>> +IDE_DMA_READ = IDE_DMA__BEGIN,
>
> not that useful, you can use directly:
>
> IDE_DMA_READ = 0,
>
I could do lots of things; one of the things I like to do out of habit
is to name an enumerator after the beginning so that if I were to ever
loop over the items contained within, I can write a loop that looks like
this:
for (i = IDE_DMA__BEGIN; i < IDE_DMA__END; i++) {
...foo...;
}
instead of having to name a specific constant in the beginning.
Why?
Because, heaven help us, someone reorders the enum for whatever reason,
the program is still going to compile and nobody will notice this change
halfway across the globe.
Having a distinct __START __END (or even COUNT as you suggest) clues us
in to the notion t
Re: [Qemu-block] [Qemu-devel] [PATCH 9/9] AHCI: remove DPRINTF macro
On 08/25/2017 09:48 AM, Philippe Mathieu-Daudé wrote: > On 08/08/2017 03:33 PM, John Snow wrote: >> Signed-off-by: John Snow > > Reviewed-by: Philippe Mathieu-Daudé > :) I'll wait on V2 to hear back. Thank you for your feedback so far.
Re: [Qemu-block] [Qemu-devel] [PATCH 6/9] AHCI: Replace DPRINTF with trace-events
On 08/25/2017 09:17 AM, Philippe Mathieu-Daudé wrote: > Hi John, > > On 08/08/2017 03:33 PM, John Snow wrote: >> There are a few hangers-on that will be dealt with individually >> in forthcoming patches. >> >> Signed-off-by: John Snow >> --- >> hw/ide/ahci.c | 157 >> +++- >> hw/ide/trace-events | 52 - >> 2 files changed, 119 insertions(+), 90 deletions(-) >> snip, snip >> diff --git a/hw/ide/trace-events b/hw/ide/trace-events >> index 6e33cb3..e1a0457 100644 >> --- a/hw/ide/trace-events >> +++ b/hw/ide/trace-events >> @@ -52,4 +52,54 @@ ide_atapi_cmd_read(void *s, const char *method, int >> lba, int nb_sectors) "IDESta >> ide_atapi_cmd(void *s, uint8_t cmd) "IDEState: %p; cmd: 0x%02x" >> ide_atapi_cmd_read_dma_cb_aio(void *s, int lba, int n) "IDEState: >> %p; aio read: lba=%d n=%d" >> # Warning: Verbose >> -ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet) >> "IDEState: %p; limit=0x%x packet: %s" >> \ No newline at end of file >> +ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet) >> "IDEState: %p; limit=0x%x packet: %s" >> + >> +# hw/ide/ahci.c >> +ahci_port_read(void *s, int port, int offset, uint32_t ret) >> "ahci(%p)[%d]: port read @ 0x%x: 0x%08x" >> +ahci_irq_raise(void *s) "ahci(%p): raise irq" >> +ahci_irq_lower(void *s) "ahci(%p): lower irq" >> +ahci_check_irq(void *s, uint32_t old, uint32_t new) "ahci(%p): check >> irq 0x%08x --> 0x%08x" >> + >> +ahci_port_write(void *s, int port, int offset, uint32_t val) >> "ahci(%p)[%d]: port write @ 0x%x: 0x%08x" >> +ahci_mem_read_32(void *s, uint64_t addr, uint32_t val) "ahci(%p): mem >> read @ 0x%"PRIx64": 0x%08x" > > can you use same format than ahci_mem_read()? > > "ahci(%p): read%u @ 0x%"PRIx64": 0x%08x" > >> +ahci_mem_read(void *s, unsigned size, uint64_t addr, uint64_t val) >> "ahci(%p): read%u @ 0x%"PRIx64": 0x%016"PRIx64 >> +ahci_mem_write(void *s, unsigned size, uint64_t addr, uint64_t val) >> "ahci(%p): write%u @ 0x%"PRIx64": 0x%016"PRIx64 >> +ahci_mem_write_unknown(void *s, uint64_t addr) "ahci(%p): write to >> unknown register 0x%"PRIx64 > > report the value written, eventually: > > "ahci(%p): write%u @ 0x%"PRIx64": 0x%016"PRIx64" UNKNOWN" > Not necessary here; it's reported by the more generic trace_ahci_mem_write which gets all of the writes no matter where they go. In this case, too, the write is actually ignored and doesn't wind up going anywhere ... ... but I suppose it wouldn't hurt to know what that value would have been. If we enable just this trace that will probably help illuminate what the errant write is. >> +ahci_set_signature(void *s, int port, uint8_t nsector, uint8_t >> sector, uint8_t lcyl, uint8_t hcyl, uint32_t sig) "ahci(%p)[%d]: set >> signature sector:0x%02x nsector:0x%02x lcyl:0x%02x hcyl:0x%02x >> (cumulatively: 0x%08x)" >> +ahci_reset_port(void *s, int port) "ahci(%p)[%d]: reset port" > > could be generic: > > ahci_port(void *s, int port) "ahci(%p)[%d]: %s" > > then > > trace_ahci_port(s, port, "reset port"); > I don't disagree, just more trepidation on what that means for the trace events which are otherwise all named exactly for the functions that call them. Also, the problem with combining these sorts of traces becomes one with the DPRINTF we're replacing: you can't target individual sections of code anymore, and you either turn on "ahci_port" or off. If there is a better suggestion for avoiding the ahci(%p)[%d]: pattern here over and over and over and over again I'd happily take it. Stefan? [...snip...] >> +ahci_reset(void *s) "ahci(%p): HBA reset" >> +allwinner_ahci_mem_read(void *s, void *a, uint64_t addr, uint64_t >> val, unsigned size) "ahci(%p): read a=%p addr=0x%"HWADDR_PRIx" >> val=0x%"PRIx64", size=%d" > > is AllwinnerAHCIState useful? > Only as far as it happens to be the argument to this function; of course AHCIState is the real prize, but we have to fish it out of AllwinnerAHCIState first. > I'd rather use directly trace_ahci_mem_read(s, size, addr, val) > >> +allwinner_ahci_mem_write(void *s, void *a, uint64_t addr, uint64_t >> val, unsigned size) "ahci(%p): write a=%p addr=0x%"HWADDR_PRIx" >> val=0x%"PRIx64", size=%d" > > and trace_ahci_mem_write(s, size, addr, val) > > Regards, > > Phil.
Re: [Qemu-block] [Qemu-devel] [PATCH 4/9] ATAPI: Replace DEBUG_IDE_ATAPI with tracing events
On 08/08/2017 04:59 PM, Eric Blake wrote:
> On 08/08/2017 01:33 PM, John Snow wrote:
>> Goodbye, printfs.
>> Hello, fancy printfs.
>>
>> Signed-off-by: John Snow
>> ---
>> hw/ide/atapi.c| 64
>> +--
>> hw/ide/trace-events | 19 ++
>> include/hw/ide/internal.h | 1 -
>> 3 files changed, 42 insertions(+), 42 deletions(-)
>
>> @@ -1330,16 +1310,18 @@ void ide_atapi_cmd(IDEState *s)
>> uint8_t *buf = s->io_buffer;
>> const struct AtapiCmd *cmd = &atapi_cmd_table[s->io_buffer[0]];
>>
>> -#ifdef DEBUG_IDE_ATAPI
>> -{
>> +trace_ide_atapi_cmd(s, s->io_buffer[0]);
>> +
>> +if (TRACE_IDE_ATAPI_CMD_PACKET_ENABLED) {
>> +/* Each pretty-printed byte needs two bytes and a space; */
>> +char *ppacket = g_malloc(ATAPI_PACKET_SIZE * 3 + 1);
>
> Nice use of an extra conditional to make a pretty trace without the
> overhead when tracing is off. (Oh yeah, you asked me on IRC how to do
> it, and I pointed to commit bd6952a3 as the example)
>
>
>> +++ b/hw/ide/trace-events
>> @@ -6,6 +6,10 @@ ide_ioport_read(uint32_t addr, const char *reg, uint32_t
>> val, void *bus, void *s
>> ide_ioport_write(uint32_t addr, const char *reg, uint32_t val, void *bus,
>> void *s) "IDE PIO wr @ 0x%"PRIx32" (%s); val 0x%02"PRIx32"; bus %p IDEState
>> %p"
>
> Wouldn't it be nice if our trace generator would let us line-wrap?
>
>> +# Warning: Verbose
>
> Cute. Should you also add it to the ide_data_* traces, per the commit
> message in 3/9?
>
Probably.
>> +ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet)
>> "IDEState: %p; limit=0x%x packet: %s"
>> \ No newline at end of file
>
> Umm, that should be fixed (I know that 'diff' uses both / and \ to
> differentiate whether it was the pre- or post-patch version that had the
> issue, but never remember which is which - so the fix may be in an
> earlier patch). Also fix the trace-events rebase for 3/9; with that,
> you can add
>
I'll fix these and let you re-bestow me with RBs, to err on the side of
caution.
> Reviewed-by: Eric Blake
>
Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] IDE: replace DEBUG_IDE with tracing system
On 08/25/2017 09:33 AM, Philippe Mathieu-Daudé wrote:
> Hi John,
>
> On 08/08/2017 03:32 PM, John Snow wrote:
>> Out with the old, in with the new.
>>
>> Signed-off-by: John Snow
>> ---
>> Makefile.objs | 1 +
>> hw/ide/cmd646.c | 10 +++-
>> hw/ide/core.c | 65
>> +++
>> hw/ide/pci.c | 17 -
>> hw/ide/piix.c | 11
>> hw/ide/trace-events | 33
>> hw/ide/via.c | 10 +++-
>> include/hw/ide/internal.h | 1 -
>> 8 files changed, 78 insertions(+), 70 deletions(-)
>> create mode 100644 hw/ide/trace-events
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 24a4ea0..967c092 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -153,6 +153,7 @@ trace-events-subdirs += hw/acpi
>> trace-events-subdirs += hw/arm
>> trace-events-subdirs += hw/alpha
>> trace-events-subdirs += hw/xen
>> +trace-events-subdirs += hw/ide
>> trace-events-subdirs += ui
>> trace-events-subdirs += audio
>> trace-events-subdirs += net
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index 9ebb8d4..86b2a8f 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -32,6 +32,7 @@
>> #include "sysemu/dma.h"
>> #include "hw/ide/pci.h"
>> +#include "trace.h"
>> /* CMD646 specific */
>> #define CFR0x50
>> @@ -195,9 +196,8 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr,
>> val = 0xff;
>> break;
>> }
>> -#ifdef DEBUG_IDE
>> -printf("bmdma: readb " TARGET_FMT_plx " : 0x%02x\n", addr, val);
>> -#endif
>> +
>> +trace_bmdma_read_cmd646(addr, val);
>> return val;
>> }
>> @@ -211,9 +211,7 @@ static void bmdma_write(void *opaque, hwaddr addr,
>> return;
>> }
>> -#ifdef DEBUG_IDE
>> -printf("bmdma: writeb " TARGET_FMT_plx " : 0x%" PRIx64 "\n",
>> addr, val);
>> -#endif
>> +trace_bmdma_write_cmd646(addr, val);
>> switch(addr & 3) {
>> case 0:
>> bmdma_cmd_writeb(bm, val);
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 0b48b64..53fa084 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -36,6 +36,7 @@
>> #include "qemu/cutils.h"
>> #include "hw/ide/internal.h"
>> +#include "trace.h"
>> /* These values were based on a Seagate ST3500418AS but have been
>> modified
>> to make more sense in QEMU */
>> @@ -656,10 +657,7 @@ void ide_cancel_dma_sync(IDEState *s)
>>* write requests) pending and we can avoid to drain. */
>> QLIST_FOREACH(req, &s->buffered_requests, list) {
>> if (!req->orphaned) {
>> -#ifdef DEBUG_IDE
>> -printf("%s: invoking cb %p of buffered request %p with"
>> - " -ECANCELED\n", __func__, req->original_cb, req);
>> -#endif
>> +trace_ide_cancel_dma_sync_buffered(req->original_cb, req);
>> req->original_cb(req->original_opaque, -ECANCELED);
>> }
>> req->orphaned = true;
>> @@ -678,9 +676,7 @@ void ide_cancel_dma_sync(IDEState *s)
>>* aio operation with preadv/pwritev.
>>*/
>> if (s->bus->dma->aiocb) {
>> -#ifdef DEBUG_IDE
>> -printf("%s: draining all remaining requests", __func__);
>> -#endif
>> +trace_ide_cancel_dma_sync_remaining();
>> blk_drain(s->blk);
>> assert(s->bus->dma->aiocb == NULL);
>> }
>> @@ -741,9 +737,7 @@ static void ide_sector_read(IDEState *s)
>> n = s->req_nb_sectors;
>> }
>> -#if defined(DEBUG_IDE)
>> -printf("sector=%" PRId64 "\n", sector_num);
>> -#endif
>> +trace_ide_sector_read(sector_num, n);
>> if (!ide_sect_range_ok(s, sector_num, n)) {
>> ide_rw_error(s);
>> @@ -1005,14 +999,14 @@ static void ide_sector_write(IDEState *s)
>> s->status = READY_STAT | SEEK_STAT | BUSY_STAT;
>> sector_num = ide_get_sector(s);
>> -#if defined(DEBUG_IDE)
>> -printf("sector=%" PRId64 "\n", sector_num);
>> -#endif
>> +
>> n = s->nsector;
>> if (n > s->req_nb_sectors) {
>> n = s->req_nb_sectors;
>> }
>> +trace_ide_sector_write(sector_num, n);
>> +
>> if (!ide_sect_range_ok(s, sector_num, n)) {
>> ide_rw_error(s);
>> block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_WRITE);
>> @@ -1186,18 +1180,17 @@ static void ide_clear_hob(IDEBus *bus)
>> void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>> {
>> IDEBus *bus = opaque;
>> +IDEState *s = idebus_active_if(bus);
>> +int reg_num = addr & 7;
>> -#ifdef DEBUG_IDE
>> -printf("IDE: write addr=0x%x val=0x%02x\n", addr, val);
>> -#endif
>> -
>> -addr &= 7;
>> +trace_ide_ioport_write(addr, val, bus, s);
>> /* ignore writes to command block while busy with previous
>> command */
>> -if (addr != 7 && (idebus_active_if(bus)->status &
>> (BUSY_STAT|DRQ_STAT)))
>> +if (reg_num != 7 && (s->status & (BU
Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable = false
On 08/23/2017 11:03 PM, Thomas Huth wrote:
> QEMU currently aborts with an assertion message when the user is trying
> to remove a dscm1 again:
>
> $ aarch64-softmmu/qemu-system-aarch64 -S -M integratorcp -nographic
> QEMU 2.9.93 monitor - type 'help' for more information
> (qemu) device_add dscm1,id=xyz
> (qemu) device_del xyz
> **
> ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> Aborted (core dumped)
>
> Looks like this device has to be wired up in code and is not meant
> to be hot-pluggable, so let's mark it with user_creatable = false.
>
> Signed-off-by: Thomas Huth
> ---
> hw/ide/microdrive.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
> index e3fd30e..17917c0 100644
> --- a/hw/ide/microdrive.c
> +++ b/hw/ide/microdrive.c
> @@ -575,12 +575,15 @@ PCMCIACardState *dscm1_init(DriveInfo *dinfo)
> static void dscm1_class_init(ObjectClass *oc, void *data)
> {
> PCMCIACardClass *pcc = PCMCIA_CARD_CLASS(oc);
> +DeviceClass *dc = DEVICE_CLASS(oc);
>
> pcc->cis = dscm1_cis;
> pcc->cis_len = sizeof(dscm1_cis);
>
> pcc->attach = dscm1_attach;
> pcc->detach = dscm1_detach;
> +/* Reason: Needs to be wired-up in code, see dscm1_init() */
> +dc->user_creatable = false;
> }
>
> static const TypeInfo dscm1_type_info = {
>
OK; I'll look into Paolo's suggestion as a further fix, but whether or
not we treat this property as a hint, it should be set anyway as you say.
Reviewed-by: John Snow
Thanks, applied to my IDE tree:
https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git
--js
Re: [Qemu-block] [Qemu-devel] [PATCH 4/5] pci: Add INTERFACE_LEGACY_PCI_DEVICE to legacy PCI devices
On 08/25/2017 03:39 PM, Eduardo Habkost wrote: > CCing maintainers of affected devices (sorry for not CCing you > before). > > On Wed, Aug 23, 2017 at 07:14:44PM -0300, Eduardo Habkost wrote: >> Add INTERFACE_LEGACY_PCI_DEVICE to all direct subtypes of >> TYPE_PCI_DEVICE, except: >> >> 1) The ones that already have INTERFACE_PCIE_DEVICE set: >> >> * base-xhci >> * e1000e >> * nvme >> * pvscsi >> * vfio-pci >> * virtio-pci >> * vmxnet3 >> >> 2) base-pci-bridge >> >> Not all PCI bridges are legacy PCI devices, so >> INTERFACE_LEGACY_PCI_DEVICE is added only to the subtypes that >> are actually legacy PCI devices: >> >> * dec-21154-p2p-bridge >> * i82801b11-bridge >> * pbm-bridge >> * pci-bridge >> >> The direct subtypes of base-pci-bridge not touched by this patch >> are: >> >> * xilinx-pcie-root: Already marked as PCIe-only device. >> * pcie-port: all non-abstract subtypes of pcie-port are already >> marked as PCIe-only devices. >> >> 3) megasas-base >> >> Not all megasas devices are legacy PCI devices, so the interface >> names are added to the subclasses registered by >> megasas_register_types(), according to information in the >> megasas_devices[] array. >> >> "megasas-gen2" already implements INTERFACE_PCIE_DEVICE, so add >> INTERFACE_LEGACY_PCI_DEVICE only to "megasas". >> >> Signed-off-by: Eduardo Habkost >> --- [...] >> hw/ide/ich.c| 4 >> hw/ide/pci.c| 4 Acked-by: John Snow (Random fly-by comment without looking at the other patches: I assume there are reasons it's not appropriate or good to add a legacy PCI device parent that we inherit from, and it's instead better to manually add the property to all children?)
Re: [Qemu-block] [Qemu-devel] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory
On 08/28/2017 12:34 PM, Igor Mammedov wrote:
> Fixes read after freeing error reported
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
>
> ich9-ahci device creates ide buses and attaches them as QOM children
> at realize time, however it forgets to properly clean them up
> at unrealize time and frees memory containing these children,
> with following call-chain:
>
>qdev_device_add()
> object_property_set_bool('realized', true)
>device_set_realized()
> ...
> pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
>...
>s->dev = g_new0(AHCIDevice, ports);
>...
> AHCIDevice *ad = &s->dev[i];
> ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1);
> ^^^ creates bus in memory allocated by above gnew()
> and adds it as child propety to ahci device
> ...
> hotplug_handler_plug(); -> goto post_realize_fail;
> pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
> ...
>g_free(s->dev);
>^^^ free memory that holds children busses
>
> return with error from device_set_realized()
>
> As result later when qdev_device_add() tries to unparent ich9-ahci
> after failed device_set_realized(),
> object_unparent() -> object_property_del_child()
> iterates over existing QOM children including buses added by
> ide_bus_new() and tries to unparent them, which causes access to
> freed memory where they where located.
>
> Reported-by: Thomas Huth
> Signed-off-by: Igor Mammedov
> ---
> hw/ide/ahci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 406a1b5..ccbe091 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
>
> ide_exit(s);
> }
> +object_unparent(OBJECT(&ad->port));
> }
>
> g_free(s->dev);
>
Nice, Thank you.
Reviewed-by: John Snow
Thanks, applied to my IDE tree:
https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git
--js
Re: [Qemu-block] [Qemu-devel] reduce write bandwidth of qcow2 driver while allocating new cluster
On 08/28/2017 01:56 AM, Liu Qing wrote: > Dear list, > Recently I used fio to test qcow2 driver in the guest os, and found out > that when a new cluster is allocated the 4K IO will occupy 64K(default cluster > size) bandwith. > From the code qcow2 driver will fill the unused part of new allocated > cluster with 0 in perform_cow. These 0s are set in qcow2_co_readv when the > read > destination is not allocated and it has no backing file. Could I forbidden any > further write in copy_sectors if the copy source is not allocated and it has > no backing file? So only the requested data is written to the cluster. > Function > copy_sectors is only used by perform_cow in the master branch. > Do you think this change is reasonable? Thanks. > > CCing qemu-block. If I am understanding you correctly, you're noticing an adverse performance impact from initial writes to unallocated clusters through the qcow2 driver, when qcow2 performs zero-fill of uninitiated clusters. First, can you look at sources for current master branch? it hasn't been named "copy_sectors" since June 2016, and the function looks a bit differently than it did in the version you're looking at, I think. John
Re: [Qemu-block] [Qemu-devel] Persistent bitmaps for non-qcow2 formats
On 08/27/2017 10:57 PM, Fam Zheng wrote: > On Fri, 08/25 15:44, Max Reitz wrote: >> Well, OK. The main argument against supporting anything but qcow2 is >> "if you want features, use qcow2; and we are working on making qcow2 as >> fast as possible." I think that's a very good argument still. At some >> point I (and probably others, too) had the idea of making qcow2 files in >> raw layout: > > > Yes! I think this idea makes a whole lot of sense, too. Metadata tables can be > generated so old implementation can still use it. > > Fam > >> Have the data as a blob, just like a raw file, padded by >> metadata around it. An autoclear flag would specify that the qcow2 file >> is in this format, and if so, you could simply access it like a raw file >> and should have exactly the same speed as a raw file. Maybe that would >> solve this whole issue, too? I wonder if this would be sufficient to alleviate the desire to use raw files... (Eh, well, realistically, someone's still always going to ask if they can use various features with non-qcow2 files...) Nir, Yaniv; any input? (Context: We're debating how to add persistent bitmaps to raw files as I was informed that RHV was 'asking about it.' Max is reminding me there is a proposal for a style of QCOW2 that uses a raw layout for data, mitigating or eliminating any performance hits related to the L2 cache. What I am not aware of is why RHV would use raw files for any purpose. Is it performance? Simplicity? Could RHV use a raw-layout qcow2?) --js
Re: [Qemu-block] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory
On Mon, Aug 28, 2017 at 06:34:45PM +0200, Igor Mammedov wrote:
> Fixes read after freeing error reported
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
>
> ich9-ahci device creates ide buses and attaches them as QOM children
> at realize time, however it forgets to properly clean them up
> at unrealize time and frees memory containing these children,
> with following call-chain:
>
>qdev_device_add()
> object_property_set_bool('realized', true)
>device_set_realized()
> ...
> pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
>...
>s->dev = g_new0(AHCIDevice, ports);
>...
> AHCIDevice *ad = &s->dev[i];
> ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1);
> ^^^ creates bus in memory allocated by above gnew()
> and adds it as child propety to ahci device
> ...
> hotplug_handler_plug(); -> goto post_realize_fail;
> pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
> ...
>g_free(s->dev);
>^^^ free memory that holds children busses
>
> return with error from device_set_realized()
>
> As result later when qdev_device_add() tries to unparent ich9-ahci
> after failed device_set_realized(),
> object_unparent() -> object_property_del_child()
> iterates over existing QOM children including buses added by
> ide_bus_new() and tries to unparent them, which causes access to
> freed memory where they where located.
>
> Reported-by: Thomas Huth
> Signed-off-by: Igor Mammedov
Reviewed-by: Michael S. Tsirkin
Pls merge through ide tree.
> ---
> hw/ide/ahci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 406a1b5..ccbe091 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
>
> ide_exit(s);
> }
> +object_unparent(OBJECT(&ad->port));
> }
>
> g_free(s->dev);
> --
> 2.7.4
Re: [Qemu-block] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory
On 28.08.2017 18:34, Igor Mammedov wrote:
> Fixes read after freeing error reported
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
>
> ich9-ahci device creates ide buses and attaches them as QOM children
> at realize time, however it forgets to properly clean them up
> at unrealize time and frees memory containing these children,
> with following call-chain:
>
>qdev_device_add()
> object_property_set_bool('realized', true)
>device_set_realized()
> ...
> pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
>...
>s->dev = g_new0(AHCIDevice, ports);
>...
> AHCIDevice *ad = &s->dev[i];
> ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1);
> ^^^ creates bus in memory allocated by above gnew()
> and adds it as child propety to ahci device
> ...
> hotplug_handler_plug(); -> goto post_realize_fail;
> pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
> ...
>g_free(s->dev);
>^^^ free memory that holds children busses
>
> return with error from device_set_realized()
>
> As result later when qdev_device_add() tries to unparent ich9-ahci
> after failed device_set_realized(),
> object_unparent() -> object_property_del_child()
> iterates over existing QOM children including buses added by
> ide_bus_new() and tries to unparent them, which causes access to
> freed memory where they where located.
>
> Reported-by: Thomas Huth
> Signed-off-by: Igor Mammedov
> ---
> hw/ide/ahci.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 406a1b5..ccbe091 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
>
> ide_exit(s);
> }
> +object_unparent(OBJECT(&ad->port));
> }
>
> g_free(s->dev);
>
Thanks, this fixes the problem for me with both, x86_64 and mips64el!
Tested-by: Thomas Huth
[Qemu-block] ALPSS: reminder and submission deadline
Just one more month to go until the Alpine Linux Persistence and Storage Summit (ALPSS) will be held from September 27-29 at the Lizumerhuette in Austria! If you want to submit a 30-minute talk please do so until Sep 1st, as we plan to finalize our schedule. BOFs and team meetings will be scheduled ad-hoc in the available meeting rooms or outside with a beautiful mountain panorama. If you only want to attend you can do so until last minute as long as space doesn't run. To submit a talk or request attendance please send a mail to: alpss...@lists.infradead.org More details are available on our website: http://www.alpss.at/ Thank you on behalf of the program committee: Stephen Bates Sagi Grimberg Christoph Hellwig Johannes Thumshirn Richard Weinberger
Re: [Qemu-block] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory
On 08/28/2017 01:34 PM, Igor Mammedov wrote:
Fixes read after freeing error reported
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
ich9-ahci device creates ide buses and attaches them as QOM children
at realize time, however it forgets to properly clean them up
at unrealize time and frees memory containing these children,
with following call-chain:
qdev_device_add()
object_property_set_bool('realized', true)
device_set_realized()
...
pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
...
s->dev = g_new0(AHCIDevice, ports);
...
AHCIDevice *ad = &s->dev[i];
ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1);
^^^ creates bus in memory allocated by above gnew()
and adds it as child propety to ahci device
...
hotplug_handler_plug(); -> goto post_realize_fail;
pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
...
g_free(s->dev);
^^^ free memory that holds children busses
return with error from device_set_realized()
As result later when qdev_device_add() tries to unparent ich9-ahci
after failed device_set_realized(),
object_unparent() -> object_property_del_child()
iterates over existing QOM children including buses added by
ide_bus_new() and tries to unparent them, which causes access to
freed memory where they where located.
Reported-by: Thomas Huth
Signed-off-by: Igor Mammedov
Reviewed-by: Philippe Mathieu-Daudé
---
hw/ide/ahci.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 406a1b5..ccbe091 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
ide_exit(s);
}
+object_unparent(OBJECT(&ad->port));
}
g_free(s->dev);
[Qemu-block] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory
Fixes read after freeing error reported
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
ich9-ahci device creates ide buses and attaches them as QOM children
at realize time, however it forgets to properly clean them up
at unrealize time and frees memory containing these children,
with following call-chain:
qdev_device_add()
object_property_set_bool('realized', true)
device_set_realized()
...
pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
...
s->dev = g_new0(AHCIDevice, ports);
...
AHCIDevice *ad = &s->dev[i];
ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1);
^^^ creates bus in memory allocated by above gnew()
and adds it as child propety to ahci device
...
hotplug_handler_plug(); -> goto post_realize_fail;
pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
...
g_free(s->dev);
^^^ free memory that holds children busses
return with error from device_set_realized()
As result later when qdev_device_add() tries to unparent ich9-ahci
after failed device_set_realized(),
object_unparent() -> object_property_del_child()
iterates over existing QOM children including buses added by
ide_bus_new() and tries to unparent them, which causes access to
freed memory where they where located.
Reported-by: Thomas Huth
Signed-off-by: Igor Mammedov
---
hw/ide/ahci.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 406a1b5..ccbe091 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
ide_exit(s);
}
+object_unparent(OBJECT(&ad->port));
}
g_free(s->dev);
--
2.7.4
Re: [Qemu-block] [Qemu-devel] reduce write bandwidth of qcow2 driver while allocating new cluster
[adding qemu-block] On 08/28/2017 12:56 AM, Liu Qing wrote: > Dear list, > Recently I used fio to test qcow2 driver in the guest os, and found out > that when a new cluster is allocated the 4K IO will occupy 64K(default cluster > size) bandwith. > From the code qcow2 driver will fill the unused part of new allocated > cluster with 0 in perform_cow. These 0s are set in qcow2_co_readv when the > read > destination is not allocated and it has no backing file. Could I forbidden any > further write in copy_sectors if the copy source is not allocated and it has > no backing file? So only the requested data is written to the cluster. > Function > copy_sectors is only used by perform_cow in the master branch. There have already been discussions on optimizing COW writes in a manner similar to what you are describing; for example, https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg00109.html -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] qemu-iotests: Extend non-shared storage migration test (194)
On Mon, Aug 28, 2017 at 09:51:43AM -0500, Eric Blake wrote:
> On 08/28/2017 06:29 AM, Kashyap Chamarthy wrote:
> > This is the follow-up patch that was discussed[*] as part of feedback to
> > qemu-iotest 194.
> >
> > Changes in this patch:
> >
> > - Supply 'job-id' parameter to `drive-mirror` invocation.
> >
> > - Issue `block-job-cancel` command on the source QEMU to gracefully
> > complete the mirroring operation.
> >
> > - Stop the NBD server on the destination QEMU.
> >
> > - Finally, exit once the event BLOCK_JOB_COMPLETED is emitted.
> >
> > With the above, the test will also be (almost) in sync with the
> > procedure outlined in the document live-block-operations.rst[+]
> > (section: "QMP invocation for live storage migration with
> > ``drive-mirror`` + NBD").
> >
> > [*] https://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg04820.html
> > -- qemu-iotests: add 194 non-shared storage migration test
> > [+]
> > https://git.qemu.org/gitweb.cgi?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst
> >
> > Signed-off-by: Kashyap Chamarthy
> > ---
> > I wonder:
> > - Is it worth printing the MIGRATION event state change?
>
> I think waiting for both the BLOCK_JOB_COMPLETED and MIGRATION events
> make sense (in other words, let's check both events in the expected
> order, rather than just one or the other).
That sounds more robust, will do in the next iteration.
> > - Since we're not checking on the MIGRATION event anymore, can
> > the migration state change events related code (that is triggerred
> > by setting 'migrate-set-capabilities') be simply removed?
>
> If we're going to mirror libvirt's non-shared storage migration
> sequence, I think we want to keep everything, rather than drop the
> migration half.
Yes, noted.
[...]
> > -iotests.log('Starting drive-mirror on source...')
> > +iotests.log('Starting `drive-mirror` on source...')
> > iotests.log(source_vm.qmp(
> >'drive-mirror',
> >device='drive0',
> >target='nbd+unix:///drive0?socket={0}'.format(nbd_sock_path),
> >sync='full',
> >format='raw', # always raw, the server handles the format
> > - mode='existing'))
> > + mode='existing',
> > + job_id='mirror-job0'))
> >
> > -iotests.log('Waiting for drive-mirror to complete...')
> > +iotests.log('Waiting for `drive-mirror` to complete...')
>
> So, up to here is okay,
>
> > iotests.log(source_vm.event_wait('BLOCK_JOB_READY'),
> > filters=[iotests.filter_qmp_event])
> >
> > @@ -66,8 +67,14 @@ dest_vm.qmp('migrate-set-capabilities',
> > capabilities=[{'capability': 'events', 'state': True}])
> > iotests.log(source_vm.qmp('migrate',
> > uri='unix:{0}'.format(migration_sock_path)))
> >
> > +iotests.log('Gracefully ending the `drive-mirror` job on source...')
> > +iotests.log(source_vm.qmp('block-job-cancel', device='mirror-job0'))
> > +
> > +iotests.log('Stopping the NBD server on destination...')
> > +iotests.log(dest_vm.qmp('nbd-server-stop'))
> > +
> > while True:
> > -event = source_vm.event_wait('MIGRATION')
> > +event = source_vm.event_wait('BLOCK_JOB_COMPLETED')
>
> And this event makes sense for catching the block-job-cancel, but I
> think you STILL want to keep a while loop for catching migration as well.
Yes, will do.
Thanks for the quick feedback.
[...]
--
/kashyap
Re: [Qemu-block] [PATCH] qemu-iotests: Extend non-shared storage migration test (194)
On 08/28/2017 06:29 AM, Kashyap Chamarthy wrote:
> This is the follow-up patch that was discussed[*] as part of feedback to
> qemu-iotest 194.
>
> Changes in this patch:
>
> - Supply 'job-id' parameter to `drive-mirror` invocation.
>
> - Issue `block-job-cancel` command on the source QEMU to gracefully
> complete the mirroring operation.
>
> - Stop the NBD server on the destination QEMU.
>
> - Finally, exit once the event BLOCK_JOB_COMPLETED is emitted.
>
> With the above, the test will also be (almost) in sync with the
> procedure outlined in the document live-block-operations.rst[+]
> (section: "QMP invocation for live storage migration with
> ``drive-mirror`` + NBD").
>
> [*] https://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg04820.html
> -- qemu-iotests: add 194 non-shared storage migration test
> [+]
> https://git.qemu.org/gitweb.cgi?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst
>
> Signed-off-by: Kashyap Chamarthy
> ---
> I wonder:
> - Is it worth printing the MIGRATION event state change?
I think waiting for both the BLOCK_JOB_COMPLETED and MIGRATION events
make sense (in other words, let's check both events in the expected
order, rather than just one or the other).
> - Since we're not checking on the MIGRATION event anymore, can
> the migration state change events related code (that is triggerred
> by setting 'migrate-set-capabilities') be simply removed?
If we're going to mirror libvirt's non-shared storage migration
sequence, I think we want to keep everything, rather than drop the
migration half.
> ---
> tests/qemu-iotests/194 | 17 -
> tests/qemu-iotests/194.out | 14 --
> 2 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
> index
> 8028111e21bed5cf4a2e8e32dc04aa5a9ea9caca..8d746be9d0033f478f11886ee93f95b0fa55bab0
> 100755
> --- a/tests/qemu-iotests/194
> +++ b/tests/qemu-iotests/194
> @@ -46,16 +46,17 @@ iotests.log('Launching NBD server on destination...')
> iotests.log(dest_vm.qmp('nbd-server-start', addr={'type': 'unix', 'data':
> {'path': nbd_sock_path}}))
> iotests.log(dest_vm.qmp('nbd-server-add', device='drive0', writable=True))
>
> -iotests.log('Starting drive-mirror on source...')
> +iotests.log('Starting `drive-mirror` on source...')
> iotests.log(source_vm.qmp(
>'drive-mirror',
>device='drive0',
>target='nbd+unix:///drive0?socket={0}'.format(nbd_sock_path),
>sync='full',
>format='raw', # always raw, the server handles the format
> - mode='existing'))
> + mode='existing',
> + job_id='mirror-job0'))
>
> -iotests.log('Waiting for drive-mirror to complete...')
> +iotests.log('Waiting for `drive-mirror` to complete...')
So, up to here is okay,
> iotests.log(source_vm.event_wait('BLOCK_JOB_READY'),
> filters=[iotests.filter_qmp_event])
>
> @@ -66,8 +67,14 @@ dest_vm.qmp('migrate-set-capabilities',
> capabilities=[{'capability': 'events', 'state': True}])
> iotests.log(source_vm.qmp('migrate',
> uri='unix:{0}'.format(migration_sock_path)))
>
> +iotests.log('Gracefully ending the `drive-mirror` job on source...')
> +iotests.log(source_vm.qmp('block-job-cancel', device='mirror-job0'))
> +
> +iotests.log('Stopping the NBD server on destination...')
> +iotests.log(dest_vm.qmp('nbd-server-stop'))
> +
> while True:
> -event = source_vm.event_wait('MIGRATION')
> +event = source_vm.event_wait('BLOCK_JOB_COMPLETED')
And this event makes sense for catching the block-job-cancel, but I
think you STILL want to keep a while loop for catching migration as well.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v3 5/7] block/throttle-groups.c: remove throttle-groups list
On Fri 25 Aug 2017 03:23:30 PM CEST, Manos Pitsidianakis wrote:
> @@ -1957,12 +1957,18 @@ void blk_io_limits_enable(BlockBackend *blk, const
> char *group, Error **errp)
> BlockDriverState *bs = blk_bs(blk), *throttle_node;
> QDict *options = qdict_new();
> Error *local_err = NULL;
> -ThrottleState *ts;
> -
> -bdrv_drained_begin(bs);
>
> /* Force creation of group in case it doesn't exist */
> -ts = throttle_group_incref(group);
> +if (!throttle_group_exists(group)) {
> +throttle_group_new_legacy(group, &local_err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +}
After this, the reference count of the new group is 2 (as is already
explained in throttle_group_new_legacy()).
> +bdrv_drained_begin(bs);
> +
> qdict_set_default_str(options, "file", bs->node_name);
> qdict_set_default_str(options, QEMU_OPT_THROTTLE_GROUP_NAME, group);
> throttle_node = bdrv_new_open_driver(bdrv_find_format("throttle"), NULL,
And the implicit throttle node will hold and extra reference, making it
3.
How do you delete the legacy throttle group then? block_set_io_throttle
with everything set to 0 disables I/O limits and destroys the node, but
the reference count is still 2 and the group is not destroyed.
> +static int query_throttle_groups_foreach(Object *obj, void *data)
> +{
> +ThrottleGroup *tg;
> +struct ThrottleGroupQuery *query = data;
> +
> +tg = (ThrottleGroup *)object_dynamic_cast(obj, TYPE_THROTTLE_GROUP);
> +if (!tg) {
> +return 0;
> +}
> +
> +if (!g_strcmp0(query->name, tg->name)) {
> +query->result = tg;
> +return 1;
> +}
> +
> +return 0;
> +}
If you want you can make this a bit shorter if you merge both ifs:
if (tg && !g_strcmp0(query->name, tg->name)) {
query->result = tg;
return 1;
}
return 0;
> +void throttle_group_new_legacy(const char *name, Error **errp)
> +{
> +ThrottleGroup *tg = NULL;
No need to initialize tg here.
> ThrottleState *throttle_group_incref(const char *name)
> {
I think that you can make this one static and remove it from
throttle-groups.h (no one else is using it). Same with
throttle_group_unref.
Berto
Re: [Qemu-block] [PATCH v3 4/7] block: remove legacy I/O throttling
On Fri 25 Aug 2017 03:23:29 PM CEST, Manos Pitsidianakis wrote: > This commit removes all I/O throttling from block/block-backend.c. In > order to support the existing interface, it is changed to use the > block/throttle.c filter driver. > > The throttle filter node that is created by the legacy interface is > stored in a 'throttle_node' field in the BlockBackendPublic of the > device. The legacy throttle node is managed by the legacy interface > completely. More advanced configurations with the filter drive are > possible using the QMP API, but these will be ignored by the legacy > interface. > > Signed-off-by: Manos Pitsidianakis Reviewed-by: Alberto Garcia Berto
Re: [Qemu-block] [PATCH v3 3/7] block: require job-id when device is a node name
On Fri 25 Aug 2017 03:23:28 PM CEST, Manos Pitsidianakis wrote: > With implicit filter nodes on the top of the graph it is not possible > to generate job-ids with the name of the device in block_job_create() > anymore, since the job's bs will not be a child_root. Reviewed-by: Alberto Garcia Berto
Re: [Qemu-block] [PATCH v3 1/7] block: skip implicit nodes in snapshots, blockjobs
On Fri 25 Aug 2017 03:23:26 PM CEST, Manos Pitsidianakis wrote:
> +static inline BlockDriverState *child_bs(BlockDriverState *bs)
> +{
> +BlockDriverState *backing = backing_bs(bs);
> +BlockDriverState *file = file_bs(bs);
> +assert(!(file && backing));
> +return backing ?: file;
> +}
I'm still not sure how useful it is to have a separate function for
this, instead of putting it inside bdrv_get_first_explicit(), but anyway
Reviewed-by: Alberto Garcia
Berto
[Qemu-block] [PATCH] qemu-iotests: Extend non-shared storage migration test (194)
This is the follow-up patch that was discussed[*] as part of feedback to
qemu-iotest 194.
Changes in this patch:
- Supply 'job-id' parameter to `drive-mirror` invocation.
- Issue `block-job-cancel` command on the source QEMU to gracefully
complete the mirroring operation.
- Stop the NBD server on the destination QEMU.
- Finally, exit once the event BLOCK_JOB_COMPLETED is emitted.
With the above, the test will also be (almost) in sync with the
procedure outlined in the document live-block-operations.rst[+]
(section: "QMP invocation for live storage migration with
``drive-mirror`` + NBD").
[*] https://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg04820.html
-- qemu-iotests: add 194 non-shared storage migration test
[+]
https://git.qemu.org/gitweb.cgi?p=qemu.git;a=blob;f=docs/interop/live-block-operations.rst
Signed-off-by: Kashyap Chamarthy
---
I wonder:
- Is it worth printing the MIGRATION event state change?
- Since we're not checking on the MIGRATION event anymore, can
the migration state change events related code (that is triggerred
by setting 'migrate-set-capabilities') be simply removed?
---
tests/qemu-iotests/194 | 17 -
tests/qemu-iotests/194.out | 14 --
2 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index
8028111e21bed5cf4a2e8e32dc04aa5a9ea9caca..8d746be9d0033f478f11886ee93f95b0fa55bab0
100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -46,16 +46,17 @@ iotests.log('Launching NBD server on destination...')
iotests.log(dest_vm.qmp('nbd-server-start', addr={'type': 'unix', 'data':
{'path': nbd_sock_path}}))
iotests.log(dest_vm.qmp('nbd-server-add', device='drive0', writable=True))
-iotests.log('Starting drive-mirror on source...')
+iotests.log('Starting `drive-mirror` on source...')
iotests.log(source_vm.qmp(
'drive-mirror',
device='drive0',
target='nbd+unix:///drive0?socket={0}'.format(nbd_sock_path),
sync='full',
format='raw', # always raw, the server handles the format
- mode='existing'))
+ mode='existing',
+ job_id='mirror-job0'))
-iotests.log('Waiting for drive-mirror to complete...')
+iotests.log('Waiting for `drive-mirror` to complete...')
iotests.log(source_vm.event_wait('BLOCK_JOB_READY'),
filters=[iotests.filter_qmp_event])
@@ -66,8 +67,14 @@ dest_vm.qmp('migrate-set-capabilities',
capabilities=[{'capability': 'events', 'state': True}])
iotests.log(source_vm.qmp('migrate',
uri='unix:{0}'.format(migration_sock_path)))
+iotests.log('Gracefully ending the `drive-mirror` job on source...')
+iotests.log(source_vm.qmp('block-job-cancel', device='mirror-job0'))
+
+iotests.log('Stopping the NBD server on destination...')
+iotests.log(dest_vm.qmp('nbd-server-stop'))
+
while True:
-event = source_vm.event_wait('MIGRATION')
+event = source_vm.event_wait('BLOCK_JOB_COMPLETED')
iotests.log(event, filters=[iotests.filter_qmp_event])
-if event['data']['status'] in ('completed', 'failed'):
+if event['event'] == 'BLOCK_JOB_COMPLETED':
break
diff --git a/tests/qemu-iotests/194.out b/tests/qemu-iotests/194.out
index
ae501fecacb706b1851cb9063ce9c9d5a28bb7ea..3a0e3a26e5342b0e3f0373623efd9d2b3ee8d2be
100644
--- a/tests/qemu-iotests/194.out
+++ b/tests/qemu-iotests/194.out
@@ -2,12 +2,14 @@ Launching VMs...
Launching NBD server on destination...
{u'return': {}}
{u'return': {}}
-Starting drive-mirror on source...
+Starting `drive-mirror` on source...
{u'return': {}}
-Waiting for drive-mirror to complete...
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'device': u'drive0', u'type': u'mirror', u'speed': 0, u'len': 1073741824,
u'offset': 1073741824}, u'event': u'BLOCK_JOB_READY'}
+Waiting for `drive-mirror` to complete...
+{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'device': u'mirror-job0', u'type': u'mirror', u'speed': 0, u'len':
1073741824, u'offset': 1073741824}, u'event': u'BLOCK_JOB_READY'}
Starting migration...
{u'return': {}}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'setup'}, u'event': u'MIGRATION'}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'active'}, u'event': u'MIGRATION'}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'status': u'completed'}, u'event': u'MIGRATION'}
+Gracefully ending the `drive-mirror` job on source...
+{u'return': {}}
+Stopping the NBD server on destination...
+{u'return': {}}
+{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data':
{u'device': u'mirror-job0', u'type': u'mirror', u'speed': 0, u'len':
1073741824, u'offset': 1073741824}, u'event': u'BLOCK_JOB_COMPLETED'}
--
2.9.5
Re: [Qemu-block] [PATCH v9 4/6] block: convert ThrottleGroup to object with QOM
On Fri 25 Aug 2017 03:20:26 PM CEST, Manos Pitsidianakis wrote: > ThrottleGroup is converted to an object. This will allow the future > throttle block filter drive easy creation and configuration of throttle > groups in QMP and cli. Reviewed-by: Alberto Garcia Berto
Re: [Qemu-block] [Qemu-devel] [PATCH 4/5] pci: Add INTERFACE_LEGACY_PCI_DEVICE to legacy PCI devices
On Fri 25 Aug 2017 09:39:22 PM CEST, Eduardo Habkost wrote:
> CCing maintainers of affected devices (sorry for not CCing you
> before).
>> diff --git a/hw/ipack/tpci200.c b/hw/ipack/tpci200.c
>> index 4dfa6b3..e380378 100644
>> --- a/hw/ipack/tpci200.c
>> +++ b/hw/ipack/tpci200.c
>> @@ -646,6 +646,10 @@ static const TypeInfo tpci200_info = {
>> .parent= TYPE_PCI_DEVICE,
>> .instance_size = sizeof(TPCI200State),
>> .class_init= tpci200_class_init,
>> +.interfaces = (InterfaceInfo[]) {
>> +{ INTERFACE_LEGACY_PCI_DEVICE },
>> +{ },
>> +},
>> };
Acked-by: Alberto Garcia
Berto
