[PATCH 0/4] Add PCI to PCI bridge support to Xen

[PATCH 0/4] Add PCI to PCI bridge support to Xen

[Don Slutz]

The biggest use of these is to allow Windows disk images that are
not built on Xen to be started without any changes.

patch #1 is a bug fix.
patch #2 fixes hvmloader.

These are the patches that enable PCI to PCI bridges.

patch #3 and patch #4 allow better access to disks and nics on PCI to PCI bridges.

I.E. make imported Windows disk images more usable under Xen.

Don Slutz (4):
  hvmloader: Fixup pci_write* macros
  hvmloader: Add support for PCI to PCI bridge
  Allow vif= to specify PCI address for each nic
  Allow disk= to specify their emulated bus address

 docs/misc/xl-disk-configuration.txt         |  25 +
 docs/misc/xl-network-configuration.markdown |  14 +
 tools/firmware/hvmloader/pci.c              | 805 +++++++++++++++++++++-------
 tools/firmware/hvmloader/pci_regs.h         |  39 ++
 tools/firmware/hvmloader/util.h             |   6 +-
 tools/libxl/libxl_dm.c                      | 194 +++++--
 tools/libxl/libxl_types.idl                 |   6 +
 tools/libxl/libxlu_disk_l.l                 |   4 +
 tools/libxl/xl_cmdimpl.c                    |   4 +
 9 files changed, 857 insertions(+), 240 deletions(-)

-- 
1.8.4

[PATCH 1/4] hvmloader: Fixup pci_write* macros

[Don Slutz]

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <dslutz@verizon.com>
---
 tools/firmware/hvmloader/util.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index a70e4aa..8431f2d 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -82,9 +82,9 @@ uint32_t pci_read(uint32_t devfn, uint32_t reg, uint32_t len);
 #define pci_readw(devfn, reg) ((uint16_t)pci_read(devfn, reg, 2))
 #define pci_readl(devfn, reg) ((uint32_t)pci_read(devfn, reg, 4))
 void pci_write(uint32_t devfn, uint32_t reg, uint32_t len, uint32_t val);
-#define pci_writeb(devfn, reg, val) (pci_write(devfn, reg, 1, (uint8_t) val))
-#define pci_writew(devfn, reg, val) (pci_write(devfn, reg, 2, (uint16_t)val))
-#define pci_writel(devfn, reg, val) (pci_write(devfn, reg, 4, (uint32_t)val))
+#define pci_writeb(devfn, reg, val) (pci_write(devfn, reg, 1, (uint8_t) (val)))
+#define pci_writew(devfn, reg, val) (pci_write(devfn, reg, 2, (uint16_t)(val)))
+#define pci_writel(devfn, reg, val) (pci_write(devfn, reg, 4, (uint32_t)(val)))
 
 /* Get a pointer to the shared-info page */
 struct shared_info *get_shared_info(void) __attribute__ ((const));
-- 
1.8.4

Re: [PATCH 1/4] hvmloader: Fixup pci_write* macros

[Andrew Cooper]

On 15/06/15 15:15, Don Slutz wrote:
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> CC: Don Slutz <dslutz@verizon.com>

Fix how?  It looks like you are bracketing val.

This is an improvement, but please always be specific as to what is
being fixed.

Furthermore, what about devfn or reg?

~Andrew

> ---
>  tools/firmware/hvmloader/util.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index a70e4aa..8431f2d 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -82,9 +82,9 @@ uint32_t pci_read(uint32_t devfn, uint32_t reg, uint32_t len);
>  #define pci_readw(devfn, reg) ((uint16_t)pci_read(devfn, reg, 2))
>  #define pci_readl(devfn, reg) ((uint32_t)pci_read(devfn, reg, 4))
>  void pci_write(uint32_t devfn, uint32_t reg, uint32_t len, uint32_t val);
> -#define pci_writeb(devfn, reg, val) (pci_write(devfn, reg, 1, (uint8_t) val))
> -#define pci_writew(devfn, reg, val) (pci_write(devfn, reg, 2, (uint16_t)val))
> -#define pci_writel(devfn, reg, val) (pci_write(devfn, reg, 4, (uint32_t)val))
> +#define pci_writeb(devfn, reg, val) (pci_write(devfn, reg, 1, (uint8_t) (val)))
> +#define pci_writew(devfn, reg, val) (pci_write(devfn, reg, 2, (uint16_t)(val)))
> +#define pci_writel(devfn, reg, val) (pci_write(devfn, reg, 4, (uint32_t)(val)))
>  
>  /* Get a pointer to the shared-info page */
>  struct shared_info *get_shared_info(void) __attribute__ ((const));

Re: [PATCH 1/4] hvmloader: Fixup pci_write* macros

[Don Slutz]

On 06/15/15 10:19, Andrew Cooper wrote:
> On 15/06/15 15:15, Don Slutz wrote:
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> CC: Don Slutz <dslutz@verizon.com>
> 
> Fix how?  It looks like you are bracketing val.
> 

If val is an expression, the macro most likely does the wrong thing.

For example:

pci_writeb(devfn, PCI_IO_BASE, addr >> PCI_IO_SHIFT);


is pci_write(devfn, reg, 1, (uint8_t)addr >> PCI_IO_SHIFT); which is not
correct.  I would expect:

pci_write(devfn, reg, 1, (uint8_t)(addr >> PCI_IO_SHIFT));

> This is an improvement, but please always be specific as to what is
> being fixed.
> 

Does:

Improve pci_write* macros to act like a function does.

scan better?

> Furthermore, what about devfn or reg?
> 

devfn and reg do not need the bracketing since they are just passed,
but I have no issue with adding the extra brackets.

   -Don Slutz

> ~Andrew
> 
>> ---
>>  tools/firmware/hvmloader/util.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
>> index a70e4aa..8431f2d 100644
>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -82,9 +82,9 @@ uint32_t pci_read(uint32_t devfn, uint32_t reg, uint32_t len);
>>  #define pci_readw(devfn, reg) ((uint16_t)pci_read(devfn, reg, 2))
>>  #define pci_readl(devfn, reg) ((uint32_t)pci_read(devfn, reg, 4))
>>  void pci_write(uint32_t devfn, uint32_t reg, uint32_t len, uint32_t val);
>> -#define pci_writeb(devfn, reg, val) (pci_write(devfn, reg, 1, (uint8_t) val))
>> -#define pci_writew(devfn, reg, val) (pci_write(devfn, reg, 2, (uint16_t)val))
>> -#define pci_writel(devfn, reg, val) (pci_write(devfn, reg, 4, (uint32_t)val))
>> +#define pci_writeb(devfn, reg, val) (pci_write(devfn, reg, 1, (uint8_t) (val)))
>> +#define pci_writew(devfn, reg, val) (pci_write(devfn, reg, 2, (uint16_t)(val)))
>> +#define pci_writel(devfn, reg, val) (pci_write(devfn, reg, 4, (uint32_t)(val)))
>>  
>>  /* Get a pointer to the shared-info page */
>>  struct shared_info *get_shared_info(void) __attribute__ ((const));
> 

Re: [PATCH 1/4] hvmloader: Fixup pci_write* macros

[Andrew Cooper]

On 15/06/15 15:30, Don Slutz wrote:
> On 06/15/15 10:19, Andrew Cooper wrote:
>> On 15/06/15 15:15, Don Slutz wrote:
>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>> CC: Don Slutz <dslutz@verizon.com>
>> Fix how?  It looks like you are bracketing val.
>>
> If val is an expression, the macro most likely does the wrong thing.
>
> For example:
>
> pci_writeb(devfn, PCI_IO_BASE, addr >> PCI_IO_SHIFT);
>
>
> is pci_write(devfn, reg, 1, (uint8_t)addr >> PCI_IO_SHIFT); which is not
> correct.  I would expect:
>
> pci_write(devfn, reg, 1, (uint8_t)(addr >> PCI_IO_SHIFT));
>
>> This is an improvement, but please always be specific as to what is
>> being fixed.
>>
> Does:
>
> Improve pci_write* macros to act like a function does.
>
> scan better?
>
>> Furthermore, what about devfn or reg?
>>
> devfn and reg do not need the bracketing since they are just passed,
> but I have no issue with adding the extra brackets.

Macros, under all circumstances, should have all of their parameters
bracketed for safety.

Simply "Fix pci_write* macros by bracketing their parameters" would be
ok.  The why is obvious to C programmers, but simply "fix" is too
generic to be useful as a description.

~Andrew

Re: [PATCH 1/4] hvmloader: Fixup pci_write* macros

[Jan Beulich]

>>> On 15.06.15 at 16:35, <andrew.cooper3@citrix.com> wrote:
> On 15/06/15 15:30, Don Slutz wrote:
>> On 06/15/15 10:19, Andrew Cooper wrote:
>>> Furthermore, what about devfn or reg?
>>>
>> devfn and reg do not need the bracketing since they are just passed,
>> but I have no issue with adding the extra brackets.
> 
> Macros, under all circumstances, should have all of their parameters
> bracketed for safety.

No, as this harms readability:

#define macro(x) function((x))

would completely pointlessly be having an extra set of parentheses.
And

#define macro(x, y) function(x, y)

is just the logical extension to that: There is nothing the expressions
passed as arguments could contain to require parenthesization, as
there's no operator of precedence lower than comma (and if either
argument contains a comma expression, it needs to be
parenthesized at the invocation site anyway).

Jan

Re: [PATCH 1/4] hvmloader: Fixup pci_write* macros

[Mihai Donțu]

On Mon, 15 Jun 2015 16:23:13 +0100 Jan Beulich wrote:
> >>> On 15.06.15 at 16:35, <andrew.cooper3@citrix.com> wrote:
> > On 15/06/15 15:30, Don Slutz wrote:
> >> On 06/15/15 10:19, Andrew Cooper wrote:
> >>> Furthermore, what about devfn or reg?
> >>>
> >> devfn and reg do not need the bracketing since they are just passed,
> >> but I have no issue with adding the extra brackets.
> > 
> > Macros, under all circumstances, should have all of their parameters
> > bracketed for safety.
> 
> No, as this harms readability:
> 
> #define macro(x) function((x))
> 
> would completely pointlessly be having an extra set of parentheses.
> And
> 
> #define macro(x, y) function(x, y)
> 
> is just the logical extension to that: There is nothing the expressions
> passed as arguments could contain to require parenthesization, as
> there's no operator of precedence lower than comma (and if either
> argument contains a comma expression, it needs to be
> parenthesized at the invocation site anyway).

I think parenthesis are just long term guards against classic surprises
such as:

#define macro(x) function(x * 2)
...
macro(N + 3)

You could treat this case separately though, but people often go for the
"good practice".

-- 
Mihai Donțu

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] hvmloader: Fixup pci_write* macros

[Andrew Cooper]

On 15/06/15 17:09, Mihai Donțu wrote:
> On Mon, 15 Jun 2015 16:23:13 +0100 Jan Beulich wrote:
>>>>> On 15.06.15 at 16:35, <andrew.cooper3@citrix.com> wrote:
>>> On 15/06/15 15:30, Don Slutz wrote:
>>>> On 06/15/15 10:19, Andrew Cooper wrote:
>>>>> Furthermore, what about devfn or reg?
>>>>>
>>>> devfn and reg do not need the bracketing since they are just passed,
>>>> but I have no issue with adding the extra brackets.
>>> Macros, under all circumstances, should have all of their parameters
>>> bracketed for safety.
>> No, as this harms readability:
>>
>> #define macro(x) function((x))
>>
>> would completely pointlessly be having an extra set of parentheses.
>> And
>>
>> #define macro(x, y) function(x, y)
>>
>> is just the logical extension to that: There is nothing the expressions
>> passed as arguments could contain to require parenthesization, as
>> there's no operator of precedence lower than comma (and if either
>> argument contains a comma expression, it needs to be
>> parenthesized at the invocation site anyway).
> I think parenthesis are just long term guards against classic surprises
> such as:
>
> #define macro(x) function(x * 2)
> ...
> macro(N + 3)
>
> You could treat this case separately though, but people often go for the
> "good practice".
>

Personally, I would go a step further and suggest that if a macro is not
doing {string,token}isation, or deliberately playing with local state,
it should be a static inline function instead (although I do appear to
be in the minority with this view).

This way, the compiler is able to provide far more coherent error
messages and spot type errors, both of which reduce the number of errors
which can accidentally be introduced.  Furthermore, the compiler is also
able to ignore the 'inline' if it decides it knows better, which
generally results in more efficient code.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/4] hvmloader: Fixup pci_write* macros

