| Message ID | 1441876643-7467-8-git-send-email-mst@redhat.com |
|---|---|
| State | Accepted |
| Commit | 0f288f854b96f56247e38f4207f71647133f0184 |
| Headers | show |
Hi, On 09/10/15 11:27, Michael S. Tsirkin wrote: > From: Laszlo Ersek <lersek@redhat.com> > > The current trace prototypes and (matching) trace calls lead to > "unorthodox" PCI BDF notation in at least the stderr trace backend. For > example, the four BARs of a QXL video card at 00:01.0 (bus 0, slot 1, > function 0) are traced like this (PID and timestamps removed): > > pci_update_mappings_add d=0x7f14a73bf890 00:00.1 0,0x84000000+0x4000000 > pci_update_mappings_add d=0x7f14a73bf890 00:00.1 1,0x80000000+0x4000000 > pci_update_mappings_add d=0x7f14a73bf890 00:00.1 2,0x88200000+0x2000 > pci_update_mappings_add d=0x7f14a73bf890 00:00.1 3,0xd060+0x20 > > The slot and function values are in reverse order. > > Stick with the conventional BDF notation. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Don Koch <dkoch@verizon.com> > Cc: qemu-trivial@nongnu.org > Fixes: 7828d75045 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> can you please add Marcel's R-b too? From <http://thread.gmane.org/gmane.comp.emulators.qemu/358561/focus=358673>. Also, the patch has been picked up by Michael Tokarev for qemu-trivial, <http://thread.gmane.org/gmane.comp.emulators.qemu/358561/focus=358673>. I apologize for the confusion, but I really couldn't figure out myself if this should go "only" to trivial, or to PCI "only". I guess the patch could be dropped from exactly one of the two queues, but I also expect that whenever the chronologically second one is going to be merged, git will simply realize that the patch is already present. So, my point is that please add *both* R-b tags, regardless of which tree this is going to be pulled from first. Also: any guidance for the next time I have a trivial patch for PCI? Should I Cc qemu-trivial, or not? Thanks! Laszlo > --- > hw/pci/pci.c | 4 ++-- > trace-events | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index eba7ca2..ccea628 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -1154,16 +1154,16 @@ static void pci_update_mappings(PCIDevice *d) > /* now do the real mapping */ > if (r->addr != PCI_BAR_UNMAPPED) { > trace_pci_update_mappings_del(d, pci_bus_num(d->bus), > - PCI_FUNC(d->devfn), > PCI_SLOT(d->devfn), > + PCI_FUNC(d->devfn), > i, r->addr, r->size); > memory_region_del_subregion(r->address_space, r->memory); > } > r->addr = new_addr; > if (r->addr != PCI_BAR_UNMAPPED) { > trace_pci_update_mappings_add(d, pci_bus_num(d->bus), > - PCI_FUNC(d->devfn), > PCI_SLOT(d->devfn), > + PCI_FUNC(d->devfn), > i, r->addr, r->size); > memory_region_add_subregion_overlap(r->address_space, > r->addr, r->memory, 1); > diff --git a/trace-events b/trace-events > index 0a82f0c..1684a95 100644 > --- a/trace-events > +++ b/trace-events > @@ -1303,8 +1303,8 @@ spapr_pci_lsi_set(const char *busname, int pin, uint32_t irq) "%s PIN%d IRQ %u" > spapr_pci_msi_retry(unsigned config_addr, unsigned req_num, unsigned max_irqs) "Guest device at %x asked %u, have only %u" > > # hw/pci/pci.c > -pci_update_mappings_del(void *d, uint32_t bus, uint32_t func, uint32_t slot, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 > -pci_update_mappings_add(void *d, uint32_t bus, uint32_t func, uint32_t slot, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 > +pci_update_mappings_del(void *d, uint32_t bus, uint32_t slot, uint32_t func, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 > +pci_update_mappings_add(void *d, uint32_t bus, uint32_t slot, uint32_t func, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 > > # hw/net/pcnet.c > pcnet_s_reset(void *s) "s=%p" >
On Thu, Sep 10, 2015 at 11:45:01AM +0200, Laszlo Ersek wrote: > Hi, > > On 09/10/15 11:27, Michael S. Tsirkin wrote: > > From: Laszlo Ersek <lersek@redhat.com> > > > > The current trace prototypes and (matching) trace calls lead to > > "unorthodox" PCI BDF notation in at least the stderr trace backend. For > > example, the four BARs of a QXL video card at 00:01.0 (bus 0, slot 1, > > function 0) are traced like this (PID and timestamps removed): > > > > pci_update_mappings_add d=0x7f14a73bf890 00:00.1 0,0x84000000+0x4000000 > > pci_update_mappings_add d=0x7f14a73bf890 00:00.1 1,0x80000000+0x4000000 > > pci_update_mappings_add d=0x7f14a73bf890 00:00.1 2,0x88200000+0x2000 > > pci_update_mappings_add d=0x7f14a73bf890 00:00.1 3,0xd060+0x20 > > > > The slot and function values are in reverse order. > > > > Stick with the conventional BDF notation. > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > Cc: Don Koch <dkoch@verizon.com> > > Cc: qemu-trivial@nongnu.org > > Fixes: 7828d75045 > > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > can you please add Marcel's R-b too? From > <http://thread.gmane.org/gmane.comp.emulators.qemu/358561/focus=358673>. > > Also, the patch has been picked up by Michael Tokarev for qemu-trivial, > <http://thread.gmane.org/gmane.comp.emulators.qemu/358561/focus=358673>. > I apologize for the confusion, but I really couldn't figure out myself > if this should go "only" to trivial, or to PCI "only". I guess the patch > could be dropped from exactly one of the two queues, but I also expect > that whenever the chronologically second one is going to be merged, git > will simply realize that the patch is already present. > > So, my point is that please add *both* R-b tags, regardless of which > tree this is going to be pulled from first. > > Also: any guidance for the next time I have a trivial patch for PCI? > Should I Cc qemu-trivial, or not? > > Thanks! > Laszlo I dropped it from the tag, thanks. I think you did everything right, but [trivial PATCH] confused my scripts. Better would be [PATCH <anything>]. For pci, best to Cc just me - I can handle trivial patches there with no problems. > > > --- > > hw/pci/pci.c | 4 ++-- > > trace-events | 4 ++-- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index eba7ca2..ccea628 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -1154,16 +1154,16 @@ static void pci_update_mappings(PCIDevice *d) > > /* now do the real mapping */ > > if (r->addr != PCI_BAR_UNMAPPED) { > > trace_pci_update_mappings_del(d, pci_bus_num(d->bus), > > - PCI_FUNC(d->devfn), > > PCI_SLOT(d->devfn), > > + PCI_FUNC(d->devfn), > > i, r->addr, r->size); > > memory_region_del_subregion(r->address_space, r->memory); > > } > > r->addr = new_addr; > > if (r->addr != PCI_BAR_UNMAPPED) { > > trace_pci_update_mappings_add(d, pci_bus_num(d->bus), > > - PCI_FUNC(d->devfn), > > PCI_SLOT(d->devfn), > > + PCI_FUNC(d->devfn), > > i, r->addr, r->size); > > memory_region_add_subregion_overlap(r->address_space, > > r->addr, r->memory, 1); > > diff --git a/trace-events b/trace-events > > index 0a82f0c..1684a95 100644 > > --- a/trace-events > > +++ b/trace-events > > @@ -1303,8 +1303,8 @@ spapr_pci_lsi_set(const char *busname, int pin, uint32_t irq) "%s PIN%d IRQ %u" > > spapr_pci_msi_retry(unsigned config_addr, unsigned req_num, unsigned max_irqs) "Guest device at %x asked %u, have only %u" > > > > # hw/pci/pci.c > > -pci_update_mappings_del(void *d, uint32_t bus, uint32_t func, uint32_t slot, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 > > -pci_update_mappings_add(void *d, uint32_t bus, uint32_t func, uint32_t slot, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 > > +pci_update_mappings_del(void *d, uint32_t bus, uint32_t slot, uint32_t func, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 > > +pci_update_mappings_add(void *d, uint32_t bus, uint32_t slot, uint32_t func, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 > > > > # hw/net/pcnet.c > > pcnet_s_reset(void *s) "s=%p" > > > > >
On 09/10/15 11:54, Michael S. Tsirkin wrote: > On Thu, Sep 10, 2015 at 11:45:01AM +0200, Laszlo Ersek wrote: >> Hi, >> >> On 09/10/15 11:27, Michael S. Tsirkin wrote: >>> From: Laszlo Ersek <lersek@redhat.com> >>> >>> The current trace prototypes and (matching) trace calls lead to >>> "unorthodox" PCI BDF notation in at least the stderr trace backend. For >>> example, the four BARs of a QXL video card at 00:01.0 (bus 0, slot 1, >>> function 0) are traced like this (PID and timestamps removed): >>> >>> pci_update_mappings_add d=0x7f14a73bf890 00:00.1 0,0x84000000+0x4000000 >>> pci_update_mappings_add d=0x7f14a73bf890 00:00.1 1,0x80000000+0x4000000 >>> pci_update_mappings_add d=0x7f14a73bf890 00:00.1 2,0x88200000+0x2000 >>> pci_update_mappings_add d=0x7f14a73bf890 00:00.1 3,0xd060+0x20 >>> >>> The slot and function values are in reverse order. >>> >>> Stick with the conventional BDF notation. >>> >>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>> Cc: Don Koch <dkoch@verizon.com> >>> Cc: qemu-trivial@nongnu.org >>> Fixes: 7828d75045 >>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> >> can you please add Marcel's R-b too? From >> <http://thread.gmane.org/gmane.comp.emulators.qemu/358561/focus=358673>. >> >> Also, the patch has been picked up by Michael Tokarev for qemu-trivial, >> <http://thread.gmane.org/gmane.comp.emulators.qemu/358561/focus=358673>. >> I apologize for the confusion, but I really couldn't figure out myself >> if this should go "only" to trivial, or to PCI "only". I guess the patch >> could be dropped from exactly one of the two queues, but I also expect >> that whenever the chronologically second one is going to be merged, git >> will simply realize that the patch is already present. >> >> So, my point is that please add *both* R-b tags, regardless of which >> tree this is going to be pulled from first. >> >> Also: any guidance for the next time I have a trivial patch for PCI? >> Should I Cc qemu-trivial, or not? >> >> Thanks! >> Laszlo > > I dropped it from the tag, thanks. > > I think you did everything right, but [trivial PATCH] confused > my scripts. Better would be [PATCH <anything>]. > > For pci, best to Cc just me - I can handle trivial patches there with no > problems. Thanks, I'll try to remember both of these points. Laszlo > > >> >>> --- >>> hw/pci/pci.c | 4 ++-- >>> trace-events | 4 ++-- >>> 2 files changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>> index eba7ca2..ccea628 100644 >>> --- a/hw/pci/pci.c >>> +++ b/hw/pci/pci.c >>> @@ -1154,16 +1154,16 @@ static void pci_update_mappings(PCIDevice *d) >>> /* now do the real mapping */ >>> if (r->addr != PCI_BAR_UNMAPPED) { >>> trace_pci_update_mappings_del(d, pci_bus_num(d->bus), >>> - PCI_FUNC(d->devfn), >>> PCI_SLOT(d->devfn), >>> + PCI_FUNC(d->devfn), >>> i, r->addr, r->size); >>> memory_region_del_subregion(r->address_space, r->memory); >>> } >>> r->addr = new_addr; >>> if (r->addr != PCI_BAR_UNMAPPED) { >>> trace_pci_update_mappings_add(d, pci_bus_num(d->bus), >>> - PCI_FUNC(d->devfn), >>> PCI_SLOT(d->devfn), >>> + PCI_FUNC(d->devfn), >>> i, r->addr, r->size); >>> memory_region_add_subregion_overlap(r->address_space, >>> r->addr, r->memory, 1); >>> diff --git a/trace-events b/trace-events >>> index 0a82f0c..1684a95 100644 >>> --- a/trace-events >>> +++ b/trace-events >>> @@ -1303,8 +1303,8 @@ spapr_pci_lsi_set(const char *busname, int pin, uint32_t irq) "%s PIN%d IRQ %u" >>> spapr_pci_msi_retry(unsigned config_addr, unsigned req_num, unsigned max_irqs) "Guest device at %x asked %u, have only %u" >>> >>> # hw/pci/pci.c >>> -pci_update_mappings_del(void *d, uint32_t bus, uint32_t func, uint32_t slot, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 >>> -pci_update_mappings_add(void *d, uint32_t bus, uint32_t func, uint32_t slot, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 >>> +pci_update_mappings_del(void *d, uint32_t bus, uint32_t slot, uint32_t func, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 >>> +pci_update_mappings_add(void *d, uint32_t bus, uint32_t slot, uint32_t func, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 >>> >>> # hw/net/pcnet.c >>> pcnet_s_reset(void *s) "s=%p" >>> >> >> >>
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index eba7ca2..ccea628 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -1154,16 +1154,16 @@ static void pci_update_mappings(PCIDevice *d) /* now do the real mapping */ if (r->addr != PCI_BAR_UNMAPPED) { trace_pci_update_mappings_del(d, pci_bus_num(d->bus), - PCI_FUNC(d->devfn), PCI_SLOT(d->devfn), + PCI_FUNC(d->devfn), i, r->addr, r->size); memory_region_del_subregion(r->address_space, r->memory); } r->addr = new_addr; if (r->addr != PCI_BAR_UNMAPPED) { trace_pci_update_mappings_add(d, pci_bus_num(d->bus), - PCI_FUNC(d->devfn), PCI_SLOT(d->devfn), + PCI_FUNC(d->devfn), i, r->addr, r->size); memory_region_add_subregion_overlap(r->address_space, r->addr, r->memory, 1); diff --git a/trace-events b/trace-events index 0a82f0c..1684a95 100644 --- a/trace-events +++ b/trace-events @@ -1303,8 +1303,8 @@ spapr_pci_lsi_set(const char *busname, int pin, uint32_t irq) "%s PIN%d IRQ %u" spapr_pci_msi_retry(unsigned config_addr, unsigned req_num, unsigned max_irqs) "Guest device at %x asked %u, have only %u" # hw/pci/pci.c -pci_update_mappings_del(void *d, uint32_t bus, uint32_t func, uint32_t slot, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 -pci_update_mappings_add(void *d, uint32_t bus, uint32_t func, uint32_t slot, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 +pci_update_mappings_del(void *d, uint32_t bus, uint32_t slot, uint32_t func, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 +pci_update_mappings_add(void *d, uint32_t bus, uint32_t slot, uint32_t func, int bar, uint64_t addr, uint64_t size) "d=%p %02x:%02x.%x %d,%#"PRIx64"+%#"PRIx64 # hw/net/pcnet.c pcnet_s_reset(void *s) "s=%p"