[Qemu-devel] [PATCH 0/6] pci: keep window properties up to date

[Qemu-devel] [PATCH 0/6] pci: keep window properties up to date

[Michael S. Tsirkin]

w32/w64 properties that we report in QOM are
currently static, but this is wrong:
guest firmware can select its own windows:
optimal placement for w64 is guest-dependent, further,
for Q35, w32 is affected by the MCFG base and size.

This detects the actual window configuration used by guest
and reports it in QOM.

Michael S. Tsirkin (6):
  q35: make pci window address/size match guest cfg
  range: add min/max operations on ranges
  pci: add helper to retrieve the 64-bit range
  range: add Range to typedefs
  q35: use 64 bit window programmed by guest
  piix: use 64 bit window programmed by guest

 include/hw/pci/pci.h    |  1 +
 include/qemu/range.h    | 19 ++++++++++++++++++-
 include/qemu/typedefs.h |  1 +
 hw/pci-host/piix.c      |  6 ++++++
 hw/pci-host/q35.c       | 16 ++++++++++++++++
 hw/pci/pci.c            | 43 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 85 insertions(+), 1 deletion(-)

-- 
MST

[Qemu-devel] [PATCH 1/6] q35: make pci window address/size match guest cfg

[Michael S. Tsirkin]

For Q35, MMCFG address and size are guest configurable.
Update w32 property to make it behave accordingly.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci-host/q35.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 4febd24..3f1d447 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch)
     }
     addr = pciexbar & addr_mask;
     pcie_host_mmcfg_update(pehb, enable, addr, length);
+    /* Leave enough space for the MCFG BAR */
+    /*
+     * TODO: this matches current bios behaviour, but it's not a power of two,
+     * which means an MTRR can't cover it exactly.
+     */
+    if (enable) {
+        mch->pci_info.w32.begin = addr + length;
+    } else {
+        mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
+    }
 }
 
 /* PAM */
-- 
MST

[Qemu-devel] [PATCH 2/6] range: add Range to typedefs

[Michael S. Tsirkin]

will help simplify header dependencies.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/qemu/range.h    | 2 +-
 include/qemu/typedefs.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/qemu/range.h b/include/qemu/range.h
index b76cc0d..4a0780d 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -2,6 +2,7 @@
 #define QEMU_RANGE_H
 
 #include <inttypes.h>
+#include <qemu/typedefs.h>
 
 /*
  * Operations on 64 bit address ranges.
@@ -15,7 +16,6 @@ struct Range {
     uint64_t begin; /* First byte of the range, or 0 if empty. */
     uint64_t end;   /* 1 + the last byte. 0 if range empty or ends at ~0x0LL. */
 };
-typedef struct Range Range;
 
 /* Get last byte of a range from offset + length.
  * Undefined for ranges that wrap around 0. */
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index ac9f8d4..70d250f 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -65,5 +65,6 @@ typedef struct QEMUSGList QEMUSGList;
 typedef struct SHPCDevice SHPCDevice;
 typedef struct FWCfgState FWCfgState;
 typedef struct PcGuestInfo PcGuestInfo;
+typedef struct Range Range;
 
 #endif /* QEMU_TYPEDEFS_H */
-- 
MST

[Qemu-devel] [PATCH 3/6] range: add min/max operations on ranges

[Michael S. Tsirkin]

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/qemu/range.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/qemu/range.h b/include/qemu/range.h
index 4a0780d..1c688ca 100644
--- a/include/qemu/range.h
+++ b/include/qemu/range.h
@@ -17,6 +17,23 @@ struct Range {
     uint64_t end;   /* 1 + the last byte. 0 if range empty or ends at ~0x0LL. */
 };
 
+static inline void range_extend(Range *range, Range *extend_by)
+{
+    if (!extend_by->begin && !extend_by->end) {
+        return;
+    }
+    if (!range->begin && !range->end) {
+        *range = *extend_by;
+        return;
+    }
+    if (range->begin > extend_by->begin) {
+        range->begin = extend_by->begin;
+    }
+    if (range->end - 1 < extend_by->end - 1) {
+        range->end = extend_by->end;
+    }
+}
+
 /* Get last byte of a range from offset + length.
  * Undefined for ranges that wrap around 0. */
 static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
-- 
MST

[Qemu-devel] [PATCH 4/6] pci: add helper to retrieve the 64-bit range

[Michael S. Tsirkin]

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/pci/pci.h |  1 +
 hw/pci/pci.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 2374aa9..7be93ae 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -397,6 +397,7 @@ const char *pci_root_bus_path(PCIDevice *dev);
 PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
 int pci_qdev_find_device(const char *id, PCIDevice **pdev);
 PCIBus *pci_get_bus_devfn(int *devfnp, PCIBus *root, const char *devaddr);
+void pci_bus_get_w64_range(PCIBus *bus, Range *range);
 
 int pci_parse_devaddr(const char *addr, int *domp, int *busp,
                       unsigned int *slotp, unsigned int *funcp);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8c33352..d9f9bdf 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2257,6 +2257,49 @@ void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
     bus->iommu_opaque = opaque;
 }
 