[Jan Beulich]

>>> On 15.06.15 at 19:14, <andrew.cooper3@citrix.com> wrote:
> Personally, I would go a step further and suggest that if a macro is not
> doing {string,token}isation, or deliberately playing with local state,
> it should be a static inline function instead (although I do appear to
> be in the minority with this view).
> 
> This way, the compiler is able to provide far more coherent error
> messages and spot type errors, both of which reduce the number of errors
> which can accidentally be introduced.  Furthermore, the compiler is also
> able to ignore the 'inline' if it decides it knows better, which
> generally results in more efficient code.

I certainly agree, but due to dependencies between headers this
isn't always possible. Plus, in cases where things just get forwarded
to a function type checking will still take place as desired. I.e. again
simple things like

#define macro(x, y) function(x, y, 0)

don't have a need to become inline functions. More involved
things would preferably indeed get converted.

Jan

Re: [PATCH 1/4] hvmloader: Fixup pci_write* macros

[Jan Beulich]

>>> On 15.06.15 at 18:09, <mihai.dontu@gmail.com> wrote:
> On Mon, 15 Jun 2015 16:23:13 +0100 Jan Beulich wrote:
>> >>> On 15.06.15 at 16:35, <andrew.cooper3@citrix.com> wrote:
>> > On 15/06/15 15:30, Don Slutz wrote:
>> >> On 06/15/15 10:19, Andrew Cooper wrote:
>> >>> Furthermore, what about devfn or reg?
>> >>>
>> >> devfn and reg do not need the bracketing since they are just passed,
>> >> but I have no issue with adding the extra brackets.
>> > 
>> > Macros, under all circumstances, should have all of their parameters
>> > bracketed for safety.
>> 
>> No, as this harms readability:
>> 
>> #define macro(x) function((x))
>> 
>> would completely pointlessly be having an extra set of parentheses.
>> And
>> 
>> #define macro(x, y) function(x, y)
>> 
>> is just the logical extension to that: There is nothing the expressions
>> passed as arguments could contain to require parenthesization, as
>> there's no operator of precedence lower than comma (and if either
>> argument contains a comma expression, it needs to be
>> parenthesized at the invocation site anyway).
> 
> I think parenthesis are just long term guards against classic surprises
> such as:
> 
> #define macro(x) function(x * 2)
> ...
> macro(N + 3)

There is no question that x needs to be parenthesized in the example
you give; that does in no way mean _all_ macro parameters need to
follow that, which is what my earlier reply was trying to explain.

Jan

Re: [PATCH 1/4] hvmloader: Fixup pci_write* macros

[Jan Beulich]

>>> On 15.06.15 at 16:30, <dslutz@verizon.com> wrote:
> On 06/15/15 10:19, Andrew Cooper wrote:
>> On 15/06/15 15:15, Don Slutz wrote:
>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>> CC: Don Slutz <dslutz@verizon.com>
>> 
>> Fix how?  It looks like you are bracketing val.
>> 
> 
> If val is an expression, the macro most likely does the wrong thing.
> 
> For example:
> 
> pci_writeb(devfn, PCI_IO_BASE, addr >> PCI_IO_SHIFT);
> 
> 
> is pci_write(devfn, reg, 1, (uint8_t)addr >> PCI_IO_SHIFT); which is not
> correct.  I would expect:
> 
> pci_write(devfn, reg, 1, (uint8_t)(addr >> PCI_IO_SHIFT));
> 
>> This is an improvement, but please always be specific as to what is
>> being fixed.
>> 
> 
> Does:
> 
> Improve pci_write* macros to act like a function does.
> 
> scan better?

How about simply "correctly parenthesize pci_write* macros"?

Jan

Re: [PATCH 1/4] hvmloader: Fixup pci_write* macros

[Jan Beulich]

>>> On 15.06.15 at 16:19, <andrew.cooper3@citrix.com> wrote:
> On 15/06/15 15:15, Don Slutz wrote:
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> CC: Don Slutz <dslutz@verizon.com>
> 
> Fix how?  It looks like you are bracketing val.
> 
> This is an improvement, but please always be specific as to what is
> being fixed.
> 
> Furthermore, what about devfn or reg?

They explicitly don't need parenthesization, as they're just being
forwarded.

Jan

>> --- a/tools/firmware/hvmloader/util.h
>> +++ b/tools/firmware/hvmloader/util.h
>> @@ -82,9 +82,9 @@ uint32_t pci_read(uint32_t devfn, uint32_t reg, uint32_t len);
>>  #define pci_readw(devfn, reg) ((uint16_t)pci_read(devfn, reg, 2))
>>  #define pci_readl(devfn, reg) ((uint32_t)pci_read(devfn, reg, 4))
>>  void pci_write(uint32_t devfn, uint32_t reg, uint32_t len, uint32_t val);
>> -#define pci_writeb(devfn, reg, val) (pci_write(devfn, reg, 1, (uint8_t) 
> val))
>> -#define pci_writew(devfn, reg, val) (pci_write(devfn, reg, 2, 
> (uint16_t)val))
>> -#define pci_writel(devfn, reg, val) (pci_write(devfn, reg, 4, 
> (uint32_t)val))
>> +#define pci_writeb(devfn, reg, val) (pci_write(devfn, reg, 1, (uint8_t) 
> (val)))
>> +#define pci_writew(devfn, reg, val) (pci_write(devfn, reg, 2, 
> (uint16_t)(val)))
>> +#define pci_writel(devfn, reg, val) (pci_write(devfn, reg, 4, 
> (uint32_t)(val)))
>>  
>>  /* Get a pointer to the shared-info page */
>>  struct shared_info *get_shared_info(void) __attribute__ ((const));

[PATCH 2/4] hvmloader: Add support for PCI to PCI bridge

[Don Slutz]

Most of this code is ported from SeaBIOS.

This allows many more PCI devices to be added.

It can also allow Windows to find it boot disks.

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <dslutz@verizon.com>
---
 tools/firmware/hvmloader/pci.c      | 805 +++++++++++++++++++++++++++---------
 tools/firmware/hvmloader/pci_regs.h |  39 ++
 2 files changed, 647 insertions(+), 197 deletions(-)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 5ff87a7..189ce5f 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -38,28 +38,572 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
 enum virtual_vga virtual_vga = VGA_none;
 unsigned long igd_opregion_pgbase = 0;
 
