[Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Paul Durrant]

Because of concerns over backwards compatibility and a suggestion that
xenfv should be retired in favour of using the pc machine type I have re-
worked my original patch into 2 patches:

[PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM
[PATCH 2/2] Move hardcoded initialization of xen-platform device.

Application of both these patches allows alternative pc machine types to be
used with the accel=xen option, but preserves the hardcoded creation of
the xen-platform device only for machine type xenfv.

v3:
- Add test for xen_enabled() that went missing in v2

v4:
- Remove erroneous whitespace hunk
- Replace hw_error() with fprintf()+exit(1)
- Add braces to single-line if

[Qemu-devel] [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM domains.

[Paul Durrant]

Xen HVM domains normally spawn QEMU with a dedicated xenfv machine type. The
initialization code for this machine type can easily be pulled into the
generic pc initialization code and guarded with a test for whether the xen
accelerator options is specified, which is more consistent with the way
other accelerators are used.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 hw/i386/pc_piix.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index d618570..f96e0c2 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -91,6 +91,11 @@ static void pc_init1(MemoryRegion *system_memory,
     DeviceState *icc_bridge;
     FWCfgState *fw_cfg = NULL;
 
+    if (xen_enabled() && xen_hvm_init() != 0) {
+        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
+        exit(1);
+    }
+
     icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
     object_property_add_child(qdev_get_machine(), "icc-bridge",
                               OBJECT(icc_bridge), NULL);
@@ -320,9 +325,6 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
 #ifdef CONFIG_XEN
 static void pc_xen_hvm_init(QEMUMachineInitArgs *args)
 {
-    if (xen_hvm_init() != 0) {
-        hw_error("xen hardware virtual machine initialisation failed");
-    }
     pc_init_pci(args);
 }
 #endif
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/2] Move hardcoded initialization of xen-platform device.

[Paul Durrant]

Creation of the xen-platform device is currently hardcoded into machine
type pc's initialization code, guarded by a test for the whether the xen
accelerator is enabled. This patch moves the creation of xen-platform into
the initialization code of the xenfv machine type. This maintains backwards
compatibility for that machine type but allows more flexibility if another
machine type is used with Xen HVM domains.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
 hw/i386/pc_piix.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index f96e0c2..c294138 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -179,9 +179,6 @@ static void pc_init1(MemoryRegion *system_memory,
     pc_register_ferr_irq(gsi[13]);
 
     pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
-    if (xen_enabled()) {
-        pci_create_simple(pci_bus, -1, "xen-platform");
-    }
 
     /* init basic PC hardware */
     pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled());
@@ -325,7 +322,14 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
 #ifdef CONFIG_XEN
 static void pc_xen_hvm_init(QEMUMachineInitArgs *args)
 {
+    PCIBus *bus;
+
     pc_init_pci(args);
+
+    bus = pci_find_root_bus(0);
+    if (bus != NULL) {
+        pci_create_simple(bus, -1, "xen-platform");
+    }
 }
 #endif
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Paolo Bonzini]

Il 18/06/2013 13:16, Paul Durrant ha scritto:
> Because of concerns over backwards compatibility and a suggestion that
> xenfv should be retired in favour of using the pc machine type I have re-
> worked my original patch into 2 patches:
> 
> [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM
> [PATCH 2/2] Move hardcoded initialization of xen-platform device.
> 
> Application of both these patches allows alternative pc machine types to be
> used with the accel=xen option, but preserves the hardcoded creation of
> the xen-platform device only for machine type xenfv.

Perfect!

Paolo

> v3:
> - Add test for xen_enabled() that went missing in v2
> 
> v4:
> - Remove erroneous whitespace hunk
> - Replace hw_error() with fprintf()+exit(1)
> - Add braces to single-line if
> 
> 

Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Laszlo Ersek]

Hi Paul,

(xen-devel snipped)

On 06/18/13 13:16, Paul Durrant wrote:
> Because of concerns over backwards compatibility and a suggestion that
> xenfv should be retired in favour of using the pc machine type I have re-
> worked my original patch into 2 patches:
> 
> [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM
> [PATCH 2/2] Move hardcoded initialization of xen-platform device.
> 
> Application of both these patches allows alternative pc machine types to be
> used with the accel=xen option, but preserves the hardcoded creation of
> the xen-platform device only for machine type xenfv.
> 
> v3:
> - Add test for xen_enabled() that went missing in v2
> 
> v4:
> - Remove erroneous whitespace hunk
> - Replace hw_error() with fprintf()+exit(1)
> - Add braces to single-line if

can you please offer an opinion in the

  [PATCH 1/2] pvpanic: initialization cleanup
  http://thread.gmane.org/gmane.comp.emulators.qemu/216940

thread?

>From where I stand (which is "quite afar" :)) this series of yours seems
somewhat related to my doubt there.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Michael S. Tsirkin]