+static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
+{
+    Range *range = opaque;
+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
+    uint16_t cmd = pci_get_word(dev->config + PCI_COMMAND);
+    int r;
+
+    if (!(cmd & PCI_COMMAND_MEMORY)) {
+        return;
+    }
+
+    if (pc->is_bridge) {
+        pcibus_t base = pci_bridge_get_base(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
+        pcibus_t limit = pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
+
+        if (limit >= base) {
+            Range pref_range;
+            pref_range.begin = base;
+            pref_range.end = limit + 1;
+            range_extend(range, &pref_range);
+        }
+    }
+    for (r = 0; r < PCI_NUM_REGIONS; ++r) {
+        PCIIORegion *region = &dev->io_regions[r];
+        Range region_range;
+
+        if (!region->size ||
+            (region->type & PCI_BASE_ADDRESS_SPACE_IO) ||
+            !(region->type & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
+            continue;
+        }
+        region_range.begin = pci_get_quad(dev->config + pci_bar(dev, r));
+        region_range.end = region_range.begin + region->size;
+        range_extend(range, &region_range);
+    }
+}
+
+void pci_bus_get_w64_range(PCIBus *bus, Range *range)
+{
+    range->begin = range->end = 0;
+    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
+}
+
 static const TypeInfo pci_device_type_info = {
     .name = TYPE_PCI_DEVICE,
     .parent = TYPE_DEVICE,
-- 
MST

[Qemu-devel] [PATCH 5/6] q35: use 64 bit window programmed by guest

[Michael S. Tsirkin]

Detect the 64 bit window programmed by firmware
and configure properties accordingly.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci-host/q35.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 3f1d447..5cb1e8a 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -90,6 +90,9 @@ static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v,
                                           Error **errp)
 {
     Q35PCIHost *s = Q35_HOST_DEVICE(obj);
+    PCIHostState *h = PCI_HOST_BRIDGE(obj);
+
+    pci_bus_get_w64_range(h->bus, &s->mch.pci_info.w64);
 
     visit_type_uint64(v, &s->mch.pci_info.w64.begin, name, errp);
 }
@@ -99,6 +102,9 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
                                         Error **errp)
 {
     Q35PCIHost *s = Q35_HOST_DEVICE(obj);
+    PCIHostState *h = PCI_HOST_BRIDGE(obj);
+
+    pci_bus_get_w64_range(h->bus, &s->mch.pci_info.w64);
 
     visit_type_uint64(v, &s->mch.pci_info.w64.end, name, errp);
 }
-- 
MST

[Qemu-devel] [PATCH 6/6] piix: use 64 bit window programmed by guest

[Michael S. Tsirkin]

Detect the 64 bit window programmed by firmware
and configure properties accordingly.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci-host/piix.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 221d82b..9e7f82a 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -236,6 +236,9 @@ static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v,
                                                 Error **errp)
 {
     I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
+    PCIHostState *h = PCI_HOST_BRIDGE(obj);
+
+    pci_bus_get_w64_range(h->bus, &s->pci_info.w64);
 
     visit_type_uint64(v, &s->pci_info.w64.begin, name, errp);
 }
@@ -245,6 +248,9 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v,
                                               Error **errp)
 {
     I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj);
+    PCIHostState *h = PCI_HOST_BRIDGE(obj);
+
+    pci_bus_get_w64_range(h->bus, &s->pci_info.w64);
 
     visit_type_uint64(v, &s->pci_info.w64.end, name, errp);
 }
-- 
MST

Re: [Qemu-devel] [PATCH 3/6] range: add min/max operations on ranges

[Igor Mammedov]

On Wed, 4 Sep 2013 13:48:35 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/qemu/range.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/qemu/range.h b/include/qemu/range.h
> index 4a0780d..1c688ca 100644
> --- a/include/qemu/range.h
> +++ b/include/qemu/range.h
> @@ -17,6 +17,23 @@ struct Range {
>      uint64_t end;   /* 1 + the last byte. 0 if range empty or ends at ~0x0LL. */
>  };
>  
> +static inline void range_extend(Range *range, Range *extend_by)
doc comment what it does pls.

> +{
> +    if (!extend_by->begin && !extend_by->end) {
> +        return;
> +    }
> +    if (!range->begin && !range->end) {
> +        *range = *extend_by;
> +        return;
> +    }
> +    if (range->begin > extend_by->begin) {
> +        range->begin = extend_by->begin;
> +    }
> +    if (range->end - 1 < extend_by->end - 1) {
(foo)->end could be 0 at this point leading to overflow when subtracted,
is it intended to be so?

> +        range->end = extend_by->end;
> +    }
> +}
> +
>  /* Get last byte of a range from offset + length.
>   * Undefined for ranges that wrap around 0. */
>  static inline uint64_t range_get_last(uint64_t offset, uint64_t len)

Re: [Qemu-devel] [PATCH 3/6] range: add min/max operations on ranges

[Michael S. Tsirkin]

On Tue, Sep 10, 2013 at 11:35:54AM +0200, Igor Mammedov wrote:
> On Wed, 4 Sep 2013 13:48:35 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/qemu/range.h | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/include/qemu/range.h b/include/qemu/range.h
> > index 4a0780d..1c688ca 100644
> > --- a/include/qemu/range.h
> > +++ b/include/qemu/range.h
> > @@ -17,6 +17,23 @@ struct Range {
> >      uint64_t end;   /* 1 + the last byte. 0 if range empty or ends at ~0x0LL. */
> >  };
> >  
> > +static inline void range_extend(Range *range, Range *extend_by)
> doc comment what it does pls.
> 
> > +{
> > +    if (!extend_by->begin && !extend_by->end) {
> > +        return;
> > +    }
> > +    if (!range->begin && !range->end) {
> > +        *range = *extend_by;
> > +        return;
> > +    }
> > +    if (range->begin > extend_by->begin) {
> > +        range->begin = extend_by->begin;
> > +    }
> > +    if (range->end - 1 < extend_by->end - 1) {
> (foo)->end could be 0 at this point leading to overflow when subtracted,
> is it intended to be so?

Absolutely - as the comment near this field definition states:
0 means region ends at ~0x0LL.


> > +        range->end = extend_by->end;
> > +    }
> > +}
> > +
> >  /* Get last byte of a range from offset + length.
> >   * Undefined for ranges that wrap around 0. */
> >  static inline uint64_t range_get_last(uint64_t offset, uint64_t len)
> 