+#define ALIGN(x,a)              __ALIGN_MASK(x,(typeof(x))(a)-1)
+#define __ALIGN_MASK(x,mask)    (((x)+(mask))&~(mask))
+#define ALIGN_DOWN(x,a)         ((x) & ~((typeof(x))(a)-1))
+
+static inline uint8_t devfn_to_bus(uint32_t devfn) {
+    return devfn >> 8;
+}
+static inline uint8_t devfn_to_dev(uint32_t devfn) {
+    return (devfn >> 3) & 0x1f;
+}
+static inline uint8_t devfn_to_fn(uint32_t devfn) {
+    return devfn & 0x07;
+}
+
+struct pci_device {
+    uint32_t devfn;
+
+    /* Configuration space device information */
+    uint16_t class, vendor, device;
+    uint8_t header_type;
+    uint8_t secondary_bus;
+};
+
+#define PCI_CLASS_BRIDGE_PCI 0x0604
+#define PCI_HEADER_TYPE_MULTI_FUNCTION 0x80
+
+#define PCI_ROM_SLOT 6
+#define PCI_NUM_REGIONS 7
+#define PCI_BRIDGE_NUM_REGIONS 2
+
+#define PCI_DEVICE_MEM_MIN     0x1000
+#define PCI_BRIDGE_IO_MIN      0x1000
+#define PCI_BRIDGE_MEM_MIN   0x100000
+
+enum pci_region_type {
+    PCI_REGION_TYPE_IO,
+    PCI_REGION_TYPE_MEM,
+    PCI_REGION_TYPE_PREFMEM,
+    PCI_REGION_TYPE_COUNT,
+};
+
+static const char *region_type_name[] = {
+    [ PCI_REGION_TYPE_IO ]      = "io",
+    [ PCI_REGION_TYPE_MEM ]     = "mem",
+    [ PCI_REGION_TYPE_PREFMEM ] = "prefmem",
+};
+
+struct pci_region_entry {
+    struct pci_device *dev;
+    int bar;
+    uint64_t size;
+    uint64_t align;
+    int is64;
+    enum pci_region_type type;
+    struct pci_region_entry *next;
+};
+
+struct pci_region {
+    /* pci region assignments */
+    uint64_t base;
+    struct pci_region_entry *list;
+};
+
+struct pci_bus {
+    struct pci_region r[PCI_REGION_TYPE_COUNT];
+    struct pci_device *bus_dev;
+};
+
+/* Resources assignable to PCI devices via BARs. */
+struct resource {
+    uint64_t base, max;
+};
+
+static uint32_t pci_bar(struct pci_device *pci, int region_num)
+{
+    uint8_t type = pci->header_type & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
+
+    if (region_num != PCI_ROM_SLOT) {
+        return PCI_BASE_ADDRESS_0 + region_num * 4;
+    }
+
+    return type == PCI_HEADER_TYPE_BRIDGE ? PCI_ROM_ADDRESS1 : PCI_ROM_ADDRESS;
+}
+
+static void
+pci_set_io_region_addr(struct pci_device *pci, int bar, uint64_t addr, int is64)
+{
+    uint32_t ofs = pci_bar(pci, bar);
+    uint32_t addr32 = addr >> 32;
+
+    pci_writel(pci->devfn, ofs, addr);
+    if (is64)
+        pci_writel(pci->devfn, ofs + 4, addr32);
+}
+
+/****************************************************************
+ * Bus initialization
+ ****************************************************************/
+
+static void
+pci_bios_init_bus_rec(int bus, uint8_t *pci_bus)
+{
+    int devfn;
+    uint16_t class;
+
+    /* prevent accidental access to unintended devices */
+    for ( devfn = bus * 256; devfn < (bus + 1) * 256; devfn++ ) {
+        class = pci_readw(devfn, PCI_CLASS_DEVICE);
+        if (class == PCI_CLASS_BRIDGE_PCI) {
+            pci_writeb(devfn, PCI_SECONDARY_BUS, 255);
+            pci_writeb(devfn, PCI_SUBORDINATE_BUS, 0);
+        }
+    }
+
+    for ( devfn = bus * 256; devfn < (bus + 1) * 256; devfn++ ) {
+        uint8_t pribus, secbus, subbus;
+
+        class = pci_readw(devfn, PCI_CLASS_DEVICE);
+        if (class != PCI_CLASS_BRIDGE_PCI) {
+            continue;
+        }
+
+        pribus = pci_readb(devfn, PCI_PRIMARY_BUS);
+        if (pribus != bus) {
+            pci_writeb(devfn, PCI_PRIMARY_BUS, bus);
+        }
+
+        secbus = pci_readb(devfn, PCI_SECONDARY_BUS);
+        (*pci_bus)++;
+        if (*pci_bus != secbus) {
+            secbus = *pci_bus;
+            pci_writeb(devfn, PCI_SECONDARY_BUS, secbus);
+        }
+
+        /* set to max for access to all subordinate buses.
+           later set it to accurate value */
+        subbus = pci_readb(devfn, PCI_SUBORDINATE_BUS);
+        pci_writeb(devfn, PCI_SUBORDINATE_BUS, 255);
+
+        pci_bios_init_bus_rec(secbus, pci_bus);
+
+        if (subbus != *pci_bus) {
+            subbus = *pci_bus;
+        }
+        pci_writeb(devfn, PCI_SUBORDINATE_BUS, subbus);
+    }
+}
+
+static uint8_t
+pci_bios_init_bus(void)
+{
+    uint8_t pci_bus = 0;
+
+    pci_bios_init_bus_rec(0 /* host bus */, &pci_bus);
+    return pci_bus;
+}
+
+
+/****************************************************************
+ * Bus sizing
+ ****************************************************************/
+
+static void
+pci_bios_get_bar(struct pci_device *pci, int bar,
+                 int *ptype, uint64_t *psize, int *pis64)
+{
+    uint32_t ofs = pci_bar(pci, bar);
+    uint16_t devfn = pci->devfn;
+    uint32_t old = pci_readl(devfn, ofs);
+    int is64 = 0, type = PCI_REGION_TYPE_MEM;
+    uint64_t mask;
+    uint64_t val;
+
+    if (bar == PCI_ROM_SLOT) {
+        mask = PCI_ROM_ADDRESS_MASK;
+        pci_writel(devfn, ofs, mask);
+    } else {
+        if (old & PCI_BASE_ADDRESS_SPACE_IO) {
+            mask = PCI_BASE_ADDRESS_IO_MASK;
+            type = PCI_REGION_TYPE_IO;
+        } else {
+            mask = PCI_BASE_ADDRESS_MEM_MASK;
+            if (old & PCI_BASE_ADDRESS_MEM_PREFETCH)
+                type = PCI_REGION_TYPE_PREFMEM;
+            is64 = ((old & PCI_BASE_ADDRESS_MEM_TYPE_MASK)
+                    == PCI_BASE_ADDRESS_MEM_TYPE_64);
+        }
+        pci_writel(devfn, ofs, ~0);
+    }
+    val = pci_readl(devfn, ofs);
+    pci_writel(devfn, ofs, old);
+    if (is64) {
+        uint32_t hold = pci_readl(devfn, ofs + 4);
+        uint32_t high;
+
+        pci_writel(devfn, ofs + 4, ~0);
+        high = pci_readl(devfn, ofs + 4);
+        pci_writel(devfn, ofs + 4, hold);
+        val |= ((uint64_t)high << 32);
+        mask |= ((uint64_t)0xffffffff << 32);
+        *psize = (~(val & mask)) + 1;
+    } else {
+        *psize = ((~(val & mask)) + 1) & 0xffffffff;
+    }
+    *ptype = type;
+    *pis64 = is64;
+}
+
+static int pci_bios_bridge_region_is64(struct pci_region *r,
+                                       struct pci_device *pci, int type)
+{
+    uint32_t pmem;
+    struct pci_region_entry *entry;
+
+    if (type != PCI_REGION_TYPE_PREFMEM)
+        return 0;
+    pmem = pci_readl(pci->devfn, PCI_PREF_MEMORY_BASE);
+    if (!pmem) {
+        pci_writel(pci->devfn, PCI_PREF_MEMORY_BASE, 0xfff0fff0);
+        pmem = pci_readl(pci->devfn, PCI_PREF_MEMORY_BASE);
+        pci_writel(pci->devfn, PCI_PREF_MEMORY_BASE, 0x0);
+    }
+    if ((pmem & PCI_PREF_RANGE_TYPE_MASK) != PCI_PREF_RANGE_TYPE_64)
+        return 0;
+    entry = r->list;
+    while (entry) {
+        if (!entry->is64)
+            return 0;
+        entry = entry->next;
+    }
+    return 1;
+}
+
+static uint64_t pci_region_align(struct pci_region *r)
+{
+    if (!r->list)
+        return 1;
+    /* The first entry in the sorted list has the largest alignment */
+    return r->list->align;
+}
+
+static uint64_t pci_region_sum(struct pci_region *r)
+{
+    struct pci_region_entry *entry = r->list;
+    uint64_t sum = 0;
+    while (entry) {
+        sum += entry->size;
+        entry = entry->next;
+    }
+    return sum;
+}
+
+static void pci_region_migrate_64bit_entries(struct pci_region *from,
+                                             struct pci_region *to)
+{
+    struct pci_region_entry **pprev = &from->list, **last = &to->list;
+    while (*pprev) {
+        struct pci_region_entry *entry = *pprev;
+        if (!entry->is64) {
+            pprev = &entry->next;
+            continue;
+        }
+        /* Move from source list to destination list. */
+        *pprev = entry->next;
+        entry->next = NULL;
+        *last = entry;
+        last = &entry->next;
+    }
+}
+
+static void
+pci_region_create_entry(struct pci_region_entry * entry,
+                        struct pci_bus *bus, struct pci_device *dev,
+                        int bar, uint64_t size, uint64_t align, int type, int is64)
+{
+    struct pci_region_entry **pprev;
+
+    memset(entry, 0, sizeof(*entry));
+    entry->dev = dev;
+    entry->bar = bar;
+    entry->size = size;
+    entry->align = align;
+    entry->is64 = is64;
+    entry->type = type;
+
+    /* Insert into list in sorted order. */
+    for (pprev = &bus->r[type].list; *pprev; pprev = &(*pprev)->next) {
+        struct pci_region_entry *pos = *pprev;
+        if (pos->align < align || (pos->align == align && pos->size < size))
+            break;
+    }
+    entry->next = *pprev;
+    *pprev = entry;
+}
+
+static int pci_bios_check_devices(struct pci_device *pci_base, unsigned int nr_pci_devs,
+                                  struct pci_bus *busses, int MaxPCIBus)
+{
+    int pci_idx;
+    int secondary_bus;
+    struct pci_region_entry *entry = (struct pci_region_entry *)&busses[MaxPCIBus + 1];
+
+    /* Calculate resources needed for regular (non-bus) devices. */
+    for ( pci_idx = 0; pci_idx < nr_pci_devs; pci_idx++ ) {
+        struct pci_device *pci = &pci_base[pci_idx];
+        struct pci_bus *bus;
+        int i;
+
+        if (pci->class == PCI_CLASS_BRIDGE_PCI)
+            busses[pci->secondary_bus].bus_dev = pci;
+
+        bus = &busses[devfn_to_bus(pci->devfn)];
+        for (i = 0; i < PCI_NUM_REGIONS; i++) {
+            int type, is64;
+            uint64_t size;
+
+            if ((pci->class == PCI_CLASS_BRIDGE_PCI) &&
+                (i >= PCI_BRIDGE_NUM_REGIONS && i < PCI_ROM_SLOT))
+                continue;
+
+            pci_bios_get_bar(pci, i, &type, &size, &is64);
+            if (size == 0)
+                continue;
+
+            if (type != PCI_REGION_TYPE_IO && size < PCI_DEVICE_MEM_MIN)
+                size = PCI_DEVICE_MEM_MIN;
+            pci_region_create_entry(entry, bus, pci, i,
+                                    size, size, type, is64);
+            entry++;
+
+            if (is64)
+                i++;
+        }
+    }
+
+    /* Propagate required bus resources to parent busses. */
+    for (secondary_bus=MaxPCIBus; secondary_bus>0; secondary_bus--) {
+        struct pci_bus *s = &busses[secondary_bus];
+        struct pci_bus *parent;
+        int type;
+
+        if (!s->bus_dev)
+            continue;
+        parent = &busses[devfn_to_bus(s->bus_dev->devfn)];
+        for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) {
+            uint64_t align = (type == PCI_REGION_TYPE_IO) ?
+                PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN;
+            uint64_t sum;
+            uint64_t size;
+            int is64;                    
+
+            if (pci_region_align(&s->r[type]) > align)
+                align = pci_region_align(&s->r[type]);
+            sum = pci_region_sum(&s->r[type]);
+            size = ALIGN(sum, align);
+            is64 = pci_bios_bridge_region_is64(&s->r[type],
+                                               s->bus_dev, type);
+            /* entry->bar is -1 if the entry represents a bridge region */
+            pci_region_create_entry(entry, parent, s->bus_dev, -1,
+                                    size, align, type, is64);
+            entry++;
+        }
+    }
+    return 0;
+}
+
+
+/****************************************************************
+ * BAR assignment
+ ****************************************************************/
+
+/* Setup region bases (given the regions' size and alignment) */
+static int pci_bios_init_root_regions(struct pci_bus *bus,
+                                      struct resource *mem_resource,
+                                      struct resource *high_mem_resource,
+                                      struct resource *io_resource)
+{
+    uint64_t sum, align;
+    struct pci_region *r_end = &bus->r[PCI_REGION_TYPE_PREFMEM];
+    struct pci_region *r_start = &bus->r[PCI_REGION_TYPE_MEM];
+
+    bus->r[PCI_REGION_TYPE_IO].base = io_resource->base;
+
+    if (pci_region_align(r_start) >= pci_region_align(r_end)) {
+        /* Swap regions to improve alignment. */
+        r_end = r_start;
+        r_start = &bus->r[PCI_REGION_TYPE_PREFMEM];
+    }
+    align = pci_region_align(r_end);
+    r_end->base = ALIGN(mem_resource->base, align);
+    sum = pci_region_sum(r_end);
+    align = pci_region_align(r_start);
+    r_start->base = ALIGN((r_end->base + sum), align);
+    sum = pci_region_sum(r_start);
+
+    if ((r_start->base < mem_resource->base) ||
+        ((r_start->base + sum) > mem_resource->max))
+        /* Memory range requested is larger than available. */
+        return -1;
+    return 0;
+}
+
+static uint32_t pci_bios_size_root_regions(struct pci_bus *bus)
+{
+    uint64_t sum, align;
+    struct pci_region *r_end = &bus->r[PCI_REGION_TYPE_PREFMEM];
+    struct pci_region *r_start = &bus->r[PCI_REGION_TYPE_MEM];
+
+    if (pci_region_align(r_start) < pci_region_align(r_end)) {
+        /* Swap regions to improve alignment. */
+        r_end = r_start;
+        r_start = &bus->r[PCI_REGION_TYPE_PREFMEM];
+    }
+    sum = pci_region_sum(r_end);
+    align = pci_region_align(r_end);
+    r_end->base = ALIGN_DOWN((pci_mem_end - sum), align);
+    sum = pci_region_sum(r_start);
+    align = pci_region_align(r_start);
+    r_start->base = ALIGN_DOWN((r_end->base - sum), align);
+
+    return pci_mem_end - r_start->base;
+}
+
+#define PCI_IO_SHIFT            8
+#define PCI_MEMORY_SHIFT        16
+#define PCI_PREF_MEMORY_SHIFT   16
+
+static void
+pci_region_map_one_entry(struct pci_region_entry *entry, uint64_t addr)
+{
+    uint16_t devfn = entry->dev->devfn;
+    uint64_t limit = addr + entry->size - 1;
+    uint32_t cmd;
+
+    if (entry->size) {
+        /* Now enable the memory or I/O mapping. */
+        cmd = pci_readb(devfn, PCI_COMMAND);
+        if ( entry->type == PCI_REGION_TYPE_IO )
+            cmd |= PCI_COMMAND_IO;
+        else
+            cmd |= PCI_COMMAND_MEMORY;
+        pci_writeb(devfn, PCI_COMMAND, cmd);
+    }
+
+    if (entry->bar >= 0) {
+        if (entry->size)
+            printf("pci dev %02x:%02x.%x"
+                   " bar %d size "PRIllx": "PRIllx" [%s]\n",
+                   devfn_to_bus(devfn), devfn_to_dev(devfn),
+                   devfn_to_fn(devfn), entry->bar, PRIllx_arg(entry->size),
+                   PRIllx_arg(addr), region_type_name[entry->type]);
+
+        pci_set_io_region_addr(entry->dev, entry->bar, addr, entry->is64);
+        return;
+    }
+
+    if (entry->size)
+        printf("pci bri %02x:%02x.%x"
+               "       size "PRIllx": "PRIllx" [%s]\n",
+               devfn_to_bus(devfn), devfn_to_dev(devfn), devfn_to_fn(devfn),
+               PRIllx_arg(entry->size), PRIllx_arg(addr),
+               region_type_name[entry->type]);
+
+    if (entry->type == PCI_REGION_TYPE_IO) {
+        pci_writeb(devfn, PCI_IO_BASE, addr >> PCI_IO_SHIFT);
+        pci_writew(devfn, PCI_IO_BASE_UPPER16, 0);
+        pci_writeb(devfn, PCI_IO_LIMIT, limit >> PCI_IO_SHIFT);
+        pci_writew(devfn, PCI_IO_LIMIT_UPPER16, 0);
+    }
+    if (entry->type == PCI_REGION_TYPE_MEM) {
+        pci_writew(devfn, PCI_MEMORY_BASE, addr >> PCI_MEMORY_SHIFT);
+        pci_writew(devfn, PCI_MEMORY_LIMIT, limit >> PCI_MEMORY_SHIFT);
+    }
+    if (entry->type == PCI_REGION_TYPE_PREFMEM) {
+        ASSERT(PCI_MEMORY_SHIFT == PCI_PREF_MEMORY_SHIFT);
+        pci_writew(devfn, PCI_PREF_MEMORY_BASE, addr >> PCI_MEMORY_SHIFT);
+        pci_writew(devfn, PCI_PREF_MEMORY_LIMIT, limit >> PCI_MEMORY_SHIFT);
+        pci_writel(devfn, PCI_PREF_BASE_UPPER32, addr >> 32);
+        pci_writel(devfn, PCI_PREF_LIMIT_UPPER32, limit >> 32);
+    }
+}
+
+static void pci_region_map_entries(struct pci_bus *busses, struct pci_region *r)
+{
+    struct pci_region_entry *entry = r->list;
+    while (entry) {
+        uint64_t addr = r->base;
+        struct pci_region_entry *next = entry->next;
+
+        r->base += entry->size;
+        if (entry->bar == -1)
+            /* Update bus base address if entry is a bridge region */
+            busses[entry->dev->secondary_bus].r[entry->type].base = addr;
+        pci_region_map_one_entry(entry, addr);
+        entry = next;
+    }
+}
+
+static void pci_bios_map_devices(struct pci_bus *busses, int MaxPCIBus,
+                                 struct resource *mem_resource,
+                                 struct resource *high_mem_resource,
+                                 struct resource *io_resource)
+{
+    int bus;
+
+    if (pci_bios_init_root_regions(busses, mem_resource, high_mem_resource,
+                                   io_resource)) {
+        struct pci_region r64_mem, r64_pref;
+        uint64_t sum_mem;
+        uint64_t sum_pref;
+        uint64_t align_mem;
+        uint64_t align_pref;
+
+        r64_mem.list = NULL;
+        r64_pref.list = NULL;
+        pci_region_migrate_64bit_entries(&busses[0].r[PCI_REGION_TYPE_MEM],
+                                         &r64_mem);
+        pci_region_migrate_64bit_entries(&busses[0].r[PCI_REGION_TYPE_PREFMEM],
+                                         &r64_pref);
+        if (pci_bios_init_root_regions(busses, mem_resource, high_mem_resource,
+                                       io_resource)) {
+            printf("PCI: out of 32bit address space\n");
+            ASSERT(0);
+        }
+
+        sum_mem = pci_region_sum(&r64_mem);
+        sum_pref = pci_region_sum(&r64_pref);
+        align_mem = pci_region_align(&r64_mem);
+        align_pref = pci_region_align(&r64_pref);
+
+        ASSERT(high_mem_resource->base);
+        r64_mem.base = ALIGN(high_mem_resource->base, align_mem);
+        r64_pref.base = ALIGN(r64_mem.base + sum_mem, align_pref);
+        if ((r64_mem.base < high_mem_resource->base) ||
+            ((r64_pref.base + sum_pref) > high_mem_resource->max)) {
+            printf("PCI: out of 64bit address space\n");
+            ASSERT(0);
+        }
+        pci_region_map_entries(busses, &r64_mem);
+        pci_region_map_entries(busses, &r64_pref);
+    }
+    /* Map regions on each device. */
+    for ( bus = 0; bus <= MaxPCIBus; bus++ ) {
+        int type;
+        for ( type = 0; type < PCI_REGION_TYPE_COUNT; type++ )
+            pci_region_map_entries(busses, &busses[bus].r[type]);
+    }
+}
+
+/****************************************************************
+ * Main setup code
+ ****************************************************************/
+
 void pci_setup(void)
 {
-    uint8_t is_64bar, using_64bar, bar64_relocate = 0;
-    uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
-    uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
+    uint32_t devfn, cmd, mmio_total = 0;
     uint32_t vga_devfn = 256;
     uint16_t class, vendor_id, device_id;
-    unsigned int bar, pin, link, isa_irq;
-
+    unsigned int pin, link, isa_irq;
     /* Resources assignable to PCI devices via BARs. */
-    struct resource {
-        uint64_t base, max;
-    } *resource, mem_resource, high_mem_resource, io_resource;
-
-    /* Create a list of device BARs in descending order of size. */
-    struct bars {
-        uint32_t is_64bar;
-        uint32_t devfn;
-        uint32_t bar_reg;
-        uint64_t bar_sz;
-    } *bars = (struct bars *)scratch_start;
-    unsigned int i, nr_bars = 0;
+    struct resource mem_resource, high_mem_resource, io_resource;
+    uint32_t maxBus = 1;
+
+    struct pci_device *pci_devs = (struct pci_device *)scratch_start;
+    unsigned int nr_pci_devs = 0, pci_idx;
+    struct pci_bus *busses;
     uint64_t mmio_hole_size = 0;
 
     const char *s;
@@ -88,6 +632,38 @@ void pci_setup(void)
     printf("Relocating guest memory for lowmem MMIO space %s\n",
            allow_memory_relocate?"enabled":"disabled");
 
+    /* Calculate the max PCI bus */
+    maxBus = pci_bios_init_bus();
+
+    /* Scan the PCI busses */
+    for ( devfn = 0; devfn < (maxBus + 1) * 256; devfn++ )
+    {
+        class     = pci_readw(devfn, PCI_CLASS_DEVICE);
+        vendor_id = pci_readw(devfn, PCI_VENDOR_ID);
+        device_id = pci_readw(devfn, PCI_DEVICE_ID);
+        if ( (vendor_id == 0xffff) && (device_id == 0xffff) )
+            continue;
+        if ( (vendor_id == 0x8086) && 
+             ((device_id == 0x7000) || (device_id == 0x7110)) ) {
+            ASSERT(PCI_ISA_DEVFN == devfn);
+        }
+        pci_devs[nr_pci_devs].devfn = devfn;
+        pci_devs[nr_pci_devs].class = class;
+        pci_devs[nr_pci_devs].vendor = vendor_id;
+        pci_devs[nr_pci_devs].device = device_id;
+        pci_devs[nr_pci_devs].header_type = pci_readb(devfn, PCI_HEADER_TYPE);
+        pci_devs[nr_pci_devs].secondary_bus = 0;
+        nr_pci_devs++;
+
+        if (class != PCI_CLASS_BRIDGE_PCI) {
+            continue;
+        }
+        pci_devs[nr_pci_devs - 1].secondary_bus = pci_readb(devfn, PCI_SECONDARY_BUS);
+    }
+    busses = (struct pci_bus *)&pci_devs[nr_pci_devs];
+    memset(busses, 0, sizeof(*busses) * (maxBus + 1));
+    pci_bios_check_devices(pci_devs, nr_pci_devs, busses, maxBus);
+
     s = xenstore_read("platform/mmio_hole_size", NULL);
     if ( s )
         mmio_hole_size = strtoll(s, NULL, 0);
@@ -106,17 +682,14 @@ void pci_setup(void)
     outb(0x4d0, (uint8_t)(PCI_ISA_IRQ_MASK >> 0));
     outb(0x4d1, (uint8_t)(PCI_ISA_IRQ_MASK >> 8));
 
-    /* Scan the PCI bus and map resources. */
-    for ( devfn = 0; devfn < 256; devfn++ )
-    {
-        class     = pci_readw(devfn, PCI_CLASS_DEVICE);
-        vendor_id = pci_readw(devfn, PCI_VENDOR_ID);
-        device_id = pci_readw(devfn, PCI_DEVICE_ID);
-        if ( (vendor_id == 0xffff) && (device_id == 0xffff) )
-            continue;
+    /* Scan the PCI bus and adjust resources. */
+    for ( pci_idx = 0; pci_idx < nr_pci_devs; pci_idx++ ) {
+        struct pci_device *pci = &pci_devs[pci_idx];
 
-        ASSERT((devfn != PCI_ISA_DEVFN) ||
-               ((vendor_id == 0x8086) && (device_id == 0x7000)));
+        devfn = pci->devfn;
+        class = pci->class;
+        vendor_id = pci->vendor;
+        device_id = pci->device;
 
         switch ( class )
         {
@@ -170,75 +743,6 @@ void pci_setup(void)
             break;
         }
 
-        /* Map the I/O memory and port resources. */
-        for ( bar = 0; bar < 7; bar++ )
-        {
-            bar_sz_upper = 0;
-            bar_reg = PCI_BASE_ADDRESS_0 + 4*bar;
-            if ( bar == 6 )
-                bar_reg = PCI_ROM_ADDRESS;
-
-            bar_data = pci_readl(devfn, bar_reg);
-            if ( bar_reg != PCI_ROM_ADDRESS )
-            {
-                is_64bar = !!((bar_data & (PCI_BASE_ADDRESS_SPACE |
-                             PCI_BASE_ADDRESS_MEM_TYPE_MASK)) ==
-                             (PCI_BASE_ADDRESS_SPACE_MEMORY |
-                             PCI_BASE_ADDRESS_MEM_TYPE_64));
-                pci_writel(devfn, bar_reg, ~0);
-            }
-            else
-            {
-                is_64bar = 0;
-                pci_writel(devfn, bar_reg,
-                           (bar_data | PCI_ROM_ADDRESS_MASK) &
-                           ~PCI_ROM_ADDRESS_ENABLE);
-            }
-            bar_sz = pci_readl(devfn, bar_reg);
-            pci_writel(devfn, bar_reg, bar_data);
-
-            if ( bar_reg != PCI_ROM_ADDRESS )
-                bar_sz &= (((bar_data & PCI_BASE_ADDRESS_SPACE) ==
-                            PCI_BASE_ADDRESS_SPACE_MEMORY) ?
-                           PCI_BASE_ADDRESS_MEM_MASK :
-                           (PCI_BASE_ADDRESS_IO_MASK & 0xffff));
-            else
-                bar_sz &= PCI_ROM_ADDRESS_MASK;
-            if (is_64bar) {
-                bar_data_upper = pci_readl(devfn, bar_reg + 4);
-                pci_writel(devfn, bar_reg + 4, ~0);
-                bar_sz_upper = pci_readl(devfn, bar_reg + 4);
-                pci_writel(devfn, bar_reg + 4, bar_data_upper);
-                bar_sz = (bar_sz_upper << 32) | bar_sz;
-            }
-            bar_sz &= ~(bar_sz - 1);
-            if ( bar_sz == 0 )
-                continue;
-
-            for ( i = 0; i < nr_bars; i++ )
-                if ( bars[i].bar_sz < bar_sz )
-                    break;
-
-            if ( i != nr_bars )
-                memmove(&bars[i+1], &bars[i], (nr_bars-i) * sizeof(*bars));
-
-            bars[i].is_64bar = is_64bar;
-            bars[i].devfn   = devfn;
-            bars[i].bar_reg = bar_reg;
-            bars[i].bar_sz  = bar_sz;
-
-            if ( ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
-                  PCI_BASE_ADDRESS_SPACE_MEMORY) ||
-                 (bar_reg == PCI_ROM_ADDRESS) )
-                mmio_total += bar_sz;
-
-            nr_bars++;
-
-            /*The upper half is already calculated, skip it! */
-            if (is_64bar)
-                bar++;
-        }
-
         /* Map the interrupt. */
         pin = pci_readb(devfn, PCI_INTERRUPT_PIN);
         if ( pin != 0 )
@@ -247,8 +751,9 @@ void pci_setup(void)
             link = ((pin - 1) + (devfn >> 3)) & 3;
             isa_irq = pci_readb(PCI_ISA_DEVFN, 0x60 + link);
             pci_writeb(devfn, PCI_INTERRUPT_LINE, isa_irq);
-            printf("pci dev %02x:%x INT%c->IRQ%u\n",
-                   devfn>>3, devfn&7, 'A'+pin-1, isa_irq);
+            printf("pci dev %02x:%02x.%x INT%c->IRQ%u\n",
+                   devfn_to_bus(devfn), devfn_to_dev(devfn),
+                   devfn_to_fn(devfn), 'A'+pin-1, isa_irq);
         }
 
         /* Enable bus mastering. */
@@ -276,9 +781,12 @@ void pci_setup(void)
                    pci_mem_start, HVM_BELOW_4G_MMIO_START,
                    (long)mmio_hole_size);
         }