On Tue, Jun 18, 2013 at 02:37:58PM +0200, Laszlo Ersek wrote:
> Hi Paul,
> 
> (xen-devel snipped)
> 
> On 06/18/13 13:16, Paul Durrant wrote:
> > Because of concerns over backwards compatibility and a suggestion that
> > xenfv should be retired in favour of using the pc machine type I have re-
> > worked my original patch into 2 patches:
> > 
> > [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM
> > [PATCH 2/2] Move hardcoded initialization of xen-platform device.
> > 
> > Application of both these patches allows alternative pc machine types to be
> > used with the accel=xen option, but preserves the hardcoded creation of
> > the xen-platform device only for machine type xenfv.
> > 
> > v3:
> > - Add test for xen_enabled() that went missing in v2
> > 
> > v4:
> > - Remove erroneous whitespace hunk
> > - Replace hw_error() with fprintf()+exit(1)
> > - Add braces to single-line if
> 
> can you please offer an opinion in the
> 
>   [PATCH 1/2] pvpanic: initialization cleanup
>   http://thread.gmane.org/gmane.comp.emulators.qemu/216940
> 
> thread?
> 
> >From where I stand (which is "quite afar" :)) this series of yours seems
> somewhat related to my doubt there.
> 
> Thanks!
> Laszlo

OK will make it skip fwcfg as we did earlier.
Thanks for the review.

-- 
MST

Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Paul Durrant]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: 18 June 2013 13:52
> To: Laszlo Ersek
> Cc: Paul Durrant; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform
> device initialization (v4)
> 
> On Tue, Jun 18, 2013 at 02:37:58PM +0200, Laszlo Ersek wrote:
> > Hi Paul,
> >
> > (xen-devel snipped)
> >
> > On 06/18/13 13:16, Paul Durrant wrote:
> > > Because of concerns over backwards compatibility and a suggestion that
> > > xenfv should be retired in favour of using the pc machine type I have re-
> > > worked my original patch into 2 patches:
> > >
> > > [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM
> > > [PATCH 2/2] Move hardcoded initialization of xen-platform device.
> > >
> > > Application of both these patches allows alternative pc machine types to
> be
> > > used with the accel=xen option, but preserves the hardcoded creation of
> > > the xen-platform device only for machine type xenfv.
> > >
> > > v3:
> > > - Add test for xen_enabled() that went missing in v2
> > >
> > > v4:
> > > - Remove erroneous whitespace hunk
> > > - Replace hw_error() with fprintf()+exit(1)
> > > - Add braces to single-line if
> >
> > can you please offer an opinion in the
> >
> >   [PATCH 1/2] pvpanic: initialization cleanup
> >   http://thread.gmane.org/gmane.comp.emulators.qemu/216940
> >
> > thread?
> >
> > >From where I stand (which is "quite afar" :)) this series of yours seems
> > somewhat related to my doubt there.
> >
> > Thanks!
> > Laszlo
> 
> OK will make it skip fwcfg as we did earlier.
> Thanks for the review.
> 

Yes, I think the assert(fw_cfg) would be problematic in the xen case where, up until my patch, machine types was necessarily xenfv.

  Paul

Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Michael S. Tsirkin]

On Tue, Jun 18, 2013 at 12:57:54PM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > Sent: 18 June 2013 13:52
> > To: Laszlo Ersek
> > Cc: Paul Durrant; qemu-devel@nongnu.org
> > Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform
> > device initialization (v4)
> > 
> > On Tue, Jun 18, 2013 at 02:37:58PM +0200, Laszlo Ersek wrote:
> > > Hi Paul,
> > >
> > > (xen-devel snipped)
> > >
> > > On 06/18/13 13:16, Paul Durrant wrote:
> > > > Because of concerns over backwards compatibility and a suggestion that
> > > > xenfv should be retired in favour of using the pc machine type I have re-
> > > > worked my original patch into 2 patches:
> > > >
> > > > [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM
> > > > [PATCH 2/2] Move hardcoded initialization of xen-platform device.
> > > >
> > > > Application of both these patches allows alternative pc machine types to
> > be
> > > > used with the accel=xen option, but preserves the hardcoded creation of
> > > > the xen-platform device only for machine type xenfv.
> > > >
> > > > v3:
> > > > - Add test for xen_enabled() that went missing in v2
> > > >
> > > > v4:
> > > > - Remove erroneous whitespace hunk
> > > > - Replace hw_error() with fprintf()+exit(1)
> > > > - Add braces to single-line if
> > >
> > > can you please offer an opinion in the
> > >
> > >   [PATCH 1/2] pvpanic: initialization cleanup
> > >   http://thread.gmane.org/gmane.comp.emulators.qemu/216940
> > >
> > > thread?
> > >
> > > >From where I stand (which is "quite afar" :)) this series of yours seems
> > > somewhat related to my doubt there.
> > >
> > > Thanks!
> > > Laszlo
> > 
> > OK will make it skip fwcfg as we did earlier.
> > Thanks for the review.
> > 
> 
> Yes, I think the assert(fw_cfg) would be problematic in the xen case where, up until my patch, machine types was necessarily xenfv.
> 
>   Paul