Re: [Qemu-devel] [PATCH 1/6] q35: make pci window address/size match guest cfg

[Igor Mammedov]

On Wed, 4 Sep 2013 13:48:29 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> For Q35, MMCFG address and size are guest configurable.
> Update w32 property to make it behave accordingly.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/pci-host/q35.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 4febd24..3f1d447 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch)
>      }
>      addr = pciexbar & addr_mask;
>      pcie_host_mmcfg_update(pehb, enable, addr, length);
> +    /* Leave enough space for the MCFG BAR */
> +    /*
> +     * TODO: this matches current bios behaviour, but it's not a power of two,
> +     * which means an MTRR can't cover it exactly.
> +     */
> +    if (enable) {
> +        mch->pci_info.w32.begin = addr + length;
> +    } else {
> +        mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
> +    }
>  }
I probably miss something but where is remapping in system address space?
If there is none then, then updated w32 might mismatch actually/initially mapped alias.

>  /* PAM */

Re: [Qemu-devel] [PATCH 4/6] pci: add helper to retrieve the 64-bit range

[Igor Mammedov]

On Wed, 4 Sep 2013 13:48:37 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/pci/pci.h |  1 +
>  hw/pci/pci.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 2374aa9..7be93ae 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -397,6 +397,7 @@ const char *pci_root_bus_path(PCIDevice *dev);
>  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
>  int pci_qdev_find_device(const char *id, PCIDevice **pdev);
>  PCIBus *pci_get_bus_devfn(int *devfnp, PCIBus *root, const char *devaddr);
> +void pci_bus_get_w64_range(PCIBus *bus, Range *range);
>  
>  int pci_parse_devaddr(const char *addr, int *domp, int *busp,
>                        unsigned int *slotp, unsigned int *funcp);
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 8c33352..d9f9bdf 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2257,6 +2257,49 @@ void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
>      bus->iommu_opaque = opaque;
>  }
>  
> +static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
> +{
> +    Range *range = opaque;
> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> +    uint16_t cmd = pci_get_word(dev->config + PCI_COMMAND);
> +    int r;
> +
> +    if (!(cmd & PCI_COMMAND_MEMORY)) {
> +        return;
> +    }
> +
> +    if (pc->is_bridge) {
> +        pcibus_t base = pci_bridge_get_base(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
what guaranties that 'base' won't be below 4Gb and be above '4Gb + above_4g_mem_size'?

> +        pcibus_t limit = pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
> +
> +        if (limit >= base) {
> +            Range pref_range;
> +            pref_range.begin = base;
> +            pref_range.end = limit + 1;
> +            range_extend(range, &pref_range);
> +        }
> +    }
> +    for (r = 0; r < PCI_NUM_REGIONS; ++r) {
> +        PCIIORegion *region = &dev->io_regions[r];
> +        Range region_range;
> +
> +        if (!region->size ||
> +            (region->type & PCI_BASE_ADDRESS_SPACE_IO) ||
> +            !(region->type & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
> +            continue;
> +        }
> +        region_range.begin = pci_get_quad(dev->config + pci_bar(dev, r));
> +        region_range.end = region_range.begin + region->size;
> +        range_extend(range, &region_range);
> +    }
> +}
> +
> +void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> +{
> +    range->begin = range->end = 0;
> +    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> +}
> +
>  static const TypeInfo pci_device_type_info = {
>      .name = TYPE_PCI_DEVICE,
>      .parent = TYPE_DEVICE,

Re: [Qemu-devel] [PATCH 1/6] q35: make pci window address/size match guest cfg

[Igor Mammedov]

On Tue, 10 Sep 2013 16:46:36 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote:
> > On Wed, 4 Sep 2013 13:48:29 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > For Q35, MMCFG address and size are guest configurable.
> > > Update w32 property to make it behave accordingly.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  hw/pci-host/q35.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > > index 4febd24..3f1d447 100644
> > > --- a/hw/pci-host/q35.c
> > > +++ b/hw/pci-host/q35.c
> > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch)
> > >      }
> > >      addr = pciexbar & addr_mask;
> > >      pcie_host_mmcfg_update(pehb, enable, addr, length);
> > > +    /* Leave enough space for the MCFG BAR */
> > > +    /*
> > > +     * TODO: this matches current bios behaviour, but it's not a power of two,
> > > +     * which means an MTRR can't cover it exactly.
> > > +     */
> > > +    if (enable) {
> > > +        mch->pci_info.w32.begin = addr + length;
> > > +    } else {
> > > +        mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
> > > +    }
> > >  }
> > I probably miss something but where is remapping in system address space?
> > If there is none then, then updated w32 might mismatch actually/initially mapped alias.
> > 
> > >  /* PAM */
> 
> You mean mmcfg?
> The re-mapping is in hw/pci/pcie_host.c
no, I mean 32-bit PCI hole