+        mmio_total = pci_bios_size_root_regions(busses);
     }
     else
     {
+        mmio_total = pci_bios_size_root_regions(busses);
+
         /*
          * At the moment qemu-xen can't deal with relocated memory regions.
          * It's too close to the release to make a proper fix; for now,
@@ -305,7 +813,6 @@ void pci_setup(void)
     {
         printf("Low MMIO hole not large enough for all devices,"
                " relocating some BARs to 64-bit\n");
-        bar64_relocate = 1;
     }
 
     /* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
@@ -352,104 +859,8 @@ void pci_setup(void)
     io_resource.base = 0xc000;
     io_resource.max = 0x10000;
 
-    /* Assign iomem and ioport resources in descending order of size. */
-    for ( i = 0; i < nr_bars; i++ )
-    {
-        devfn   = bars[i].devfn;
-        bar_reg = bars[i].bar_reg;
-        bar_sz  = bars[i].bar_sz;
-
-        /*
-         * Relocate to high memory if the total amount of MMIO needed
-         * is more than the low MMIO available.  Because devices are
-         * processed in order of bar_sz, this will preferentially
-         * relocate larger devices to high memory first.
-         *
-         * NB: The code here is rather fragile, as the check here to see
-         * whether bar_sz will fit in the low MMIO region doesn't match the
-         * real check made below, which involves aligning the base offset of the
-         * bar with the size of the bar itself.  As it happens, this will always
-         * be satisfied because:
-         * - The first one will succeed because the MMIO hole can only start at
-         *   0x{f,e,c,8}00000000.  If it fits, it will be aligned properly.
-         * - All subsequent ones will be aligned because the list is ordered
-         *   large to small, and bar_sz is always a power of 2. (At least
-         *   the code here assumes it to be.)
-         * Should either of those two conditions change, this code will break.
-         */
-        using_64bar = bars[i].is_64bar && bar64_relocate
-            && (mmio_total > (mem_resource.max - mem_resource.base));
-        bar_data = pci_readl(devfn, bar_reg);
-
-        if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
-             PCI_BASE_ADDRESS_SPACE_MEMORY )
-        {
-            /* Mapping high memory if PCI device is 64 bits bar */
-            if ( using_64bar ) {
-                if ( high_mem_resource.base & (bar_sz - 1) )
-                    high_mem_resource.base = high_mem_resource.base - 
-                        (high_mem_resource.base & (bar_sz - 1)) + bar_sz;
-                if ( !pci_hi_mem_start )
-                    pci_hi_mem_start = high_mem_resource.base;
-                resource = &high_mem_resource;
-                bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
-            } 
-            else {
-                resource = &mem_resource;
-                bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
-            }
-            mmio_total -= bar_sz;
-        }
-        else
-        {
-            resource = &io_resource;
-            bar_data &= ~PCI_BASE_ADDRESS_IO_MASK;
-        }
-
-        base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
-        bar_data |= (uint32_t)base;
-        bar_data_upper = (uint32_t)(base >> 32);
-        base += bar_sz;
-
-        if ( (base < resource->base) || (base > resource->max) )
-        {
-            printf("pci dev %02x:%x bar %02x size "PRIllx": no space for "
-                   "resource!\n", devfn>>3, devfn&7, bar_reg,
-                   PRIllx_arg(bar_sz));
-            continue;
-        }
-
-        resource->base = base;
-
-        pci_writel(devfn, bar_reg, bar_data);
-        if (using_64bar)
-            pci_writel(devfn, bar_reg + 4, bar_data_upper);
-        printf("pci dev %02x:%x bar %02x size "PRIllx": %x%08x\n",
-               devfn>>3, devfn&7, bar_reg,
-               PRIllx_arg(bar_sz),
-               bar_data_upper, bar_data);
-			
-
-        /* Now enable the memory or I/O mapping. */
-        cmd = pci_readw(devfn, PCI_COMMAND);
-        if ( (bar_reg == PCI_ROM_ADDRESS) ||
-             ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
-              PCI_BASE_ADDRESS_SPACE_MEMORY) )
-            cmd |= PCI_COMMAND_MEMORY;
-        else
-            cmd |= PCI_COMMAND_IO;
-        pci_writew(devfn, PCI_COMMAND, cmd);
-    }
-
-    if ( pci_hi_mem_start )
-    {
-        /*
-         * Make end address alignment match the start address one's so that
-         * fewer variable range MTRRs are needed to cover the range.
-         */
-        pci_hi_mem_end = ((high_mem_resource.base - 1) |
-                          ((pci_hi_mem_start & -pci_hi_mem_start) - 1)) + 1;
-    }
+    pci_bios_map_devices(busses, maxBus, &mem_resource, &high_mem_resource,
+                         &io_resource);
 
     if ( vga_devfn != 256 )
     {
diff --git a/tools/firmware/hvmloader/pci_regs.h b/tools/firmware/hvmloader/pci_regs.h
index 7bf2d87..66cf6de 100644
--- a/tools/firmware/hvmloader/pci_regs.h
+++ b/tools/firmware/hvmloader/pci_regs.h
@@ -105,6 +105,45 @@
 #define PCI_MIN_GNT  0x3e /* 8 bits */
 #define PCI_MAX_LAT  0x3f /* 8 bits */
 
+/* Header type 1 (PCI-to-PCI bridges) */
+#define PCI_PRIMARY_BUS         0x18    /* Primary bus number */
+#define PCI_SECONDARY_BUS       0x19    /* Secondary bus number */
+#define PCI_SUBORDINATE_BUS     0x1a    /* Highest bus number behind the bridge */
+#define PCI_SEC_LATENCY_TIMER   0x1b    /* Latency timer for secondary interface */
+#define PCI_IO_BASE             0x1c    /* I/O range behind the bridge */
+#define PCI_IO_LIMIT            0x1d
+#define  PCI_IO_RANGE_TYPE_MASK 0x0fUL  /* I/O bridging type */
+#define  PCI_IO_RANGE_TYPE_16   0x00
+#define  PCI_IO_RANGE_TYPE_32   0x01
+#define  PCI_IO_RANGE_MASK      (~0x0fUL)
+#define PCI_SEC_STATUS          0x1e    /* Secondary status register, only bit 14 used */
+#define PCI_MEMORY_BASE         0x20    /* Memory range behind */
+#define PCI_MEMORY_LIMIT        0x22
+#define  PCI_MEMORY_RANGE_TYPE_MASK 0x0fUL
+#define  PCI_MEMORY_RANGE_MASK  (~0x0fUL)
+#define PCI_PREF_MEMORY_BASE    0x24    /* Prefetchable memory range behind */
+#define PCI_PREF_MEMORY_LIMIT   0x26
+#define  PCI_PREF_RANGE_TYPE_MASK 0x0fUL
+#define  PCI_PREF_RANGE_TYPE_32 0x00
+#define  PCI_PREF_RANGE_TYPE_64 0x01
+#define  PCI_PREF_RANGE_MASK    (~0x0fUL)
+#define PCI_PREF_BASE_UPPER32   0x28    /* Upper half of prefetchable memory range */
+#define PCI_PREF_LIMIT_UPPER32  0x2c
+#define PCI_IO_BASE_UPPER16     0x30    /* Upper half of I/O addresses */
+#define PCI_IO_LIMIT_UPPER16    0x32
+/* 0x34 same as for htype 0 */
+/* 0x35-0x3b is reserved */
+#define PCI_ROM_ADDRESS1        0x38    /* Same as PCI_ROM_ADDRESS, but for htype 1 */
+/* 0x3c-0x3d are same as for htype 0 */
+#define PCI_BRIDGE_CONTROL      0x3e
+#define  PCI_BRIDGE_CTL_PARITY  0x01    /* Enable parity detection on secondary interface */
+#define  PCI_BRIDGE_CTL_SERR    0x02    /* The same for SERR forwarding */
+#define  PCI_BRIDGE_CTL_ISA     0x04    /* Enable ISA mode */
+#define  PCI_BRIDGE_CTL_VGA     0x08    /* Forward VGA addresses */
+#define  PCI_BRIDGE_CTL_MASTER_ABORT    0x20  /* Report master aborts */
+#define  PCI_BRIDGE_CTL_BUS_RESET       0x40    /* Secondary bus reset */
+#define  PCI_BRIDGE_CTL_FAST_BACK       0x80    /* Fast Back2Back enabled on secondary interface */
+
 #define PCI_INTEL_OPREGION 0xfc /* 4 bits */
 
 #endif /* __HVMLOADER_PCI_REGS_H__ */
-- 
1.8.4

Re: [PATCH 2/4] hvmloader: Add support for PCI to PCI bridge

[Andrew Cooper]

On 15/06/15 15:15, Don Slutz wrote:
> Most of this code is ported from SeaBIOS.
>
>

SeaBIOS is LGPLv3, while Hvmloader is GPLv2

IANAL, but the FSF indicates that this is not a compatible combination.

~Andrew

Re: [PATCH 2/4] hvmloader: Add support for PCI to PCI bridge

[Lars Kurth]

> On 15 Jun 2015, at 15:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 15/06/15 15:15, Don Slutz wrote:
>> Most of this code is ported from SeaBIOS.
>> 
>> 
> 
> SeaBIOS is LGPLv3, while Hvmloader is GPLv2
> 
> IANAL, but the FSF indicates that this is not a compatible combination.

This is correct. See https://www.gnu.org/licenses/gpl-faq.html#AllCompatibility for more information.
It is not clear to me from this mail and the patch though whether there is a problem and what it is. 

The patch does not appear to create any additional linkage between SeaBIOS and Hvmloader (unless it was there before, in which case we already have an issue). Is this correct?

If so, then this becomes a question of what "this code is ported from SeaBIOS" exactly means and whether that implies a licensing issue. In other words, were any portions of files from SeaBIOS copied, etc. If so, what exact license were these under, etc. Maybe you can clarify.

Regards
Lars

Re: [PATCH 2/4] hvmloader: Add support for PCI to PCI bridge

[George Dunlap]

On Mon, Jun 15, 2015 at 3:26 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 15/06/15 15:15, Don Slutz wrote:
>> Most of this code is ported from SeaBIOS.
>>
>>
>
> SeaBIOS is LGPLv3, while Hvmloader is GPLv2
>
> IANAL, but the FSF indicates that this is not a compatible combination.

So I guess in part it depends on the degree to which "ported" means
"copied verbatim" vs just "used as an inspiration for writing the new
functionality".  (I.e., the degree to which the authors of SeaBIOS can
claim copyright over the code in this patch.)

 -George