Do you guys actually need the pvpanic device?
How do you know which port to use without fwcfg?

-- 
MST

Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Paul Durrant]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: 18 June 2013 14:02
> To: Paul Durrant
> Cc: Laszlo Ersek; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform
> device initialization (v4)
> 
> On Tue, Jun 18, 2013 at 12:57:54PM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > > Sent: 18 June 2013 13:52
> > > To: Laszlo Ersek
> > > Cc: Paul Durrant; qemu-devel@nongnu.org
> > > Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform
> > > device initialization (v4)
> > >
> > > On Tue, Jun 18, 2013 at 02:37:58PM +0200, Laszlo Ersek wrote:
> > > > Hi Paul,
> > > >
> > > > (xen-devel snipped)
> > > >
> > > > On 06/18/13 13:16, Paul Durrant wrote:
> > > > > Because of concerns over backwards compatibility and a suggestion
> that
> > > > > xenfv should be retired in favour of using the pc machine type I have
> re-
> > > > > worked my original patch into 2 patches:
> > > > >
> > > > > [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM
> > > > > [PATCH 2/2] Move hardcoded initialization of xen-platform device.
> > > > >
> > > > > Application of both these patches allows alternative pc machine types
> to
> > > be
> > > > > used with the accel=xen option, but preserves the hardcoded
> creation of
> > > > > the xen-platform device only for machine type xenfv.
> > > > >
> > > > > v3:
> > > > > - Add test for xen_enabled() that went missing in v2
> > > > >
> > > > > v4:
> > > > > - Remove erroneous whitespace hunk
> > > > > - Replace hw_error() with fprintf()+exit(1)
> > > > > - Add braces to single-line if
> > > >
> > > > can you please offer an opinion in the
> > > >
> > > >   [PATCH 1/2] pvpanic: initialization cleanup
> > > >   http://thread.gmane.org/gmane.comp.emulators.qemu/216940
> > > >
> > > > thread?
> > > >
> > > > >From where I stand (which is "quite afar" :)) this series of yours seems
> > > > somewhat related to my doubt there.
> > > >
> > > > Thanks!
> > > > Laszlo
> > >
> > > OK will make it skip fwcfg as we did earlier.
> > > Thanks for the review.
> > >
> >
> > Yes, I think the assert(fw_cfg) would be problematic in the xen case where,
> up until my patch, machine types was necessarily xenfv.
> >
> >   Paul
> 
> Do you guys actually need the pvpanic device?

I don't believe anyone uses it. You should probably ask on xen-devel to make sure though.

  Paul

Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Laszlo Ersek]

On 06/18/13 15:01, Michael S. Tsirkin wrote:
> On Tue, Jun 18, 2013 at 12:57:54PM +0000, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Michael S. Tsirkin [mailto:mst@redhat.com]
>>> Sent: 18 June 2013 13:52
>>> To: Laszlo Ersek
>>> Cc: Paul Durrant; qemu-devel@nongnu.org
>>> Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform
>>> device initialization (v4)
>>>
>>> On Tue, Jun 18, 2013 at 02:37:58PM +0200, Laszlo Ersek wrote:
>>>> Hi Paul,
>>>>
>>>> (xen-devel snipped)
>>>>
>>>> On 06/18/13 13:16, Paul Durrant wrote:
>>>>> Because of concerns over backwards compatibility and a suggestion that
>>>>> xenfv should be retired in favour of using the pc machine type I have re-
>>>>> worked my original patch into 2 patches:
>>>>>
>>>>> [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM
>>>>> [PATCH 2/2] Move hardcoded initialization of xen-platform device.
>>>>>
>>>>> Application of both these patches allows alternative pc machine types to
>>> be
>>>>> used with the accel=xen option, but preserves the hardcoded creation of
>>>>> the xen-platform device only for machine type xenfv.
>>>>>
>>>>> v3:
>>>>> - Add test for xen_enabled() that went missing in v2
>>>>>
>>>>> v4:
>>>>> - Remove erroneous whitespace hunk
>>>>> - Replace hw_error() with fprintf()+exit(1)
>>>>> - Add braces to single-line if
>>>>
>>>> can you please offer an opinion in the
>>>>
>>>>   [PATCH 1/2] pvpanic: initialization cleanup
>>>>   http://thread.gmane.org/gmane.comp.emulators.qemu/216940
>>>>
>>>> thread?
>>>>
>>>> >From where I stand (which is "quite afar" :)) this series of yours seems
>>>> somewhat related to my doubt there.
>>>>
>>>> Thanks!
>>>> Laszlo
>>>
>>> OK will make it skip fwcfg as we did earlier.
>>> Thanks for the review.
>>>
>>
>> Yes, I think the assert(fw_cfg) would be problematic in the xen case where, up until my patch, machine types was necessarily xenfv.
>>
>>   Paul
> 
> Do you guys actually need the pvpanic device?
> How do you know which port to use without fwcfg?