Re: [Qemu-devel] [PATCH 1/6] q35: make pci window address/size match guest cfg

[Michael S. Tsirkin]

On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote:
> On Wed, 4 Sep 2013 13:48:29 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > For Q35, MMCFG address and size are guest configurable.
> > Update w32 property to make it behave accordingly.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/pci-host/q35.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index 4febd24..3f1d447 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch)
> >      }
> >      addr = pciexbar & addr_mask;
> >      pcie_host_mmcfg_update(pehb, enable, addr, length);
> > +    /* Leave enough space for the MCFG BAR */
> > +    /*
> > +     * TODO: this matches current bios behaviour, but it's not a power of two,
> > +     * which means an MTRR can't cover it exactly.
> > +     */
> > +    if (enable) {
> > +        mch->pci_info.w32.begin = addr + length;
> > +    } else {
> > +        mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
> > +    }
> >  }
> I probably miss something but where is remapping in system address space?
> If there is none then, then updated w32 might mismatch actually/initially mapped alias.
> 
> >  /* PAM */

You mean mmcfg?
The re-mapping is in hw/pci/pcie_host.c

Re: [Qemu-devel] [PATCH 4/6] pci: add helper to retrieve the 64-bit range

[Michael S. Tsirkin]

On Tue, Sep 10, 2013 at 03:40:03PM +0200, Igor Mammedov wrote:
> On Wed, 4 Sep 2013 13:48:37 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  include/hw/pci/pci.h |  1 +
> >  hw/pci/pci.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 44 insertions(+)
> > 
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index 2374aa9..7be93ae 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -397,6 +397,7 @@ const char *pci_root_bus_path(PCIDevice *dev);
> >  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
> >  int pci_qdev_find_device(const char *id, PCIDevice **pdev);
> >  PCIBus *pci_get_bus_devfn(int *devfnp, PCIBus *root, const char *devaddr);
> > +void pci_bus_get_w64_range(PCIBus *bus, Range *range);
> >  
> >  int pci_parse_devaddr(const char *addr, int *domp, int *busp,
> >                        unsigned int *slotp, unsigned int *funcp);
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 8c33352..d9f9bdf 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -2257,6 +2257,49 @@ void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
> >      bus->iommu_opaque = opaque;
> >  }
> >  
> > +static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
> > +{
> > +    Range *range = opaque;
> > +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> > +    uint16_t cmd = pci_get_word(dev->config + PCI_COMMAND);
> > +    int r;
> > +
> > +    if (!(cmd & PCI_COMMAND_MEMORY)) {
> > +        return;
> > +    }
> > +
> > +    if (pc->is_bridge) {
> > +        pcibus_t base = pci_bridge_get_base(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
> what guaranties that 'base' won't be below 4Gb

Hmm, this needs some thought.

> and be above '4Gb + above_4g_mem_size'?

This one is harmless - guest will only hurt itself if
it misconfigures the bridge like this.

> > +        pcibus_t limit = pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
> > +
> > +        if (limit >= base) {
> > +            Range pref_range;
> > +            pref_range.begin = base;
> > +            pref_range.end = limit + 1;
> > +            range_extend(range, &pref_range);
> > +        }
> > +    }
> > +    for (r = 0; r < PCI_NUM_REGIONS; ++r) {
> > +        PCIIORegion *region = &dev->io_regions[r];
> > +        Range region_range;
> > +
> > +        if (!region->size ||
> > +            (region->type & PCI_BASE_ADDRESS_SPACE_IO) ||
> > +            !(region->type & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
> > +            continue;
> > +        }
> > +        region_range.begin = pci_get_quad(dev->config + pci_bar(dev, r));
> > +        region_range.end = region_range.begin + region->size;
> > +        range_extend(range, &region_range);
> > +    }
> > +}
> > +
> > +void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> > +{
> > +    range->begin = range->end = 0;
> > +    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> > +}
> > +
> >  static const TypeInfo pci_device_type_info = {
> >      .name = TYPE_PCI_DEVICE,
> >      .parent = TYPE_DEVICE,

Re: [Qemu-devel] [PATCH 1/6] q35: make pci window address/size match guest cfg

[Michael S. Tsirkin]

On Tue, Sep 10, 2013 at 03:46:13PM +0200, Igor Mammedov wrote:
> On Tue, 10 Sep 2013 16:46:36 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote:
> > > On Wed, 4 Sep 2013 13:48:29 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > For Q35, MMCFG address and size are guest configurable.
> > > > Update w32 property to make it behave accordingly.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  hw/pci-host/q35.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > > > index 4febd24..3f1d447 100644
> > > > --- a/hw/pci-host/q35.c
> > > > +++ b/hw/pci-host/q35.c
> > > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch)
> > > >      }
> > > >      addr = pciexbar & addr_mask;
> > > >      pcie_host_mmcfg_update(pehb, enable, addr, length);
> > > > +    /* Leave enough space for the MCFG BAR */
> > > > +    /*
> > > > +     * TODO: this matches current bios behaviour, but it's not a power of two,
> > > > +     * which means an MTRR can't cover it exactly.
> > > > +     */
> > > > +    if (enable) {
> > > > +        mch->pci_info.w32.begin = addr + length;
> > > > +    } else {
> > > > +        mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
> > > > +    }
> > > >  }
> > > I probably miss something but where is remapping in system address space?
> > > If there is none then, then updated w32 might mismatch actually/initially mapped alias.
> > > 
> > > >  /* PAM */
> > 
> > You mean mmcfg?
> > The re-mapping is in hw/pci/pcie_host.c
> no, I mean 32-bit PCI hole