Re: [PATCH 2/4] hvmloader: Add support for PCI to PCI bridge

[Don Slutz]

On 06/15/15 10:58, George Dunlap wrote:
> On Mon, Jun 15, 2015 at 3:26 PM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 15/06/15 15:15, Don Slutz wrote:
>>> Most of this code is ported from SeaBIOS.
>>>
>>>
>>
>> SeaBIOS is LGPLv3, while Hvmloader is GPLv2
>>
>> IANAL, but the FSF indicates that this is not a compatible combination.
> 
> So I guess in part it depends on the degree to which "ported" means
> "copied verbatim" vs just "used as an inspiration for writing the new
> functionality".  (I.e., the degree to which the authors of SeaBIOS can
> claim copyright over the code in this patch.)
> 

I was not careful enough during the port.  I does look like I "copied"
some routines and data structures.  So my take is I messed up. :(

However it does become complex because in QEMU's hw/pci/pci.c:

Which has neither licence, it says:

/*
 * QEMU PCI bus manager
 *
 * Copyright (c) 2004 Fabrice Bellard
 *
 * Permission is hereby granted, free of charge, to any person obtaining
a copy
 * of this software and associated documentation files (the "Software"),
to deal
 * in the Software without restriction, including without limitation the
rights
 * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
 * copies of the Software, and to permit persons to whom the Software is
 * furnished to do so, subject to the following conditions:
 *
 * The above copyright notice and this permission notice shall be
included in
 * all copies or substantial portions of the Software.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
 * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
 * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
OTHER
 * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
ARISING FROM,
 * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 * THE SOFTWARE.
 */

and has routine:


int pci_bar(PCIDevice *d, int reg)
{
    uint8_t type;

    if (reg != PCI_ROM_SLOT)
        return PCI_BASE_ADDRESS_0 + reg * 4;

    type = d->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
    return type == PCI_HEADER_TYPE_BRIDGE ? PCI_ROM_ADDRESS1 :
PCI_ROM_ADDRESS;
}


vs SeaBIOS src/fw/pciinit.c:

// Initialize PCI devices (on emulators)
//
// Copyright (C) 2008  Kevin O'Connor <kevin@koconnor.net>
// Copyright (C) 2006 Fabrice Bellard
//
// This file may be distributed under the terms of the GNU LGPLv3 license.

static u32 pci_bar(struct pci_device *pci, int region_num)
{
    if (region_num != PCI_ROM_SLOT) {
        return PCI_BASE_ADDRESS_0 + region_num * 4;
    }

#define PCI_HEADER_TYPE_MULTI_FUNCTION 0x80
    u8 type = pci->header_type & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
    return type == PCI_HEADER_TYPE_BRIDGE ? PCI_ROM_ADDRESS1 :
PCI_ROM_ADDRESS;
}


which looks to me as long as I list

Copyright (c) 2004 Fabrice Bellard

I could add this to Xen.

   -Don Slutz

>  -George
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 2/4] hvmloader: Add support for PCI to PCI bridge

[Don Slutz]

On 06/15/15 10:26, Andrew Cooper wrote:
> On 15/06/15 15:15, Don Slutz wrote:
>> Most of this code is ported from SeaBIOS.
>>
>>
> 
> SeaBIOS is LGPLv3, while Hvmloader is GPLv2
> 
> IANAL, but the FSF indicates that this is not a compatible combination.
> 