Xen domains don't know the port and don't use the pvpanic device, but
qemu starts at least. In other words, the pvpanic device is created, but
unreachable. Maybe the has_pvpanic logic should depend on (or extended
with) !xen_enabled().

Just a guess.

Laszlo

Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Paul Durrant]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: 18 June 2013 14:14
> To: Michael S. Tsirkin
> Cc: Paul Durrant; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform
> device initialization (v4)
> 
> On 06/18/13 15:01, Michael S. Tsirkin wrote:
> > On Tue, Jun 18, 2013 at 12:57:54PM +0000, Paul Durrant wrote:
> >>> -----Original Message-----
> >>> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> >>> Sent: 18 June 2013 13:52
> >>> To: Laszlo Ersek
> >>> Cc: Paul Durrant; qemu-devel@nongnu.org
> >>> Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-
> platform
> >>> device initialization (v4)
> >>>
> >>> On Tue, Jun 18, 2013 at 02:37:58PM +0200, Laszlo Ersek wrote:
> >>>> Hi Paul,
> >>>>
> >>>> (xen-devel snipped)
> >>>>
> >>>> On 06/18/13 13:16, Paul Durrant wrote:
> >>>>> Because of concerns over backwards compatibility and a suggestion
> that
> >>>>> xenfv should be retired in favour of using the pc machine type I have
> re-
> >>>>> worked my original patch into 2 patches:
> >>>>>
> >>>>> [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM
> >>>>> [PATCH 2/2] Move hardcoded initialization of xen-platform device.
> >>>>>
> >>>>> Application of both these patches allows alternative pc machine types
> to
> >>> be
> >>>>> used with the accel=xen option, but preserves the hardcoded
> creation of
> >>>>> the xen-platform device only for machine type xenfv.
> >>>>>
> >>>>> v3:
> >>>>> - Add test for xen_enabled() that went missing in v2
> >>>>>
> >>>>> v4:
> >>>>> - Remove erroneous whitespace hunk
> >>>>> - Replace hw_error() with fprintf()+exit(1)
> >>>>> - Add braces to single-line if
> >>>>
> >>>> can you please offer an opinion in the
> >>>>
> >>>>   [PATCH 1/2] pvpanic: initialization cleanup
> >>>>   http://thread.gmane.org/gmane.comp.emulators.qemu/216940
> >>>>
> >>>> thread?
> >>>>
> >>>> >From where I stand (which is "quite afar" :)) this series of yours seems
> >>>> somewhat related to my doubt there.
> >>>>
> >>>> Thanks!
> >>>> Laszlo
> >>>
> >>> OK will make it skip fwcfg as we did earlier.
> >>> Thanks for the review.
> >>>
> >>
> >> Yes, I think the assert(fw_cfg) would be problematic in the xen case
> where, up until my patch, machine types was necessarily xenfv.
> >>
> >>   Paul
> >
> > Do you guys actually need the pvpanic device?
> > How do you know which port to use without fwcfg?
> 
> Xen domains don't know the port and don't use the pvpanic device, but
> qemu starts at least. In other words, the pvpanic device is created, but
> unreachable. Maybe the has_pvpanic logic should depend on (or extended
> with) !xen_enabled().
> 

That seems entirely reasonable to me.

  Paul

Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Michael S. Tsirkin]