It works differently - see mch_init.
32-bit PCI hole is not remapped - instead mmcfg overlaps it
and shadows a chunk of it.

Re: [Qemu-devel] [PATCH 1/6] q35: make pci window address/size match guest cfg

[Igor Mammedov]

On Tue, 10 Sep 2013 16:53:58 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Sep 10, 2013 at 03:46:13PM +0200, Igor Mammedov wrote:
> > On Tue, 10 Sep 2013 16:46:36 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote:
> > > > On Wed, 4 Sep 2013 13:48:29 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > For Q35, MMCFG address and size are guest configurable.
> > > > > Update w32 property to make it behave accordingly.
> > > > >
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >  hw/pci-host/q35.c | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > > 
> > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > > > > index 4febd24..3f1d447 100644
> > > > > --- a/hw/pci-host/q35.c
> > > > > +++ b/hw/pci-host/q35.c
> > > > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch)
> > > > >      }
> > > > >      addr = pciexbar & addr_mask;
> > > > >      pcie_host_mmcfg_update(pehb, enable, addr, length);
> > > > > +    /* Leave enough space for the MCFG BAR */
> > > > > +    /*
> > > > > +     * TODO: this matches current bios behaviour, but it's not a power of two,
> > > > > +     * which means an MTRR can't cover it exactly.
> > > > > +     */
> > > > > +    if (enable) {
> > > > > +        mch->pci_info.w32.begin = addr + length;
> > > > > +    } else {
> > > > > +        mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
> > > > > +    }
> > > > >  }
> > > > I probably miss something but where is remapping in system address space?
> > > > If there is none then, then updated w32 might mismatch actually/initially mapped alias.
> > > > 
> > > > >  /* PAM */
> > > 
> > > You mean mmcfg?
> > > The re-mapping is in hw/pci/pcie_host.c
> > no, I mean 32-bit PCI hole
> 
> It works differently - see mch_init.
> 32-bit PCI hole is not remapped - instead mmcfg overlaps it
> and shadows a chunk of it.
> 
it probably won't cause harm, but actual pci_hole alias doesn't match
its window reported via properties anymore (it's just inconsistent).

Re: [Qemu-devel] [PATCH 5/6] q35: use 64 bit window programmed by guest

[Igor Mammedov]

On Wed, 4 Sep 2013 13:48:40 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Detect the 64 bit window programmed by firmware
> and configure properties accordingly.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/pci-host/q35.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 3f1d447..5cb1e8a 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -90,6 +90,9 @@ static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v,
>                                            Error **errp)
>  {
>      Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> +    PCIHostState *h = PCI_HOST_BRIDGE(obj);
> +
> +    pci_bus_get_w64_range(h->bus, &s->mch.pci_info.w64);
Shouldn't be it done at the time when BIOS initializes BAR, to avoid
inconsistent state in between write and read.

Also missing remapping of existing 64-bit PCI hole alias to a new range.

>  
>      visit_type_uint64(v, &s->mch.pci_info.w64.begin, name, errp);
>  }
> @@ -99,6 +102,9 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
>                                          Error **errp)
>  {
>      Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> +    PCIHostState *h = PCI_HOST_BRIDGE(obj);
> +
> +    pci_bus_get_w64_range(h->bus, &s->mch.pci_info.w64);
ditto

>  
>      visit_type_uint64(v, &s->mch.pci_info.w64.end, name, errp);
>  }

Re: [Qemu-devel] [PATCH 1/6] q35: make pci window address/size match guest cfg

[Michael S. Tsirkin]