Sigh,  Well I did code some of it from scratch, but that is not clear.

So it looks like I will need to withdraw this patch.

   -Don Slutz

> ~Andrew
> 

[PATCH 3/4] Allow vif= to specify PCI address for each nic

[Don Slutz]

This allows more then 32 nics.

This can help with Windows finding nics at boot time.

This allows changing config file:

    builder = "hvm"
    device_model_args_hvm = [
     "-device",
     "pci-bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0",
     "-device",
     "vmxnet3,id=nic3,netdev=net3,mac=00:0c:29:86:44:be,bus=pciBridge5.0,addr=0x4.0x0",
     "-netdev",
     "type=tap,id=net3,ifname=vif.3-emu,script=/etc/qemu-ifup,downscript=no",
    ]
    vif = [
    ]

to:

    builder = "hvm"
    device_model_args_hvm = [
     "-device",
     "pci-bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0",
    ]
    vif = [
     "model=vmxnet3,bridge=xenbr0,mac=00:0c:29:86:44:a0,bus=pciBridge5.0,addr=0x4.0x0",
    ]

which enables usage of xen-netback.

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <dslutz@verizon.com>
---
 docs/misc/xl-network-configuration.markdown | 14 ++++++++++++++
 tools/libxl/libxl_dm.c                      | 23 ++++++++++++++++++++---
 tools/libxl/libxl_types.idl                 |  2 ++
 tools/libxl/xl_cmdimpl.c                    |  4 ++++
 4 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/docs/misc/xl-network-configuration.markdown b/docs/misc/xl-network-configuration.markdown
index 3c439d4..6a7f6db 100644
--- a/docs/misc/xl-network-configuration.markdown
+++ b/docs/misc/xl-network-configuration.markdown
@@ -60,6 +60,20 @@ strategy. Otherwise in general you should prefer to generate a random
 MAC and set the locally administered bit since this allows for more
 bits of randomness than using the Xen OUI.
 
+### bus
+
+Specifies the name of the network bridge which this VIF should be
+added to. The default is `xenbr0`. The bridge must be configured using
+your distribution's network configuration tools. See the [wiki][net]
+for guidance and examples.
+
+### addr
+
+Specifies the name of the network bridge which this VIF should be
+added to. The default is `xenbr0`. The bridge must be configured using
+your distribution's network configuration tools. See the [wiki][net]
+for guidance and examples.
+
 ### bridge
 
 Specifies the name of the network bridge which this VIF should be
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 8a8f972..2bf4dab 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -315,6 +315,16 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
                 const char *ifname = libxl__device_nic_devname(gc,
                                                 domid, nics[i].devid,
                                                 LIBXL_NIC_TYPE_VIF_IOEMU);