On Tue, Jun 18, 2013 at 01:15:57PM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: 18 June 2013 14:14
> > To: Michael S. Tsirkin
> > Cc: Paul Durrant; qemu-devel@nongnu.org
> > Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform
> > device initialization (v4)
> > 
> > On 06/18/13 15:01, Michael S. Tsirkin wrote:
> > > On Tue, Jun 18, 2013 at 12:57:54PM +0000, Paul Durrant wrote:
> > >>> -----Original Message-----
> > >>> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > >>> Sent: 18 June 2013 13:52
> > >>> To: Laszlo Ersek
> > >>> Cc: Paul Durrant; qemu-devel@nongnu.org
> > >>> Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-
> > platform
> > >>> device initialization (v4)
> > >>>
> > >>> On Tue, Jun 18, 2013 at 02:37:58PM +0200, Laszlo Ersek wrote:
> > >>>> Hi Paul,
> > >>>>
> > >>>> (xen-devel snipped)
> > >>>>
> > >>>> On 06/18/13 13:16, Paul Durrant wrote:
> > >>>>> Because of concerns over backwards compatibility and a suggestion
> > that
> > >>>>> xenfv should be retired in favour of using the pc machine type I have
> > re-
> > >>>>> worked my original patch into 2 patches:
> > >>>>>
> > >>>>> [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM
> > >>>>> [PATCH 2/2] Move hardcoded initialization of xen-platform device.
> > >>>>>
> > >>>>> Application of both these patches allows alternative pc machine types
> > to
> > >>> be
> > >>>>> used with the accel=xen option, but preserves the hardcoded
> > creation of
> > >>>>> the xen-platform device only for machine type xenfv.
> > >>>>>
> > >>>>> v3:
> > >>>>> - Add test for xen_enabled() that went missing in v2
> > >>>>>
> > >>>>> v4:
> > >>>>> - Remove erroneous whitespace hunk
> > >>>>> - Replace hw_error() with fprintf()+exit(1)
> > >>>>> - Add braces to single-line if
> > >>>>
> > >>>> can you please offer an opinion in the
> > >>>>
> > >>>>   [PATCH 1/2] pvpanic: initialization cleanup
> > >>>>   http://thread.gmane.org/gmane.comp.emulators.qemu/216940
> > >>>>
> > >>>> thread?
> > >>>>
> > >>>> >From where I stand (which is "quite afar" :)) this series of yours seems
> > >>>> somewhat related to my doubt there.
> > >>>>
> > >>>> Thanks!
> > >>>> Laszlo
> > >>>
> > >>> OK will make it skip fwcfg as we did earlier.
> > >>> Thanks for the review.
> > >>>
> > >>
> > >> Yes, I think the assert(fw_cfg) would be problematic in the xen case
> > where, up until my patch, machine types was necessarily xenfv.
> > >>
> > >>   Paul
> > >
> > > Do you guys actually need the pvpanic device?
> > > How do you know which port to use without fwcfg?
> > 
> > Xen domains don't know the port and don't use the pvpanic device, but
> > qemu starts at least. In other words, the pvpanic device is created, but
> > unreachable. Maybe the has_pvpanic logic should depend on (or extended
> > with) !xen_enabled().
> > 
> 
> That seems entirely reasonable to me.
> 
>   Paul

We can just skip creating the device if there's no fw cfg.

Re: [Qemu-devel] [Xen-devel] [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM domains.

[Stefano Stabellini]

On Tue, 18 Jun 2013, Paul Durrant wrote:
> Xen HVM domains normally spawn QEMU with a dedicated xenfv machine type. The
> initialization code for this machine type can easily be pulled into the
> generic pc initialization code and guarded with a test for whether the xen
> accelerator options is specified, which is more consistent with the way
> other accelerators are used.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  hw/i386/pc_piix.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index d618570..f96e0c2 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -91,6 +91,11 @@ static void pc_init1(MemoryRegion *system_memory,
>      DeviceState *icc_bridge;
>      FWCfgState *fw_cfg = NULL;
>  
> +    if (xen_enabled() && xen_hvm_init() != 0) {
> +        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
> +        exit(1);
> +    }
> +
>      icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
>      object_property_add_child(qdev_get_machine(), "icc-bridge",
>                                OBJECT(icc_bridge), NULL);
> @@ -320,9 +325,6 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
>  #ifdef CONFIG_XEN
>  static void pc_xen_hvm_init(QEMUMachineInitArgs *args)
>  {
> -    if (xen_hvm_init() != 0) {
> -        hw_error("xen hardware virtual machine initialisation failed");
> -    }
>      pc_init_pci(args);
>  }
>  #endif
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] Move hardcoded initialization of xen-platform device.

[Stefano Stabellini]

On Tue, 18 Jun 2013, Paul Durrant wrote:
> Creation of the xen-platform device is currently hardcoded into machine
> type pc's initialization code, guarded by a test for the whether the xen
> accelerator is enabled. This patch moves the creation of xen-platform into
> the initialization code of the xenfv machine type. This maintains backwards
> compatibility for that machine type but allows more flexibility if another
> machine type is used with Xen HVM domains.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  hw/i386/pc_piix.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index f96e0c2..c294138 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -179,9 +179,6 @@ static void pc_init1(MemoryRegion *system_memory,
>      pc_register_ferr_irq(gsi[13]);
>  
>      pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
> -    if (xen_enabled()) {
> -        pci_create_simple(pci_bus, -1, "xen-platform");
> -    }
>  
>      /* init basic PC hardware */
>      pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled());
> @@ -325,7 +322,14 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
>  #ifdef CONFIG_XEN
>  static void pc_xen_hvm_init(QEMUMachineInitArgs *args)
>  {
> +    PCIBus *bus;
> +
>      pc_init_pci(args);
> +
> +    bus = pci_find_root_bus(0);
> +    if (bus != NULL) {
> +        pci_create_simple(bus, -1, "xen-platform");
> +    }
>  }
>  #endif

Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Paolo Bonzini]