On Tue, Sep 10, 2013 at 04:09:21PM +0200, Igor Mammedov wrote:
> On Tue, 10 Sep 2013 16:53:58 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Sep 10, 2013 at 03:46:13PM +0200, Igor Mammedov wrote:
> > > On Tue, 10 Sep 2013 16:46:36 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote:
> > > > > On Wed, 4 Sep 2013 13:48:29 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > For Q35, MMCFG address and size are guest configurable.
> > > > > > Update w32 property to make it behave accordingly.
> > > > > >
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > ---
> > > > > >  hw/pci-host/q35.c | 10 ++++++++++
> > > > > >  1 file changed, 10 insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > > > > > index 4febd24..3f1d447 100644
> > > > > > --- a/hw/pci-host/q35.c
> > > > > > +++ b/hw/pci-host/q35.c
> > > > > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch)
> > > > > >      }
> > > > > >      addr = pciexbar & addr_mask;
> > > > > >      pcie_host_mmcfg_update(pehb, enable, addr, length);
> > > > > > +    /* Leave enough space for the MCFG BAR */
> > > > > > +    /*
> > > > > > +     * TODO: this matches current bios behaviour, but it's not a power of two,
> > > > > > +     * which means an MTRR can't cover it exactly.
> > > > > > +     */
> > > > > > +    if (enable) {
> > > > > > +        mch->pci_info.w32.begin = addr + length;
> > > > > > +    } else {
> > > > > > +        mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
> > > > > > +    }
> > > > > >  }
> > > > > I probably miss something but where is remapping in system address space?
> > > > > If there is none then, then updated w32 might mismatch actually/initially mapped alias.
> > > > > 
> > > > > >  /* PAM */
> > > > 
> > > > You mean mmcfg?
> > > > The re-mapping is in hw/pci/pcie_host.c
> > > no, I mean 32-bit PCI hole
> > 
> > It works differently - see mch_init.
> > 32-bit PCI hole is not remapped - instead mmcfg overlaps it
> > and shadows a chunk of it.
> > 
> it probably won't cause harm, but actual pci_hole alias doesn't match
> its window reported via properties anymore (it's just inconsistent).

That's exactly what happens at the moment too:
for q35 pci hole has a chunk that is not reported in w32.

Do you imply that w32 should just report the PCI hole,
and make users (e.g. acpi) take mmcfg out of that?


-- 
MST

Re: [Qemu-devel] [PATCH 5/6] q35: use 64 bit window programmed by guest

[Michael S. Tsirkin]

On Tue, Sep 10, 2013 at 04:12:09PM +0200, Igor Mammedov wrote:
> On Wed, 4 Sep 2013 13:48:40 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Detect the 64 bit window programmed by firmware
> > and configure properties accordingly.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/pci-host/q35.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index 3f1d447..5cb1e8a 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -90,6 +90,9 @@ static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v,
> >                                            Error **errp)
> >  {
> >      Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> > +    PCIHostState *h = PCI_HOST_BRIDGE(obj);
> > +
> > +    pci_bus_get_w64_range(h->bus, &s->mch.pci_info.w64);
> Shouldn't be it done at the time when BIOS initializes BAR, to avoid
> inconsistent state in between write and read.

What's inconsistent with what?
We just report the current state.
If you query it during initialization, you get what
has been programmed so far.

> Also missing remapping of existing 64-bit PCI hole alias to a new range.

We shouldn't remap the region - this is not how hardware works.
This property merely reports what BIOS decided to use.


> >  
> >      visit_type_uint64(v, &s->mch.pci_info.w64.begin, name, errp);
> >  }
> > @@ -99,6 +102,9 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
> >                                          Error **errp)
> >  {
> >      Q35PCIHost *s = Q35_HOST_DEVICE(obj);
> > +    PCIHostState *h = PCI_HOST_BRIDGE(obj);
> > +
> > +    pci_bus_get_w64_range(h->bus, &s->mch.pci_info.w64);
> ditto
> 
> >  
> >      visit_type_uint64(v, &s->mch.pci_info.w64.end, name, errp);
> >  }

Re: [Qemu-devel] [PATCH 1/6] q35: make pci window address/size match guest cfg

[Igor Mammedov]