+                if (nics[i].bus && nics[i].bus[0]) {
+                    LOG(ERROR,
+                        "bus= not supported by device_model_version=qemu-xen-traditional");
+                    return ERROR_INVAL;
+                }
+                if (nics[i].addr && nics[i].addr[0]) {
+                    LOG(ERROR,
+                        "addr= not supported by device_model_version=qemu-xen-traditional");
+                    return ERROR_INVAL;
+                }
                 flexarray_vappend(dm_args,
                                   "-net",
                                   GCSPRINTF(
@@ -747,11 +757,18 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                 const char *ifname = libxl__device_nic_devname(gc,
                                                 guest_domid, nics[i].devid,
                                                 LIBXL_NIC_TYPE_VIF_IOEMU);
+                char * busp = "";
+                char * addrp = "";
+
+                if (nics[i].bus && nics[i].bus[0])
+                    busp = libxl__sprintf(gc, ",bus=%s", nics[i].bus);
+                if (nics[i].addr && nics[i].addr[0])
+                    addrp = libxl__sprintf(gc, ",addr=%s", nics[i].addr);
                 flexarray_append(dm_args, "-device");
-                flexarray_append(dm_args,
-                   libxl__sprintf(gc, "%s,id=nic%d,netdev=net%d,mac=%s",
+                flexarray_append(dm_args, GCSPRINTF(
+                                     "%s,id=nic%d,netdev=net%d,mac=%s%s%s",
                                                 nics[i].model, nics[i].devid,
-                                                nics[i].devid, smac));
+                                          nics[i].devid, smac, busp, addrp));
                 flexarray_append(dm_args, "-netdev");
                 flexarray_append(dm_args, GCSPRINTF(
                                           "type=tap,id=net%d,ifname=%s,"
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index c7af74b..6d3b058 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -521,6 +521,8 @@ libxl_device_nic = Struct("device_nic", [
     ("mtu", integer),
     ("model", string),
     ("mac", libxl_mac),
+    ("bus", string),
+    ("addr", string),
     ("ip", string),
     ("bridge", string),
     ("ifname", string),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index b3fe0cd..c48d52a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -961,6 +961,10 @@ static int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char *to
             nic->mac[i] = val;
             oparg = endptr + 1;
         }
+    } else if (MATCH_OPTION("bus", token, oparg)) {
+        replace_string(&nic->bus, oparg);
+    } else if (MATCH_OPTION("addr", token, oparg)) {
+        replace_string(&nic->addr, oparg);
     } else if (MATCH_OPTION("bridge", token, oparg)) {
         replace_string(&nic->bridge, oparg);
     } else if (MATCH_OPTION("netdev", token, oparg)) {
-- 
1.8.4

Re: [PATCH 3/4] Allow vif= to specify PCI address for each nic

[Wei Liu]

On Mon, Jun 15, 2015 at 10:15:51AM -0400, Don Slutz wrote:
> This allows more then 32 nics.
> 

How does this patch help? What prevents you from having more than 32
nics?

> This can help with Windows finding nics at boot time.
> 
> This allows changing config file:
> 
>     builder = "hvm"
>     device_model_args_hvm = [
>      "-device",
>      "pci-bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0",
>      "-device",
>      "vmxnet3,id=nic3,netdev=net3,mac=00:0c:29:86:44:be,bus=pciBridge5.0,addr=0x4.0x0",
>      "-netdev",
>      "type=tap,id=net3,ifname=vif.3-emu,script=/etc/qemu-ifup,downscript=no",
>     ]
>     vif = [
>     ]
> 
> to:
> 
>     builder = "hvm"
>     device_model_args_hvm = [
>      "-device",
>      "pci-bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0",
>     ]
>     vif = [
>      "model=vmxnet3,bridge=xenbr0,mac=00:0c:29:86:44:a0,bus=pciBridge5.0,addr=0x4.0x0",
>     ]
> 
> which enables usage of xen-netback.
> 

In any case, exposing HVM-only options to top-level vif configuration
space doesn't look right. Why do you want to set bus and addr? The
rationale should be stated in commit message.

> Signed-off-by: Don Slutz <dslutz@verizon.com>
> CC: Don Slutz <dslutz@verizon.com>
> ---
>  docs/misc/xl-network-configuration.markdown | 14 ++++++++++++++
>  tools/libxl/libxl_dm.c                      | 23 ++++++++++++++++++++---
>  tools/libxl/libxl_types.idl                 |  2 ++
>  tools/libxl/xl_cmdimpl.c                    |  4 ++++
>  4 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/misc/xl-network-configuration.markdown b/docs/misc/xl-network-configuration.markdown
> index 3c439d4..6a7f6db 100644
> --- a/docs/misc/xl-network-configuration.markdown
> +++ b/docs/misc/xl-network-configuration.markdown
> @@ -60,6 +60,20 @@ strategy. Otherwise in general you should prefer to generate a random
>  MAC and set the locally administered bit since this allows for more
>  bits of randomness than using the Xen OUI.
>  
> +### bus
> +
> +Specifies the name of the network bridge which this VIF should be
> +added to. The default is `xenbr0`. The bridge must be configured using
> +your distribution's network configuration tools. See the [wiki][net]
> +for guidance and examples.
> +
> +### addr
> +
> +Specifies the name of the network bridge which this VIF should be
> +added to. The default is `xenbr0`. The bridge must be configured using
> +your distribution's network configuration tools. See the [wiki][net]
> +for guidance and examples.
> +

The above two paragraphs don't make sense to me. Is that copy-n-paste
error?

Wei.

Re: [PATCH 3/4] Allow vif= to specify PCI address for each nic

[Don Slutz]

On 06/15/15 11:54, Wei Liu wrote:
> On Mon, Jun 15, 2015 at 10:15:51AM -0400, Don Slutz wrote:
>> This allows more then 32 nics.
>>
> 
> How does this patch help? What prevents you from having more than 32
> nics?
> 

Without a way to put the emulated nics onto a PCI to PCI Bridge, you are
limited to 1 PCI bus (the host one).

A PCI bus has a max of 32 PCI devices.

So, since the host bus has some PCI devices that are not nics, the limit
is smaller then 32.


>> This can help with Windows finding nics at boot time.
>>
>> This allows changing config file:
>>
>>     builder = "hvm"
>>     device_model_args_hvm = [
>>      "-device",
>>      "pci-bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0",
>>      "-device",
>>      "vmxnet3,id=nic3,netdev=net3,mac=00:0c:29:86:44:be,bus=pciBridge5.0,addr=0x4.0x0",
>>      "-netdev",
>>      "type=tap,id=net3,ifname=vif.3-emu,script=/etc/qemu-ifup,downscript=no",
>>     ]
>>     vif = [
>>     ]
>>
>> to:
>>
>>     builder = "hvm"
>>     device_model_args_hvm = [
>>      "-device",
>>      "pci-bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0",
>>     ]
>>     vif = [
>>      "model=vmxnet3,bridge=xenbr0,mac=00:0c:29:86:44:a0,bus=pciBridge5.0,addr=0x4.0x0",
>>     ]
>>
>> which enables usage of xen-netback.
>>
> 
> In any case, exposing HVM-only options to top-level vif configuration
> space doesn't look right.


There are already HVM-only options in vifs:

### type

This keyword is valid for HVM guests only.
...
### model

This keyword is valid for HVM guest devices with `type=ioemu` only.
...

> Why do you want to set bus and addr? The
> rationale should be stated in commit message.


That is why I said:

>> This can help with Windows finding nics at boot time.

Windows boot code is not as flexible as Linux.  Most versions of Windows
like to blue screen if the hardware changes enough.

nics as not as important as disks, but some Windows configurations wants
to do network access during boot.  If you are lucky they do not blue
screen if the nic moves.

> 
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> CC: Don Slutz <dslutz@verizon.com>
>> ---
>>  docs/misc/xl-network-configuration.markdown | 14 ++++++++++++++
>>  tools/libxl/libxl_dm.c                      | 23 ++++++++++++++++++++---
>>  tools/libxl/libxl_types.idl                 |  2 ++
>>  tools/libxl/xl_cmdimpl.c                    |  4 ++++
>>  4 files changed, 40 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/misc/xl-network-configuration.markdown b/docs/misc/xl-network-configuration.markdown
>> index 3c439d4..6a7f6db 100644
>> --- a/docs/misc/xl-network-configuration.markdown
>> +++ b/docs/misc/xl-network-configuration.markdown
>> @@ -60,6 +60,20 @@ strategy. Otherwise in general you should prefer to generate a random
>>  MAC and set the locally administered bit since this allows for more
>>  bits of randomness than using the Xen OUI.
>>  
>> +### bus
>> +
>> +Specifies the name of the network bridge which this VIF should be
>> +added to. The default is `xenbr0`. The bridge must be configured using
>> +your distribution's network configuration tools. See the [wiki][net]
>> +for guidance and examples.
>> +
>> +### addr
>> +
>> +Specifies the name of the network bridge which this VIF should be
>> +added to. The default is `xenbr0`. The bridge must be configured using
>> +your distribution's network configuration tools. See the [wiki][net]
>> +for guidance and examples.
>> +
> 
> The above two paragraphs don't make sense to me. Is that copy-n-paste
> error?
> 

Sigh, yes.  Will fix.

   -Don Slutz

> Wei.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 3/4] Allow vif= to specify PCI address for each nic

[Wei Liu]

On Mon, Jun 15, 2015 at 01:45:26PM -0400, Don Slutz wrote:
> On 06/15/15 11:54, Wei Liu wrote:
> > On Mon, Jun 15, 2015 at 10:15:51AM -0400, Don Slutz wrote:
> >> This allows more then 32 nics.
> >>
> > 
> > How does this patch help? What prevents you from having more than 32
> > nics?
> > 
> 
> Without a way to put the emulated nics onto a PCI to PCI Bridge, you are
> limited to 1 PCI bus (the host one).
> 
> A PCI bus has a max of 32 PCI devices.
> 
> So, since the host bus has some PCI devices that are not nics, the limit
> is smaller then 32.
> 

Is there anything that you can't accomplish by using
device_model_args_hvm?

> 
> >> This can help with Windows finding nics at boot time.
> >>
> >> This allows changing config file:
> >>
> >>     builder = "hvm"
> >>     device_model_args_hvm = [
> >>      "-device",
> >>      "pci-bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0",
> >>      "-device",
> >>      "vmxnet3,id=nic3,netdev=net3,mac=00:0c:29:86:44:be,bus=pciBridge5.0,addr=0x4.0x0",
> >>      "-netdev",
> >>      "type=tap,id=net3,ifname=vif.3-emu,script=/etc/qemu-ifup,downscript=no",
> >>     ]
> >>     vif = [
> >>     ]
> >>
> >> to:
> >>
> >>     builder = "hvm"
> >>     device_model_args_hvm = [
> >>      "-device",
> >>      "pci-bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0",
> >>     ]
> >>     vif = [
> >>      "model=vmxnet3,bridge=xenbr0,mac=00:0c:29:86:44:a0,bus=pciBridge5.0,addr=0x4.0x0",
> >>     ]
> >>

The way that you construct PCI bridge via device_model_args_hvm while
plumbing some other information via vif= is fragile and not general
useful to others. I don't think I would accept a half-baked solution
like this.

> >> which enables usage of xen-netback.
> >>
> > 
> > In any case, exposing HVM-only options to top-level vif configuration
> > space doesn't look right.
> 
> 
> There are already HVM-only options in vifs:
> 
> ### type
> 
> This keyword is valid for HVM guests only.
> ...
> ### model
> 
> This keyword is valid for HVM guest devices with `type=ioemu` only.
> ...
> 
> > Why do you want to set bus and addr? The
> > rationale should be stated in commit message.
> 
> 
> That is why I said:
> 
> >> This can help with Windows finding nics at boot time.
> 
> Windows boot code is not as flexible as Linux.  Most versions of Windows
> like to blue screen if the hardware changes enough.
> 

Looks like you're trying to migrate a guest from VMWare to Xen. If
device_model_args_new is sufficient please just use that.

Wei.

Re: [PATCH 3/4] Allow vif= to specify PCI address for each nic

[Don Slutz]

On 06/16/15 06:32, Wei Liu wrote:
> On Mon, Jun 15, 2015 at 01:45:26PM -0400, Don Slutz wrote:
>> On 06/15/15 11:54, Wei Liu wrote:
>>> On Mon, Jun 15, 2015 at 10:15:51AM -0400, Don Slutz wrote:
>>>> This allows more then 32 nics.
>>>>
>>>
>>> How does this patch help? What prevents you from having more than 32
>>> nics?
>>>
>>
>> Without a way to put the emulated nics onto a PCI to PCI Bridge, you are
>> limited to 1 PCI bus (the host one).
>>
>> A PCI bus has a max of 32 PCI devices.
>>
>> So, since the host bus has some PCI devices that are not nics, the limit
>> is smaller then 32.
>>
> 
> Is there anything that you can't accomplish by using
> device_model_args_hvm?
> 

Yes, you cannot use xen-netfront/xen-netback (PV access).

>>
>>>> This can help with Windows finding nics at boot time.
>>>>
>>>> This allows changing config file:
>>>>
>>>>     builder = "hvm"
>>>>     device_model_args_hvm = [
>>>>      "-device",
>>>>      "pci-bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0",
>>>>      "-device",
>>>>      "vmxnet3,id=nic3,netdev=net3,mac=00:0c:29:86:44:be,bus=pciBridge5.0,addr=0x4.0x0",
>>>>      "-netdev",
>>>>      "type=tap,id=net3,ifname=vif.3-emu,script=/etc/qemu-ifup,downscript=no",
>>>>     ]
>>>>     vif = [
>>>>     ]
>>>>
>>>> to:
>>>>
>>>>     builder = "hvm"
>>>>     device_model_args_hvm = [
>>>>      "-device",
>>>>      "pci-bridge,chassis_nr=2,msi=on,id=pciBridge5.0,multifunction=on,addr=0x15.0",
>>>>     ]
>>>>     vif = [
>>>>      "model=vmxnet3,bridge=xenbr0,mac=00:0c:29:86:44:a0,bus=pciBridge5.0,addr=0x4.0x0",
>>>>     ]
>>>>
> 
> The way that you construct PCI bridge via device_model_args_hvm while
> plumbing some other information via vif= is fragile and not general
> useful to others. I don't think I would accept a half-baked solution
> like this.
> 

Ok.  Will look to adding a pcibridge also.

>>>> which enables usage of xen-netback.
>>>>
>>>
>>> In any case, exposing HVM-only options to top-level vif configuration
>>> space doesn't look right.
>>
>>
>> There are already HVM-only options in vifs:
>>
>> ### type
>>
>> This keyword is valid for HVM guests only.
>> ...
>> ### model
>>
>> This keyword is valid for HVM guest devices with `type=ioemu` only.
>> ...
>>
>>> Why do you want to set bus and addr? The
>>> rationale should be stated in commit message.
>>
>>
>> That is why I said:
>>
>>>> This can help with Windows finding nics at boot time.
>>
>> Windows boot code is not as flexible as Linux.  Most versions of Windows
>> like to blue screen if the hardware changes enough.
>>
> 
> Looks like you're trying to migrate a guest from VMWare to Xen. If
> device_model_args_new is sufficient please just use that.
> 

It works, but slowly because you are prevented from using the faster PV
nics.

   -Don Slutz

> Wei.
> 

Re: [PATCH 3/4] Allow vif= to specify PCI address for each nic

[Wei Liu]

On Tue, Jun 16, 2015 at 11:23:46AM -0400, Don Slutz wrote:
[...]
> >>>> which enables usage of xen-netback.
> >>>>
> >>>
> >>> In any case, exposing HVM-only options to top-level vif configuration
> >>> space doesn't look right.
> >>
> >>
> >> There are already HVM-only options in vifs:
> >>
> >> ### type
> >>
> >> This keyword is valid for HVM guests only.
> >> ...
> >> ### model
> >>
> >> This keyword is valid for HVM guest devices with `type=ioemu` only.
> >> ...
> >>
> >>> Why do you want to set bus and addr? The
> >>> rationale should be stated in commit message.
> >>
> >>
> >> That is why I said:
> >>
> >>>> This can help with Windows finding nics at boot time.
> >>
> >> Windows boot code is not as flexible as Linux.  Most versions of Windows
> >> like to blue screen if the hardware changes enough.
> >>
> > 
> > Looks like you're trying to migrate a guest from VMWare to Xen. If
> > device_model_args_new is sufficient please just use that.
> > 
> 
> It works, but slowly because you are prevented from using the faster PV
> nics.
> 

I'm a bit lost here. Aren't you trying to preserve the same nic
configuration for Windows for compatibility reason (even if it's
slower)? Why is PV network involved? How does this work?

Wei.

>    -Don Slutz
> 
> > Wei.
> > 

Re: [PATCH 3/4] Allow vif= to specify PCI address for each nic

[Don Slutz]

On 06/16/15 12:14, Wei Liu wrote:
> On Tue, Jun 16, 2015 at 11:23:46AM -0400, Don Slutz wrote:
> [...]
>>>>>> which enables usage of xen-netback.
>>>>>>
>>>>>
>>>>> In any case, exposing HVM-only options to top-level vif configuration
>>>>> space doesn't look right.
>>>>
>>>>
>>>> There are already HVM-only options in vifs:
>>>>
>>>> ### type
>>>>
>>>> This keyword is valid for HVM guests only.
>>>> ...
>>>> ### model
>>>>
>>>> This keyword is valid for HVM guest devices with `type=ioemu` only.
>>>> ...
>>>>
>>>>> Why do you want to set bus and addr? The
>>>>> rationale should be stated in commit message.
>>>>
>>>>
>>>> That is why I said:
>>>>
>>>>>> This can help with Windows finding nics at boot time.
>>>>
>>>> Windows boot code is not as flexible as Linux.  Most versions of Windows
>>>> like to blue screen if the hardware changes enough.
>>>>
>>>
>>> Looks like you're trying to migrate a guest from VMWare to Xen. If
>>> device_model_args_new is sufficient please just use that.
>>>
>>
>> It works, but slowly because you are prevented from using the faster PV
>> nics.
>>
> 
> I'm a bit lost here. Aren't you trying to preserve the same nic
> configuration for Windows for compatibility reason (even if it's
> slower)? Why is PV network involved? How does this work?
> 

Yes, but there are 2 parts here:

1) boot time
2) run time

At boot time, windows wants devices to be where they were and to be
emulated.

At run time "newer" windows can switch to "Hyper-V enlightened I/O"
which as I understand it is like xen-netfront.  I.E. Windows will start
using xen-netback.

Looking at

http://www.techworld.com.au/article/303867/xen_3_4_0_released_more_client_device_hyper-v_integration/

It says "Also new is support for Microsoft's Hyper-V Enlightened I/O
interface."

And

https://msdn.microsoft.com/en-us/library/cc768521%28v=bts.10%29.aspx

says:

"Presently, both Windows Server 2008 and Windows Vista support Hyper-V
enlightened I/O and a hypervisor aware kernel via installation of
Hyper-V integration services. Integration components, which include VSC
drivers, are also available for other client operating systems."


Also last I knew Citrix does provide Windows drivers that (also?)
provided this faster access.


This may require additional configuration and/or software to be
installed on the windows disk.


I did not see a clear reason to prevent Windows from using the faster
access.

   -Don Slutz



> Wei.
> 
>>    -Don Slutz
>>
>>> Wei.
>>>

Re: [PATCH 3/4] Allow vif= to specify PCI address for each nic

[Wei Liu]

On Tue, Jun 16, 2015 at 03:02:18PM -0400, Don Slutz wrote:
> On 06/16/15 12:14, Wei Liu wrote:
> > On Tue, Jun 16, 2015 at 11:23:46AM -0400, Don Slutz wrote:
> > [...]
> >>>>>> which enables usage of xen-netback.
> >>>>>>
> >>>>>
> >>>>> In any case, exposing HVM-only options to top-level vif configuration
> >>>>> space doesn't look right.
> >>>>
> >>>>
> >>>> There are already HVM-only options in vifs:
> >>>>
> >>>> ### type
> >>>>
> >>>> This keyword is valid for HVM guests only.
> >>>> ...
> >>>> ### model
> >>>>
> >>>> This keyword is valid for HVM guest devices with `type=ioemu` only.
> >>>> ...
> >>>>
> >>>>> Why do you want to set bus and addr? The
> >>>>> rationale should be stated in commit message.
> >>>>
> >>>>
> >>>> That is why I said:
> >>>>
> >>>>>> This can help with Windows finding nics at boot time.
> >>>>
> >>>> Windows boot code is not as flexible as Linux.  Most versions of Windows
> >>>> like to blue screen if the hardware changes enough.
> >>>>
> >>>
> >>> Looks like you're trying to migrate a guest from VMWare to Xen. If
> >>> device_model_args_new is sufficient please just use that.
> >>>
> >>
> >> It works, but slowly because you are prevented from using the faster PV
> >> nics.
> >>
> > 
> > I'm a bit lost here. Aren't you trying to preserve the same nic
> > configuration for Windows for compatibility reason (even if it's
> > slower)? Why is PV network involved? How does this work?
> > 
> 
> Yes, but there are 2 parts here:
> 
> 1) boot time
> 2) run time
> 
> At boot time, windows wants devices to be where they were and to be
> emulated.
> 

This makes sense. And it's what I understood what this patch was about.

> At run time "newer" windows can switch to "Hyper-V enlightened I/O"
> which as I understand it is like xen-netfront.  I.E. Windows will start
> using xen-netback.
> 
> Looking at
> 
> http://www.techworld.com.au/article/303867/xen_3_4_0_released_more_client_device_hyper-v_integration/
> 
> It says "Also new is support for Microsoft's Hyper-V Enlightened I/O
> interface."
> 
> And
> 
> https://msdn.microsoft.com/en-us/library/cc768521%28v=bts.10%29.aspx
> 
> says:
> 
> "Presently, both Windows Server 2008 and Windows Vista support Hyper-V
> enlightened I/O and a hypervisor aware kernel via installation of
> Hyper-V integration services. Integration components, which include VSC
> drivers, are also available for other client operating systems."
> 

Neither article mentioned Xen network driver. I doubt Hyper-V
Enlightened I/O has anything to do with Xen netback.