Il 18/06/2013 16:22, Michael S. Tsirkin ha scritto:
> On Tue, Jun 18, 2013 at 01:15:57PM +0000, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: 18 June 2013 14:14
>>> To: Michael S. Tsirkin
>>> Cc: Paul Durrant; qemu-devel@nongnu.org
>>> Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform
>>> device initialization (v4)
>>>
>>> On 06/18/13 15:01, Michael S. Tsirkin wrote:
>>>> On Tue, Jun 18, 2013 at 12:57:54PM +0000, Paul Durrant wrote:
>>>>>> -----Original Message-----
>>>>>> From: Michael S. Tsirkin [mailto:mst@redhat.com]
>>>>>> Sent: 18 June 2013 13:52
>>>>>> To: Laszlo Ersek
>>>>>> Cc: Paul Durrant; qemu-devel@nongnu.org
>>>>>> Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-
>>> platform
>>>>>> device initialization (v4)
>>>>>>
>>>>>> On Tue, Jun 18, 2013 at 02:37:58PM +0200, Laszlo Ersek wrote:
>>>>>>> Hi Paul,
>>>>>>>
>>>>>>> (xen-devel snipped)
>>>>>>>
>>>>>>> On 06/18/13 13:16, Paul Durrant wrote:
>>>>>>>> Because of concerns over backwards compatibility and a suggestion
>>> that
>>>>>>>> xenfv should be retired in favour of using the pc machine type I have
>>> re-
>>>>>>>> worked my original patch into 2 patches:
>>>>>>>>
>>>>>>>> [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM
>>>>>>>> [PATCH 2/2] Move hardcoded initialization of xen-platform device.
>>>>>>>>
>>>>>>>> Application of both these patches allows alternative pc machine types
>>> to
>>>>>> be
>>>>>>>> used with the accel=xen option, but preserves the hardcoded
>>> creation of
>>>>>>>> the xen-platform device only for machine type xenfv.
>>>>>>>>
>>>>>>>> v3:
>>>>>>>> - Add test for xen_enabled() that went missing in v2
>>>>>>>>
>>>>>>>> v4:
>>>>>>>> - Remove erroneous whitespace hunk
>>>>>>>> - Replace hw_error() with fprintf()+exit(1)
>>>>>>>> - Add braces to single-line if
>>>>>>>
>>>>>>> can you please offer an opinion in the
>>>>>>>
>>>>>>>   [PATCH 1/2] pvpanic: initialization cleanup
>>>>>>>   http://thread.gmane.org/gmane.comp.emulators.qemu/216940
>>>>>>>
>>>>>>> thread?
>>>>>>>
>>>>>>> >From where I stand (which is "quite afar" :)) this series of yours seems
>>>>>>> somewhat related to my doubt there.
>>>>>>>
>>>>>>> Thanks!
>>>>>>> Laszlo
>>>>>>
>>>>>> OK will make it skip fwcfg as we did earlier.
>>>>>> Thanks for the review.
>>>>>>
>>>>>
>>>>> Yes, I think the assert(fw_cfg) would be problematic in the xen case
>>> where, up until my patch, machine types was necessarily xenfv.
>>>>>
>>>>>   Paul
>>>>
>>>> Do you guys actually need the pvpanic device?
>>>> How do you know which port to use without fwcfg?
>>>
>>> Xen domains don't know the port and don't use the pvpanic device, but
>>> qemu starts at least. In other words, the pvpanic device is created, but
>>> unreachable. Maybe the has_pvpanic logic should depend on (or extended
>>> with) !xen_enabled().
>>>
>>
>> That seems entirely reasonable to me.
> 
> We can just skip creating the device if there's no fw cfg.