On Tue, 10 Sep 2013 17:16:24 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Sep 10, 2013 at 04:09:21PM +0200, Igor Mammedov wrote:
> > On Tue, 10 Sep 2013 16:53:58 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Tue, Sep 10, 2013 at 03:46:13PM +0200, Igor Mammedov wrote:
> > > > On Tue, 10 Sep 2013 16:46:36 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote:
> > > > > > On Wed, 4 Sep 2013 13:48:29 +0300
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > 
> > > > > > > For Q35, MMCFG address and size are guest configurable.
> > > > > > > Update w32 property to make it behave accordingly.
> > > > > > >
> > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > ---
> > > > > > >  hw/pci-host/q35.c | 10 ++++++++++
> > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > > > > > > index 4febd24..3f1d447 100644
> > > > > > > --- a/hw/pci-host/q35.c
> > > > > > > +++ b/hw/pci-host/q35.c
> > > > > > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch)
> > > > > > >      }
> > > > > > >      addr = pciexbar & addr_mask;
> > > > > > >      pcie_host_mmcfg_update(pehb, enable, addr, length);
> > > > > > > +    /* Leave enough space for the MCFG BAR */
> > > > > > > +    /*
> > > > > > > +     * TODO: this matches current bios behaviour, but it's not a power of two,
> > > > > > > +     * which means an MTRR can't cover it exactly.
> > > > > > > +     */
> > > > > > > +    if (enable) {
> > > > > > > +        mch->pci_info.w32.begin = addr + length;
> > > > > > > +    } else {
> > > > > > > +        mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
> > > > > > > +    }
> > > > > > >  }
> > > > > > I probably miss something but where is remapping in system address space?
> > > > > > If there is none then, then updated w32 might mismatch actually/initially mapped alias.
> > > > > > 
> > > > > > >  /* PAM */
> > > > > 
> > > > > You mean mmcfg?
> > > > > The re-mapping is in hw/pci/pcie_host.c
> > > > no, I mean 32-bit PCI hole
> > > 
> > > It works differently - see mch_init.
> > > 32-bit PCI hole is not remapped - instead mmcfg overlaps it
> > > and shadows a chunk of it.
> > > 
> > it probably won't cause harm, but actual pci_hole alias doesn't match
> > its window reported via properties anymore (it's just inconsistent).
> 
> That's exactly what happens at the moment too:
> for q35 pci hole has a chunk that is not reported in w32.
> 
> Do you imply that w32 should just report the PCI hole,
> and make users (e.g. acpi) take mmcfg out of that?
initial pci_hole alias in system AS doesn't include mmcfg, it's mapped right after mmcfg and
w32 range matches alias exactly. But after mmcfg update that might be not true. I suggest to
move/remap alias to match new w32, to be consistent.

> 
> 

Re: [Qemu-devel] [PATCH 1/6] q35: make pci window address/size match guest cfg

[Michael S. Tsirkin]

On Tue, Sep 10, 2013 at 05:05:43PM +0200, Igor Mammedov wrote:
> On Tue, 10 Sep 2013 17:16:24 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Sep 10, 2013 at 04:09:21PM +0200, Igor Mammedov wrote:
> > > On Tue, 10 Sep 2013 16:53:58 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Tue, Sep 10, 2013 at 03:46:13PM +0200, Igor Mammedov wrote:
> > > > > On Tue, 10 Sep 2013 16:46:36 +0300
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > On Tue, Sep 10, 2013 at 03:37:12PM +0200, Igor Mammedov wrote:
> > > > > > > On Wed, 4 Sep 2013 13:48:29 +0300
> > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > 
> > > > > > > > For Q35, MMCFG address and size are guest configurable.
> > > > > > > > Update w32 property to make it behave accordingly.
> > > > > > > >
> > > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > > ---
> > > > > > > >  hw/pci-host/q35.c | 10 ++++++++++
> > > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > > > > > > > index 4febd24..3f1d447 100644
> > > > > > > > --- a/hw/pci-host/q35.c
> > > > > > > > +++ b/hw/pci-host/q35.c
> > > > > > > > @@ -214,6 +214,16 @@ static void mch_update_pciexbar(MCHPCIState *mch)
> > > > > > > >      }
> > > > > > > >      addr = pciexbar & addr_mask;
> > > > > > > >      pcie_host_mmcfg_update(pehb, enable, addr, length);
> > > > > > > > +    /* Leave enough space for the MCFG BAR */
> > > > > > > > +    /*
> > > > > > > > +     * TODO: this matches current bios behaviour, but it's not a power of two,
> > > > > > > > +     * which means an MTRR can't cover it exactly.
> > > > > > > > +     */
> > > > > > > > +    if (enable) {
> > > > > > > > +        mch->pci_info.w32.begin = addr + length;
> > > > > > > > +    } else {
> > > > > > > > +        mch->pci_info.w32.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT;
> > > > > > > > +    }
> > > > > > > >  }
> > > > > > > I probably miss something but where is remapping in system address space?
> > > > > > > If there is none then, then updated w32 might mismatch actually/initially mapped alias.
> > > > > > > 
> > > > > > > >  /* PAM */
> > > > > > 
> > > > > > You mean mmcfg?
> > > > > > The re-mapping is in hw/pci/pcie_host.c
> > > > > no, I mean 32-bit PCI hole
> > > > 
> > > > It works differently - see mch_init.
> > > > 32-bit PCI hole is not remapped - instead mmcfg overlaps it
> > > > and shadows a chunk of it.
> > > > 
> > > it probably won't cause harm, but actual pci_hole alias doesn't match
> > > its window reported via properties anymore (it's just inconsistent).
> > 
> > That's exactly what happens at the moment too:
> > for q35 pci hole has a chunk that is not reported in w32.
> > 
> > Do you imply that w32 should just report the PCI hole,
> > and make users (e.g. acpi) take mmcfg out of that?

> initial pci_hole alias in system AS doesn't include mmcfg, it's mapped right after mmcfg and
> w32 range matches alias exactly.

That would be a bug (unrelated to this patch series).
But I don't see this code: looking at mch_init,
    memory_region_add_subregion(mch->system_memory, mch->below_4g_mem_size,
                                &mch->pci_hole);

so we have pci hole right after memory, and overlapping mmcfg,
which makes sense to me.

> But after mmcfg update that might be not true. I suggest to
> move/remap alias to match new w32, to be consistent.



> > 
> > 

Re: [Qemu-devel] [PATCH 4/6] pci: add helper to retrieve the 64-bit range

[Igor Mammedov]

On Tue, 10 Sep 2013 16:50:41 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Sep 10, 2013 at 03:40:03PM +0200, Igor Mammedov wrote:
> > On Wed, 4 Sep 2013 13:48:37 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  include/hw/pci/pci.h |  1 +
> > >  hw/pci/pci.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 44 insertions(+)
> > > 
> > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > index 2374aa9..7be93ae 100644
> > > --- a/include/hw/pci/pci.h
> > > +++ b/include/hw/pci/pci.h
> > > @@ -397,6 +397,7 @@ const char *pci_root_bus_path(PCIDevice *dev);
> > >  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
> > >  int pci_qdev_find_device(const char *id, PCIDevice **pdev);
> > >  PCIBus *pci_get_bus_devfn(int *devfnp, PCIBus *root, const char *devaddr);
> > > +void pci_bus_get_w64_range(PCIBus *bus, Range *range);
> > >  
> > >  int pci_parse_devaddr(const char *addr, int *domp, int *busp,
> > >                        unsigned int *slotp, unsigned int *funcp);
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 8c33352..d9f9bdf 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -2257,6 +2257,49 @@ void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
> > >      bus->iommu_opaque = opaque;
> > >  }
> > >  
> > > +static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
> > > +{
> > > +    Range *range = opaque;
> > > +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> > > +    uint16_t cmd = pci_get_word(dev->config + PCI_COMMAND);
> > > +    int r;
> > > +
> > > +    if (!(cmd & PCI_COMMAND_MEMORY)) {
> > > +        return;
> > > +    }
> > > +
> > > +    if (pc->is_bridge) {
> > > +        pcibus_t base = pci_bridge_get_base(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
> > what guaranties that 'base' won't be below 4Gb
> 
> Hmm, this needs some thought.
> 
> > and be above '4Gb + above_4g_mem_size'?
> 
> This one is harmless - guest will only hurt itself if
> it misconfigures the bridge like this.

start of w64 is hardcoded in QEMU and Seabios as (4Gb+above_4gb_ram_size),
so maybe there is no need to update/modify it at all?


> 
> > > +        pcibus_t limit = pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
> > > +
> > > +        if (limit >= base) {
> > > +            Range pref_range;
> > > +            pref_range.begin = base;
> > > +            pref_range.end = limit + 1;
> > > +            range_extend(range, &pref_range);
> > > +        }
> > > +    }
> > > +    for (r = 0; r < PCI_NUM_REGIONS; ++r) {
> > > +        PCIIORegion *region = &dev->io_regions[r];
> > > +        Range region_range;
> > > +
> > > +        if (!region->size ||
> > > +            (region->type & PCI_BASE_ADDRESS_SPACE_IO) ||
> > > +            !(region->type & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
> > > +            continue;
> > > +        }
> > > +        region_range.begin = pci_get_quad(dev->config + pci_bar(dev, r));
> > > +        region_range.end = region_range.begin + region->size;
> > > +        range_extend(range, &region_range);
> > > +    }
> > > +}
> > > +
> > > +void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> > > +{
> > > +    range->begin = range->end = 0;
> > > +    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> > > +}
> > > +
> > >  static const TypeInfo pci_device_type_info = {
> > >      .name = TYPE_PCI_DEVICE,
> > >      .parent = TYPE_DEVICE,

Re: [Qemu-devel] [PATCH 4/6] pci: add helper to retrieve the 64-bit range

[Michael S. Tsirkin]

On Tue, Sep 10, 2013 at 07:05:10PM +0200, Igor Mammedov wrote:
> On Tue, 10 Sep 2013 16:50:41 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Sep 10, 2013 at 03:40:03PM +0200, Igor Mammedov wrote:
> > > On Wed, 4 Sep 2013 13:48:37 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  include/hw/pci/pci.h |  1 +
> > > >  hw/pci/pci.c         | 43 +++++++++++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 44 insertions(+)
> > > > 
> > > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > > index 2374aa9..7be93ae 100644
> > > > --- a/include/hw/pci/pci.h
> > > > +++ b/include/hw/pci/pci.h
> > > > @@ -397,6 +397,7 @@ const char *pci_root_bus_path(PCIDevice *dev);
> > > >  PCIDevice *pci_find_device(PCIBus *bus, int bus_num, uint8_t devfn);
> > > >  int pci_qdev_find_device(const char *id, PCIDevice **pdev);
> > > >  PCIBus *pci_get_bus_devfn(int *devfnp, PCIBus *root, const char *devaddr);
> > > > +void pci_bus_get_w64_range(PCIBus *bus, Range *range);
> > > >  
> > > >  int pci_parse_devaddr(const char *addr, int *domp, int *busp,
> > > >                        unsigned int *slotp, unsigned int *funcp);
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index 8c33352..d9f9bdf 100644
> > > > --- a/hw/pci/pci.c
> > > > +++ b/hw/pci/pci.c
> > > > @@ -2257,6 +2257,49 @@ void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, void *opaque)
> > > >      bus->iommu_opaque = opaque;
> > > >  }
> > > >  
> > > > +static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
> > > > +{
> > > > +    Range *range = opaque;
> > > > +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> > > > +    uint16_t cmd = pci_get_word(dev->config + PCI_COMMAND);
> > > > +    int r;
> > > > +
> > > > +    if (!(cmd & PCI_COMMAND_MEMORY)) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (pc->is_bridge) {
> > > > +        pcibus_t base = pci_bridge_get_base(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
> > > what guaranties that 'base' won't be below 4Gb
> > 
> > Hmm, this needs some thought.
> > 
> > > and be above '4Gb + above_4g_mem_size'?
> > 
> > This one is harmless - guest will only hurt itself if
> > it misconfigures the bridge like this.
> 
> start of w64 is hardcoded in QEMU and Seabios as (4Gb+above_4gb_ram_size),
> so maybe there is no need to update/modify it at all?
> 

Well the point is to have QEMU obey whatever bios does.
seabios hardcodes the start of window but some other
firmware might not.


> > 
> > > > +        pcibus_t limit = pci_bridge_get_limit(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
> > > > +
> > > > +        if (limit >= base) {
> > > > +            Range pref_range;
> > > > +            pref_range.begin = base;
> > > > +            pref_range.end = limit + 1;
> > > > +            range_extend(range, &pref_range);
> > > > +        }
> > > > +    }
> > > > +    for (r = 0; r < PCI_NUM_REGIONS; ++r) {
> > > > +        PCIIORegion *region = &dev->io_regions[r];
> > > > +        Range region_range;
> > > > +
> > > > +        if (!region->size ||
> > > > +            (region->type & PCI_BASE_ADDRESS_SPACE_IO) ||
> > > > +            !(region->type & PCI_BASE_ADDRESS_MEM_TYPE_64)) {
> > > > +            continue;
> > > > +        }
> > > > +        region_range.begin = pci_get_quad(dev->config + pci_bar(dev, r));
> > > > +        region_range.end = region_range.begin + region->size;
> > > > +        range_extend(range, &region_range);
> > > > +    }
> > > > +}
> > > > +
> > > > +void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> > > > +{
> > > > +    range->begin = range->end = 0;
> > > > +    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> > > > +}
> > > > +
> > > >  static const TypeInfo pci_device_type_info = {
> > > >      .name = TYPE_PCI_DEVICE,
> > > >      .parent = TYPE_DEVICE,