> 
> Also last I knew Citrix does provide Windows drivers that (also?)
> provided this faster access.
> 

Yes, that's true. I'm not sure it uses the same technique though.

In Linux, netfront writes to a magic IO port to unplug the emulated nic
and switch to using PV path. Does Citrix PV driver also uses the same
trick? Or does it take control from the beginning (i.e. no emulated nic
needed)?

If the answer is "no", that means you need to re-configure the guest
anyway, device_model_args_new should be enough to handle that one time
configuration for migration. If the answer is "yes", see below ...

> 
> This may require additional configuration and/or software to be
> installed on the windows disk.
> 
> 
> I did not see a clear reason to prevent Windows from using the faster
> access.
> 

There's nothing in toolstack that prevents you from doing that. This
reasoning ("enable Windows to use faster path") is irrelevant to this
patch.

The problem is that you need more nics than we can possibly provide,
hence you need to introduce new interface to do that.

So the merit for accepting this patch is only whether the new interfaces
and options are sensible and general useful to others.

Wei.

>    -Don Slutz
> 
> 
> 
> > Wei.
> > 
> >>    -Don Slutz
> >>
> >>> Wei.
> >>>

[PATCH 4/4] Allow disk= to specify their emulated bus address

[Don Slutz]

This allows more disks to be suported via emulation.

This can help with Windows finding disks at boot time.

This allows changing config file:

    builder = "hvm"
    device_model_args_hvm = [
     "-device",
     "pci-bridge,chassis_nr=4,msi=on,id=pciBridge7.0,multifunction=on,addr=0x17.0",
     "-device",
     "pvscsi,id=sas1,bus=pciBridge7.0,addr=0x1.0x0",
     "-drive",
     "if=none,id=disk1-1,format=raw,file=/dev/etherd/e500.2",
     "-device",
     "scsi-disk,vendor=VMware,ver=1.0,product=Virtual disk,bus=sas1.0,scsi-id=1,drive=disk1-1",
    ]
    disk = [
    ]

to:

    builder = "hvm"
    device_model_args_hvm = [
     "-device",
     "pci-bridge,chassis_nr=4,msi=on,id=pciBridge7.0,multifunction=on,addr=0x17.0",
     "-device",
     "pvscsi,id=sas1,bus=pciBridge7.0,addr=0x1.0x0",
    ]
    disk = [
      "vdev=xvdb,bus=sas1.0,target=/dev/etherd/e500.2",
    ]

which allows usage of xen-blkback.

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <dslutz@verizon.com>
---
 docs/misc/xl-disk-configuration.txt |  25 ++++++
 tools/libxl/libxl_dm.c              | 171 ++++++++++++++++++++++++++++--------
 tools/libxl/libxl_types.idl         |   4 +
 tools/libxl/libxlu_disk_l.l         |   4 +
 4 files changed, 167 insertions(+), 37 deletions(-)

diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt
index 6a2118d..99a06b2 100644
--- a/docs/misc/xl-disk-configuration.txt
+++ b/docs/misc/xl-disk-configuration.txt
@@ -178,6 +178,31 @@ information to be interpreted by the executable program <script>,
 These scripts are normally called "block-<script>".
 
 
+bus=<bus>
+---------
+
+Specifies that this disk is also to be placed on an emulated
+controller that is configured in 'device_model_args_hvm'.
+
+
+vendor=<vendor>
+---------
+
+Specifies this scsi disk's Vendor string.
+
+
+ver=<ver>
+---------
+
+Specifies this scsi disk's Rev string.
+
+
+product=<product>
+---------
+
+Specifies this scsi disk's Model string.
+
+
 direct-io-safe
 --------------
 
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 2bf4dab..1e104d8 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -851,8 +851,6 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
             int dev_number =
                 libxl__device_disk_dev_number(disks[i].vdev, &disk, &part);
             const char *format = qemu_disk_format_string(disks[i].format);
-            char *drive;
-            const char *pdev_path;
 
             if (dev_number == -1) {
                 LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "unable to determine"
@@ -860,56 +858,155 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
                 continue;
             }
 
-            if (disks[i].is_cdrom) {
-                if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY)
-                    drive = libxl__sprintf
-                        (gc, "if=ide,index=%d,media=cdrom,cache=writeback,id=ide-%i",
-                         disk, dev_number);
-                else
-                    drive = libxl__sprintf
-                        (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s,cache=writeback,id=ide-%i",
-                         disks[i].pdev_path, disk, format, dev_number);
-            } else {
-                if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
-                    LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "cannot support"
-                               " empty disk format for %s", disks[i].vdev);
-                    continue;
-                }
+            if ((disks[i].bus && disks[i].bus[0]) ||
+                (disks[i].vendor && disks[i].vendor[0]) ||
+                (disks[i].ver && disks[i].ver[0]) ||
+                (disks[i].product && disks[i].product[0])) {
+                char *drive, *device;
+                const char *pdev_path, *bus, *vendor, *ver, *product;
 
-                if (format == NULL) {
-                    LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "unable to determine"
-                               " disk image format %s", disks[i].vdev);
-                    continue;
+                if (disks[i].bus && disks[i].bus[0]) {
+                    bus = disks[i].bus;
+                } else {
+                    bus = "";
+                }
+                if (disks[i].vendor && disks[i].vendor[0]) {
+                    vendor = disks[i].vendor;
+                } else {
+                    vendor = "";
+                }
+                if (disks[i].ver && disks[i].ver[0]) {
+                    ver = disks[i].ver;
+                } else {
+                    ver = "";
+                }
+                if (disks[i].product && disks[i].product[0]) {
+                    product = disks[i].product;
+                } else {
+                    product = "";
                 }
 
-                if (disks[i].backend == LIBXL_DISK_BACKEND_TAP) {
-                    format = qemu_disk_format_string(LIBXL_DISK_FORMAT_RAW);
-                    pdev_path = libxl__blktap_devpath(gc, disks[i].pdev_path,
-                                                      disks[i].format);
+                if (disks[i].is_cdrom) {
+                    if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY)
+                        drive = libxl__sprintf
+                            (gc, "if=none,media=cdrom,cache=writeback,id=disk-%i",
+                             dev_number);
+                    else
+                        drive = libxl__sprintf
+                            (gc, "if=none,media=cdrom,format=%s,cache=writeback,id=disk-%i,file=%s",
+                             format, dev_number, disks[i].pdev_path);
                 } else {
-                    pdev_path = disks[i].pdev_path;
+                    if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
+                        LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "cannot support"
+                                   " empty disk format for %s", disks[i].vdev);
+                        continue;
+                    }
+
+                    if (format == NULL) {
+                        LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "unable to determine"
+                                   " disk image format %s", disks[i].vdev);
+                        continue;
+                    }
+
+                    if (disks[i].backend == LIBXL_DISK_BACKEND_TAP) {
+                        format = qemu_disk_format_string(LIBXL_DISK_FORMAT_RAW);
+                        pdev_path = libxl__blktap_devpath(gc, disks[i].pdev_path,
+                                                          disks[i].format);
+                    } else {
+                        pdev_path = disks[i].pdev_path;
+                    }
+                    drive = libxl__sprintf
+                        (gc, "if=none,format=%s,cache=writeback,id=disk-%i,file=%s",
+                         format, dev_number, pdev_path);
                 }
 
                 /*
-                 * Explicit sd disks are passed through as is.
+                 * Explicit sd and xvd disks are passed through as is.
                  *
                  * For other disks we translate devices 0..3 into
                  * hd[a-d] and ignore the rest.
                  */
-                if (strncmp(disks[i].vdev, "sd", 2) == 0)
-                    drive = libxl__sprintf
-                        (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=writeback",
-                         pdev_path, disk, format);
+                if (strncmp(disks[i].vdev, "sd", 2) == 0 ||
+                    strncmp(disks[i].vdev, "xvd", 3) == 0)
+                    device = libxl__sprintf
+                        (gc, "scsi-hd%s%s%s%s%s%s%s%s,scsi-id=%d,drive=disk-%i",
+                         bus[0] ? ",bus=" : "",
+                         bus,
+                         vendor[0] ? ",vendor=" : "",
+                         vendor,
+                         ver[0] ? ",ver=" : "",
+                         ver,
+                         product[0] ? ",product=" : "",
+                         product,
+                         disk, dev_number);
                 else if (disk < 4)
-                    drive = libxl__sprintf
-                        (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",
-                         pdev_path, disk, format);
+                    device = libxl__sprintf
+                        (gc, "ide-hd%s%s,unit=%d,drive=disk-%i",
+                         ver[0] ? ",ver=" : "",
+                         ver,
+                         disk, dev_number);
                 else
                     continue; /* Do not emulate this disk */
-            }
 
-            flexarray_append(dm_args, "-drive");
-            flexarray_append(dm_args, drive);
+                flexarray_append(dm_args, "-drive");
+                flexarray_append(dm_args, drive);
+                flexarray_append(dm_args, "-device");
+                flexarray_append(dm_args, device);
+            } else {
+                char *drive;
+                const char *pdev_path;
+
+                if (disks[i].is_cdrom) {
+                    if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY)
+                        drive = libxl__sprintf
+                            (gc, "if=ide,index=%d,media=cdrom,cache=writeback,id=ide-%i",
+                             disk, dev_number);
+                    else
+                        drive = libxl__sprintf
+                            (gc, "file=%s,if=ide,index=%d,media=cdrom,format=%s,cache=writeback,id=ide-%i",
+                             disks[i].pdev_path, disk, format, dev_number);
+                } else {
+                    if (disks[i].format == LIBXL_DISK_FORMAT_EMPTY) {
+                        LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "cannot support"
+                                   " empty disk format for %s", disks[i].vdev);
+                        continue;
+                    }
+
+                    if (format == NULL) {
+                        LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "unable to determine"
+                                   " disk image format %s", disks[i].vdev);
+                        continue;
+                    }
+
+                    if (disks[i].backend == LIBXL_DISK_BACKEND_TAP) {
+                        format = qemu_disk_format_string(LIBXL_DISK_FORMAT_RAW);
+                        pdev_path = libxl__blktap_devpath(gc, disks[i].pdev_path,
+                                                          disks[i].format);
+                    } else {
+                        pdev_path = disks[i].pdev_path;
+                    }
+
+                    /*
+                     * Explicit sd disks are passed through as is.
+                     *
+                     * For other disks we translate devices 0..3 into
+                     * hd[a-d] and ignore the rest.
+                     */
+                    if (strncmp(disks[i].vdev, "sd", 2) == 0)
+                        drive = libxl__sprintf
+                            (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=writeback",
+                             pdev_path, disk, format);
+                    else if (disk < 4)
+                        drive = libxl__sprintf
+                            (gc, "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",
+                             pdev_path, disk, format);
+                    else
+                        continue; /* Do not emulate this disk */
+                }
+
+                flexarray_append(dm_args, "-drive");
+                flexarray_append(dm_args, drive);
+            }
         }
 
         switch (b_info->u.hvm.vendor_device) {
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 6d3b058..b2da390 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -507,6 +507,10 @@ libxl_device_disk = Struct("device_disk", [
     ("backend", libxl_disk_backend),
     ("format", libxl_disk_format),
     ("script", string),
+    ("bus", string),
+    ("vendor", string),
+    ("ver", string),
+    ("product", string),
     ("removable", integer),
     ("readwrite", integer),
     ("is_cdrom", integer),
diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index 1a5deb5..95f1769 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -173,6 +173,10 @@ backendtype=[^,]*,? { STRIP(','); setbackendtype(DPC,FROMEQUALS); }
 
 vdev=[^,]*,?	{ STRIP(','); SAVESTRING("vdev", vdev, FROMEQUALS); }
 script=[^,]*,?	{ STRIP(','); SAVESTRING("script", script, FROMEQUALS); }
+bus=[^,]*,?	{ STRIP(','); SAVESTRING("bus", bus, FROMEQUALS); }
+vendor=[^,]*,?	{ STRIP(','); SAVESTRING("vendor", vendor, FROMEQUALS); }
+ver=[^,]*,?	{ STRIP(','); SAVESTRING("ver", ver, FROMEQUALS); }
+product=[^,]*,?	{ STRIP(','); SAVESTRING("product", product, FROMEQUALS); }
 direct-io-safe,? { DPC->disk->direct_io_safe = 1; }
 discard,?	{ libxl_defbool_set(&DPC->disk->discard_enable, true); }
 no-discard,?	{ libxl_defbool_set(&DPC->disk->discard_enable, false); }
-- 
1.8.4