No, in principle Xen domains could use another scheme to find the port
(xenstore for example).  If Xen domains do not want it, they can just
add an "if".  Or we could just skip the fw_cfg step.  The device will be
there but not ACPI-discoverable.

Paolo

Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform device initialization (v4)

[Michael S. Tsirkin]

On Thu, Jun 20, 2013 at 05:02:56PM +0200, Paolo Bonzini wrote:
> Il 18/06/2013 16:22, Michael S. Tsirkin ha scritto:
> > On Tue, Jun 18, 2013 at 01:15:57PM +0000, Paul Durrant wrote:
> >>> -----Original Message-----
> >>> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >>> Sent: 18 June 2013 14:14
> >>> To: Michael S. Tsirkin
> >>> Cc: Paul Durrant; qemu-devel@nongnu.org
> >>> Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-platform
> >>> device initialization (v4)
> >>>
> >>> On 06/18/13 15:01, Michael S. Tsirkin wrote:
> >>>> On Tue, Jun 18, 2013 at 12:57:54PM +0000, Paul Durrant wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> >>>>>> Sent: 18 June 2013 13:52
> >>>>>> To: Laszlo Ersek
> >>>>>> Cc: Paul Durrant; qemu-devel@nongnu.org
> >>>>>> Subject: Re: [Qemu-devel] [PATCH 0/2] Remove hardcoded xen-
> >>> platform
> >>>>>> device initialization (v4)
> >>>>>>
> >>>>>> On Tue, Jun 18, 2013 at 02:37:58PM +0200, Laszlo Ersek wrote:
> >>>>>>> Hi Paul,
> >>>>>>>
> >>>>>>> (xen-devel snipped)
> >>>>>>>
> >>>>>>> On 06/18/13 13:16, Paul Durrant wrote:
> >>>>>>>> Because of concerns over backwards compatibility and a suggestion
> >>> that
> >>>>>>>> xenfv should be retired in favour of using the pc machine type I have
> >>> re-
> >>>>>>>> worked my original patch into 2 patches:
> >>>>>>>>
> >>>>>>>> [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM
> >>>>>>>> [PATCH 2/2] Move hardcoded initialization of xen-platform device.
> >>>>>>>>
> >>>>>>>> Application of both these patches allows alternative pc machine types
> >>> to
> >>>>>> be
> >>>>>>>> used with the accel=xen option, but preserves the hardcoded
> >>> creation of
> >>>>>>>> the xen-platform device only for machine type xenfv.
> >>>>>>>>
> >>>>>>>> v3:
> >>>>>>>> - Add test for xen_enabled() that went missing in v2
> >>>>>>>>
> >>>>>>>> v4:
> >>>>>>>> - Remove erroneous whitespace hunk
> >>>>>>>> - Replace hw_error() with fprintf()+exit(1)
> >>>>>>>> - Add braces to single-line if
> >>>>>>>
> >>>>>>> can you please offer an opinion in the
> >>>>>>>
> >>>>>>>   [PATCH 1/2] pvpanic: initialization cleanup
> >>>>>>>   http://thread.gmane.org/gmane.comp.emulators.qemu/216940
> >>>>>>>
> >>>>>>> thread?
> >>>>>>>
> >>>>>>> >From where I stand (which is "quite afar" :)) this series of yours seems
> >>>>>>> somewhat related to my doubt there.
> >>>>>>>
> >>>>>>> Thanks!
> >>>>>>> Laszlo
> >>>>>>
> >>>>>> OK will make it skip fwcfg as we did earlier.
> >>>>>> Thanks for the review.
> >>>>>>
> >>>>>
> >>>>> Yes, I think the assert(fw_cfg) would be problematic in the xen case
> >>> where, up until my patch, machine types was necessarily xenfv.
> >>>>>
> >>>>>   Paul
> >>>>
> >>>> Do you guys actually need the pvpanic device?
> >>>> How do you know which port to use without fwcfg?
> >>>
> >>> Xen domains don't know the port and don't use the pvpanic device, but
> >>> qemu starts at least. In other words, the pvpanic device is created, but
> >>> unreachable. Maybe the has_pvpanic logic should depend on (or extended
> >>> with) !xen_enabled().
> >>>
> >>
> >> That seems entirely reasonable to me.
> > 
> > We can just skip creating the device if there's no fw cfg.
> 
> No, in principle Xen domains could use another scheme to find the port
> (xenstore for example).  If Xen domains do not want it, they can just
> add an "if".  Or we could just skip the fw_cfg step.  The device will be
> there but not ACPI-discoverable.
> 
> Paolo

That's a good reason to hide it.  We don't want multiple
mechanisms to discover the same feature, it's a
maintainance headache.

For example, if we do this guests might try to use these ports for something
else by mistake. Or they will start hard-coding the port,
then it will break if user tweaks it from command line.
Simply put, if xen wants to use features exposed through fw cfg,
let's add fw cfg support for xen and use let xen use fw cfg to discover
them.

-- 
MST

Re: [Qemu-devel] [Xen-devel] [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM domains.

[Stefano Stabellini]

On Tue, 18 Jun 2013, Paul Durrant wrote:
> Xen HVM domains normally spawn QEMU with a dedicated xenfv machine type. The
> initialization code for this machine type can easily be pulled into the
> generic pc initialization code and guarded with a test for whether the xen
> accelerator options is specified, which is more consistent with the way
> other accelerators are used.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
>  hw/i386/pc_piix.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index d618570..f96e0c2 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -91,6 +91,11 @@ static void pc_init1(MemoryRegion *system_memory,
>      DeviceState *icc_bridge;
>      FWCfgState *fw_cfg = NULL;
>  
> +    if (xen_enabled() && xen_hvm_init() != 0) {
> +        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
> +        exit(1);
> +    }
> +
>      icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
>      object_property_add_child(qdev_get_machine(), "icc-bridge",
>                                OBJECT(icc_bridge), NULL);

I was about to submit a pull request with this patch, but unfortunately
it breaks non-xen compilations:

hw/i386/pc_piix.o: In function `pc_init1':
/local/scratch/sstabellini/qemu/hw/i386/pc_piix.c:94: undefined reference to `xen_hvm_init'

you need to add the function to xen-stub.c:

diff --git a/xen-stub.c b/xen-stub.c
index 6f0516a..47c8e73 100644
--- a/xen-stub.c
+++ b/xen-stub.c
@@ -63,3 +63,8 @@ void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
 void xen_modified_memory(ram_addr_t start, ram_addr_t length)
 {
 }
+
+int xen_hvm_init(void)
+{
+    return 0;
+}

I'll include this change in the pull request

Re: [Qemu-devel] [Xen-devel] [PATCH 1/2] Allow use of pc machine type (accel=xen) for Xen HVM domains.

[Paul Durrant]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: 25 June 2013 13:02
> To: Paul Durrant
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH 1/2] Allow use of pc machine type
> (accel=xen) for Xen HVM domains.
> 
> On Tue, 18 Jun 2013, Paul Durrant wrote:
> > Xen HVM domains normally spawn QEMU with a dedicated xenfv machine
> type. The
> > initialization code for this machine type can easily be pulled into the
> > generic pc initialization code and guarded with a test for whether the xen
> > accelerator options is specified, which is more consistent with the way
> > other accelerators are used.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> >  hw/i386/pc_piix.c |    8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index d618570..f96e0c2 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -91,6 +91,11 @@ static void pc_init1(MemoryRegion
> *system_memory,
> >      DeviceState *icc_bridge;
> >      FWCfgState *fw_cfg = NULL;
> >
> > +    if (xen_enabled() && xen_hvm_init() != 0) {
> > +        fprintf(stderr, "xen hardware virtual machine initialisation failed\n");
> > +        exit(1);
> > +    }
> > +
> >      icc_bridge = qdev_create(NULL, TYPE_ICC_BRIDGE);
> >      object_property_add_child(qdev_get_machine(), "icc-bridge",
> >                                OBJECT(icc_bridge), NULL);
> 
> I was about to submit a pull request with this patch, but unfortunately
> it breaks non-xen compilations:
> 
> hw/i386/pc_piix.o: In function `pc_init1':
> /local/scratch/sstabellini/qemu/hw/i386/pc_piix.c:94: undefined reference
> to `xen_hvm_init'
> 
> you need to add the function to xen-stub.c:
> 
> diff --git a/xen-stub.c b/xen-stub.c
> index 6f0516a..47c8e73 100644
> --- a/xen-stub.c
> +++ b/xen-stub.c
> @@ -63,3 +63,8 @@ void qmp_xen_set_global_dirty_log(bool enable, Error
> **errp)
>  void xen_modified_memory(ram_addr_t start, ram_addr_t length)
>  {
>  }
> +
> +int xen_hvm_init(void)
> +{
> +    return 0;
> +}
> 
> I'll include this change in the pull request

Ok. Thanks for the fix-up.

  Paul