| Message ID | 1371416189-29563-1-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/16/13 22:59, Michael S. Tsirkin wrote: > Avoid use of static variables: PC systems initialize pvpanic device > through pvpanic_init, so we can simply create the fw_cfg file at that > point. Others don't use fw_cfg at all. This also makes it possible to > assert if fw_cfg is not there rather than skipping the device silently. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/misc/pvpanic.c | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c > index 060099b..9ed9897 100644 > --- a/hw/misc/pvpanic.c > +++ b/hw/misc/pvpanic.c > @@ -97,25 +97,22 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) > { > ISADevice *d = ISA_DEVICE(dev); > PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > - static bool port_configured; > - FWCfgState *fw_cfg; > > isa_register_ioport(d, &s->io, s->ioport); > - > - if (!port_configured) { > - fw_cfg = fw_cfg_find(); > - if (fw_cfg) { > - fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", > - g_memdup(&s->ioport, sizeof(s->ioport)), > - sizeof(s->ioport)); > - port_configured = true; > - } > - } > } > > int pvpanic_init(ISABus *bus) > { > - isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > + ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > + PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > + FWCfgState *fw_cfg = fw_cfg_find(); > + > + assert(fw_cfg); Won't the assert fire if: xen_enabled() && machine != "pc-0.10" && machine != "pc-0.11" && machine != "pc-0.12" && machine != "pc-0.13" && machine != "pc-q35-1.4" Because under the above condition "has_pvpanic" remains "true", but fw_cfg is not initialized. (pc_init_pci_no_kvmclock() in "hw/i386/pc_piix.c" sets "has_pvpanic" to "false", and claims to be "reused by xenfv", so the above condition may be constant false.) > + > + fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", > + g_memdup(&s->ioport, sizeof(s->ioport)), > + sizeof(s->ioport)); > + > return 0; > } > > Series looks good to me otherwise. Thanks Laszlo
On Mon, Jun 17, 2013 at 09:56:56AM +0200, Laszlo Ersek wrote: > On 06/16/13 22:59, Michael S. Tsirkin wrote: > > Avoid use of static variables: PC systems initialize pvpanic device > > through pvpanic_init, so we can simply create the fw_cfg file at that > > point. Others don't use fw_cfg at all. This also makes it possible to > > assert if fw_cfg is not there rather than skipping the device silently. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > hw/misc/pvpanic.c | 23 ++++++++++------------- > > 1 file changed, 10 insertions(+), 13 deletions(-) > > > > diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c > > index 060099b..9ed9897 100644 > > --- a/hw/misc/pvpanic.c > > +++ b/hw/misc/pvpanic.c > > @@ -97,25 +97,22 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) > > { > > ISADevice *d = ISA_DEVICE(dev); > > PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > > - static bool port_configured; > > - FWCfgState *fw_cfg; > > > > isa_register_ioport(d, &s->io, s->ioport); > > - > > - if (!port_configured) { > > - fw_cfg = fw_cfg_find(); > > - if (fw_cfg) { > > - fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", > > - g_memdup(&s->ioport, sizeof(s->ioport)), > > - sizeof(s->ioport)); > > - port_configured = true; > > - } > > - } > > } > > > > int pvpanic_init(ISABus *bus) > > { > > - isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > > + ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > > + PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > > + FWCfgState *fw_cfg = fw_cfg_find(); > > + > > + assert(fw_cfg); > > Won't the assert fire if: > > xen_enabled() && > machine != "pc-0.10" && machine != "pc-0.11" && > machine != "pc-0.12" && machine != "pc-0.13" && > machine != "pc-q35-1.4" > > Because under the above condition "has_pvpanic" remains "true", but > fw_cfg is not initialized. > > (pc_init_pci_no_kvmclock() in "hw/i386/pc_piix.c" sets "has_pvpanic" to > "false", and claims to be "reused by xenfv", so the above condition may > be constant false.) That's what I think - if user wants pvpanic to work, fw cfg is required ATM. > > + > > + fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", > > + g_memdup(&s->ioport, sizeof(s->ioport)), > > + sizeof(s->ioport)); > > + > > return 0; > > } > > > > > > Series looks good to me otherwise. > > Thanks > Laszlo
On 06/17/13 11:19, Michael S. Tsirkin wrote: > On Mon, Jun 17, 2013 at 09:56:56AM +0200, Laszlo Ersek wrote: >> On 06/16/13 22:59, Michael S. Tsirkin wrote: >>> Avoid use of static variables: PC systems initialize pvpanic device >>> through pvpanic_init, so we can simply create the fw_cfg file at that >>> point. Others don't use fw_cfg at all. This also makes it possible to >>> assert if fw_cfg is not there rather than skipping the device silently. >>> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> hw/misc/pvpanic.c | 23 ++++++++++------------- >>> 1 file changed, 10 insertions(+), 13 deletions(-) >>> >>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c >>> index 060099b..9ed9897 100644 >>> --- a/hw/misc/pvpanic.c >>> +++ b/hw/misc/pvpanic.c >>> @@ -97,25 +97,22 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) >>> { >>> ISADevice *d = ISA_DEVICE(dev); >>> PVPanicState *s = ISA_PVPANIC_DEVICE(dev); >>> - static bool port_configured; >>> - FWCfgState *fw_cfg; >>> >>> isa_register_ioport(d, &s->io, s->ioport); >>> - >>> - if (!port_configured) { >>> - fw_cfg = fw_cfg_find(); >>> - if (fw_cfg) { >>> - fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", >>> - g_memdup(&s->ioport, sizeof(s->ioport)), >>> - sizeof(s->ioport)); >>> - port_configured = true; >>> - } >>> - } >>> } >>> >>> int pvpanic_init(ISABus *bus) >>> { >>> - isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); >>> + ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); >>> + PVPanicState *s = ISA_PVPANIC_DEVICE(dev); >>> + FWCfgState *fw_cfg = fw_cfg_find(); >>> + >>> + assert(fw_cfg); >> >> Won't the assert fire if: >> >> xen_enabled() && >> machine != "pc-0.10" && machine != "pc-0.11" && >> machine != "pc-0.12" && machine != "pc-0.13" && >> machine != "pc-q35-1.4" >> >> Because under the above condition "has_pvpanic" remains "true", but >> fw_cfg is not initialized. >> >> (pc_init_pci_no_kvmclock() in "hw/i386/pc_piix.c" sets "has_pvpanic" to >> "false", and claims to be "reused by xenfv", so the above condition may >> be constant false.) > > That's what I think - if user wants pvpanic to work, fw cfg is required ATM. What I have in mind is the following: suppose xen is enabled and qemu is started with -M pc-i440fx-1.5. Before the patch, the pvpanic device didn't work, but qemu didn't crash either. After the patch, the assert() is triggered at startup. Of course, if starting qemu for xen with "-M pc-i440fx-1.5" is *already* broken (for other, maybe more serious, reasons), ie. PEBKAC, then the patch is correct. But I can't evaluate that condition to constant false, and suppose that it's a possible configuration, under which qemu would now start with an assertion failure. Can someone with Xen knowledge chime in? CC'ing Stefano. Laszlo
On Mon, Jun 17, 2013 at 11:35:00AM +0200, Laszlo Ersek wrote: > On 06/17/13 11:19, Michael S. Tsirkin wrote: > > On Mon, Jun 17, 2013 at 09:56:56AM +0200, Laszlo Ersek wrote: > >> On 06/16/13 22:59, Michael S. Tsirkin wrote: > >>> Avoid use of static variables: PC systems initialize pvpanic device > >>> through pvpanic_init, so we can simply create the fw_cfg file at that > >>> point. Others don't use fw_cfg at all. This also makes it possible to > >>> assert if fw_cfg is not there rather than skipping the device silently. > >>> > >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>> --- > >>> hw/misc/pvpanic.c | 23 ++++++++++------------- > >>> 1 file changed, 10 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c > >>> index 060099b..9ed9897 100644 > >>> --- a/hw/misc/pvpanic.c > >>> +++ b/hw/misc/pvpanic.c > >>> @@ -97,25 +97,22 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) > >>> { > >>> ISADevice *d = ISA_DEVICE(dev); > >>> PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > >>> - static bool port_configured; > >>> - FWCfgState *fw_cfg; > >>> > >>> isa_register_ioport(d, &s->io, s->ioport); > >>> - > >>> - if (!port_configured) { > >>> - fw_cfg = fw_cfg_find(); > >>> - if (fw_cfg) { > >>> - fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", > >>> - g_memdup(&s->ioport, sizeof(s->ioport)), > >>> - sizeof(s->ioport)); > >>> - port_configured = true; > >>> - } > >>> - } > >>> } > >>> > >>> int pvpanic_init(ISABus *bus) > >>> { > >>> - isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > >>> + ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > >>> + PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > >>> + FWCfgState *fw_cfg = fw_cfg_find(); > >>> + > >>> + assert(fw_cfg); > >> > >> Won't the assert fire if: > >> > >> xen_enabled() && > >> machine != "pc-0.10" && machine != "pc-0.11" && > >> machine != "pc-0.12" && machine != "pc-0.13" && > >> machine != "pc-q35-1.4" > >> > >> Because under the above condition "has_pvpanic" remains "true", but > >> fw_cfg is not initialized. > >> > >> (pc_init_pci_no_kvmclock() in "hw/i386/pc_piix.c" sets "has_pvpanic" to > >> "false", and claims to be "reused by xenfv", so the above condition may > >> be constant false.) > > > > That's what I think - if user wants pvpanic to work, fw cfg is required ATM. > > What I have in mind is the following: suppose xen is enabled and qemu is > started with -M pc-i440fx-1.5. > > Before the patch, the pvpanic device didn't work, but qemu didn't crash > either. After the patch, the assert() is triggered at startup. > > Of course, if starting qemu for xen with "-M pc-i440fx-1.5" is *already* > broken (for other, maybe more serious, reasons), ie. PEBKAC, then the > patch is correct. But I can't evaluate that condition to constant false, > and suppose that it's a possible configuration, under which qemu would > now start with an assertion failure. > > Can someone with Xen knowledge chime in? CC'ing Stefano. > > Laszlo A sane alternative is to avoid creating the pvpanic device. Not as easy to debug as an assert, but at least guest does not get reserved ports which said guest has no way to discover.
On 06/17/13 11:57, Michael S. Tsirkin wrote: > On Mon, Jun 17, 2013 at 11:35:00AM +0200, Laszlo Ersek wrote: >> On 06/17/13 11:19, Michael S. Tsirkin wrote: >>> On Mon, Jun 17, 2013 at 09:56:56AM +0200, Laszlo Ersek wrote: >>>> On 06/16/13 22:59, Michael S. Tsirkin wrote: >>>>> Avoid use of static variables: PC systems initialize pvpanic device >>>>> through pvpanic_init, so we can simply create the fw_cfg file at that >>>>> point. Others don't use fw_cfg at all. This also makes it possible to >>>>> assert if fw_cfg is not there rather than skipping the device silently. >>>>> >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>> --- >>>>> hw/misc/pvpanic.c | 23 ++++++++++------------- >>>>> 1 file changed, 10 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c >>>>> index 060099b..9ed9897 100644 >>>>> --- a/hw/misc/pvpanic.c >>>>> +++ b/hw/misc/pvpanic.c >>>>> @@ -97,25 +97,22 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) >>>>> { >>>>> ISADevice *d = ISA_DEVICE(dev); >>>>> PVPanicState *s = ISA_PVPANIC_DEVICE(dev); >>>>> - static bool port_configured; >>>>> - FWCfgState *fw_cfg; >>>>> >>>>> isa_register_ioport(d, &s->io, s->ioport); >>>>> - >>>>> - if (!port_configured) { >>>>> - fw_cfg = fw_cfg_find(); >>>>> - if (fw_cfg) { >>>>> - fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", >>>>> - g_memdup(&s->ioport, sizeof(s->ioport)), >>>>> - sizeof(s->ioport)); >>>>> - port_configured = true; >>>>> - } >>>>> - } >>>>> } >>>>> >>>>> int pvpanic_init(ISABus *bus) >>>>> { >>>>> - isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); >>>>> + ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); >>>>> + PVPanicState *s = ISA_PVPANIC_DEVICE(dev); >>>>> + FWCfgState *fw_cfg = fw_cfg_find(); >>>>> + >>>>> + assert(fw_cfg); >>>> >>>> Won't the assert fire if: >>>> >>>> xen_enabled() && >>>> machine != "pc-0.10" && machine != "pc-0.11" && >>>> machine != "pc-0.12" && machine != "pc-0.13" && >>>> machine != "pc-q35-1.4" >>>> >>>> Because under the above condition "has_pvpanic" remains "true", but >>>> fw_cfg is not initialized. >>>> >>>> (pc_init_pci_no_kvmclock() in "hw/i386/pc_piix.c" sets "has_pvpanic" to >>>> "false", and claims to be "reused by xenfv", so the above condition may >>>> be constant false.) >>> >>> That's what I think - if user wants pvpanic to work, fw cfg is required ATM. >> >> What I have in mind is the following: suppose xen is enabled and qemu is >> started with -M pc-i440fx-1.5. >> >> Before the patch, the pvpanic device didn't work, but qemu didn't crash >> either. After the patch, the assert() is triggered at startup. >> >> Of course, if starting qemu for xen with "-M pc-i440fx-1.5" is *already* >> broken (for other, maybe more serious, reasons), ie. PEBKAC, then the >> patch is correct. But I can't evaluate that condition to constant false, >> and suppose that it's a possible configuration, under which qemu would >> now start with an assertion failure. >> >> Can someone with Xen knowledge chime in? CC'ing Stefano. >> >> Laszlo > > A sane alternative is to avoid creating the pvpanic device. > Not as easy to debug as an assert, but at least > guest does not get reserved ports which said guest > has no way to discover. Yes, I think that's exactly what happens *if* at domain creation time the Xen userspace utilities start qemu with such a machine model that sets "has_pvpanic" to false. I'd only like to have confirmation that the leading comment on pc_init_pci_no_kvmclock() is up-to-date and we can trust this code never to run on Xen. Actually, we can figure out later, if/when it breaks under Xen. It shouldn't be hard to fix. series Reviewed-by: Laszlo Ersek <lersek@redhat.com>
On Mon, 17 Jun 2013, Laszlo Ersek wrote: > On 06/17/13 11:19, Michael S. Tsirkin wrote: > > On Mon, Jun 17, 2013 at 09:56:56AM +0200, Laszlo Ersek wrote: > >> On 06/16/13 22:59, Michael S. Tsirkin wrote: > >>> Avoid use of static variables: PC systems initialize pvpanic device > >>> through pvpanic_init, so we can simply create the fw_cfg file at that > >>> point. Others don't use fw_cfg at all. This also makes it possible to > >>> assert if fw_cfg is not there rather than skipping the device silently. > >>> > >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>> --- > >>> hw/misc/pvpanic.c | 23 ++++++++++------------- > >>> 1 file changed, 10 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c > >>> index 060099b..9ed9897 100644 > >>> --- a/hw/misc/pvpanic.c > >>> +++ b/hw/misc/pvpanic.c > >>> @@ -97,25 +97,22 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) > >>> { > >>> ISADevice *d = ISA_DEVICE(dev); > >>> PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > >>> - static bool port_configured; > >>> - FWCfgState *fw_cfg; > >>> > >>> isa_register_ioport(d, &s->io, s->ioport); > >>> - > >>> - if (!port_configured) { > >>> - fw_cfg = fw_cfg_find(); > >>> - if (fw_cfg) { > >>> - fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", > >>> - g_memdup(&s->ioport, sizeof(s->ioport)), > >>> - sizeof(s->ioport)); > >>> - port_configured = true; > >>> - } > >>> - } > >>> } > >>> > >>> int pvpanic_init(ISABus *bus) > >>> { > >>> - isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > >>> + ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > >>> + PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > >>> + FWCfgState *fw_cfg = fw_cfg_find(); > >>> + > >>> + assert(fw_cfg); > >> > >> Won't the assert fire if: > >> > >> xen_enabled() && > >> machine != "pc-0.10" && machine != "pc-0.11" && > >> machine != "pc-0.12" && machine != "pc-0.13" && > >> machine != "pc-q35-1.4" > >> > >> Because under the above condition "has_pvpanic" remains "true", but > >> fw_cfg is not initialized. > >> > >> (pc_init_pci_no_kvmclock() in "hw/i386/pc_piix.c" sets "has_pvpanic" to > >> "false", and claims to be "reused by xenfv", so the above condition may > >> be constant false.) Thanks for spotting this! > > That's what I think - if user wants pvpanic to work, fw cfg is required ATM. > > What I have in mind is the following: suppose xen is enabled and qemu is > started with -M pc-i440fx-1.5. > > Before the patch, the pvpanic device didn't work, but qemu didn't crash > either. After the patch, the assert() is triggered at startup. > > Of course, if starting qemu for xen with "-M pc-i440fx-1.5" is *already* > broken (for other, maybe more serious, reasons), ie. PEBKAC, then the > patch is correct. But I can't evaluate that condition to constant false, > and suppose that it's a possible configuration, under which qemu would > now start with an assertion failure. > > Can someone with Xen knowledge chime in? CC'ing Stefano. Give a look at hw/i386/pc_piix.c: the xenfv machine has pc_xen_hvm_init as init function; pc_xen_hvm_init calls xen_hvm_init and pc_init_pci. Unfortunately the comment on top of pc_init_pci_no_kvmclock is not up to date as of: commit 9cdf79d068f52f7de347cb45cfd8903519410e4d Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Date: Mon May 27 16:46:01 2013 +0000 xen: use pc_init_pci instead of pc_init_pci_no_kvmclock
On Mon, 17 Jun 2013, Michael S. Tsirkin wrote: > On Mon, Jun 17, 2013 at 11:35:00AM +0200, Laszlo Ersek wrote: > > On 06/17/13 11:19, Michael S. Tsirkin wrote: > > > On Mon, Jun 17, 2013 at 09:56:56AM +0200, Laszlo Ersek wrote: > > >> On 06/16/13 22:59, Michael S. Tsirkin wrote: > > >>> Avoid use of static variables: PC systems initialize pvpanic device > > >>> through pvpanic_init, so we can simply create the fw_cfg file at that > > >>> point. Others don't use fw_cfg at all. This also makes it possible to > > >>> assert if fw_cfg is not there rather than skipping the device silently. > > >>> > > >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > >>> --- > > >>> hw/misc/pvpanic.c | 23 ++++++++++------------- > > >>> 1 file changed, 10 insertions(+), 13 deletions(-) > > >>> > > >>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c > > >>> index 060099b..9ed9897 100644 > > >>> --- a/hw/misc/pvpanic.c > > >>> +++ b/hw/misc/pvpanic.c > > >>> @@ -97,25 +97,22 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) > > >>> { > > >>> ISADevice *d = ISA_DEVICE(dev); > > >>> PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > > >>> - static bool port_configured; > > >>> - FWCfgState *fw_cfg; > > >>> > > >>> isa_register_ioport(d, &s->io, s->ioport); > > >>> - > > >>> - if (!port_configured) { > > >>> - fw_cfg = fw_cfg_find(); > > >>> - if (fw_cfg) { > > >>> - fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", > > >>> - g_memdup(&s->ioport, sizeof(s->ioport)), > > >>> - sizeof(s->ioport)); > > >>> - port_configured = true; > > >>> - } > > >>> - } > > >>> } > > >>> > > >>> int pvpanic_init(ISABus *bus) > > >>> { > > >>> - isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > > >>> + ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > > >>> + PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > > >>> + FWCfgState *fw_cfg = fw_cfg_find(); > > >>> + > > >>> + assert(fw_cfg); > > >> > > >> Won't the assert fire if: > > >> > > >> xen_enabled() && > > >> machine != "pc-0.10" && machine != "pc-0.11" && > > >> machine != "pc-0.12" && machine != "pc-0.13" && > > >> machine != "pc-q35-1.4" > > >> > > >> Because under the above condition "has_pvpanic" remains "true", but > > >> fw_cfg is not initialized. > > >> > > >> (pc_init_pci_no_kvmclock() in "hw/i386/pc_piix.c" sets "has_pvpanic" to > > >> "false", and claims to be "reused by xenfv", so the above condition may > > >> be constant false.) > > > > > > That's what I think - if user wants pvpanic to work, fw cfg is required ATM. > > > > What I have in mind is the following: suppose xen is enabled and qemu is > > started with -M pc-i440fx-1.5. > > > > Before the patch, the pvpanic device didn't work, but qemu didn't crash > > either. After the patch, the assert() is triggered at startup. > > > > Of course, if starting qemu for xen with "-M pc-i440fx-1.5" is *already* > > broken (for other, maybe more serious, reasons), ie. PEBKAC, then the > > patch is correct. But I can't evaluate that condition to constant false, > > and suppose that it's a possible configuration, under which qemu would > > now start with an assertion failure. > > > > Can someone with Xen knowledge chime in? CC'ing Stefano. > > > > Laszlo > > A sane alternative is to avoid creating the pvpanic device. > Not as easy to debug as an assert, but at least > guest does not get reserved ports which said guest > has no way to discover. Yeah, that's pretty bad. Even though at the moment the Xen tools would always create QEMU with -M xenfv, I would like to be able to use other QEMUMachines too in the future.
On Mon, 17 Jun 2013, Laszlo Ersek wrote: > On 06/17/13 11:57, Michael S. Tsirkin wrote: > > On Mon, Jun 17, 2013 at 11:35:00AM +0200, Laszlo Ersek wrote: > >> On 06/17/13 11:19, Michael S. Tsirkin wrote: > >>> On Mon, Jun 17, 2013 at 09:56:56AM +0200, Laszlo Ersek wrote: > >>>> On 06/16/13 22:59, Michael S. Tsirkin wrote: > >>>>> Avoid use of static variables: PC systems initialize pvpanic device > >>>>> through pvpanic_init, so we can simply create the fw_cfg file at that > >>>>> point. Others don't use fw_cfg at all. This also makes it possible to > >>>>> assert if fw_cfg is not there rather than skipping the device silently. > >>>>> > >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>>>> --- > >>>>> hw/misc/pvpanic.c | 23 ++++++++++------------- > >>>>> 1 file changed, 10 insertions(+), 13 deletions(-) > >>>>> > >>>>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c > >>>>> index 060099b..9ed9897 100644 > >>>>> --- a/hw/misc/pvpanic.c > >>>>> +++ b/hw/misc/pvpanic.c > >>>>> @@ -97,25 +97,22 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) > >>>>> { > >>>>> ISADevice *d = ISA_DEVICE(dev); > >>>>> PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > >>>>> - static bool port_configured; > >>>>> - FWCfgState *fw_cfg; > >>>>> > >>>>> isa_register_ioport(d, &s->io, s->ioport); > >>>>> - > >>>>> - if (!port_configured) { > >>>>> - fw_cfg = fw_cfg_find(); > >>>>> - if (fw_cfg) { > >>>>> - fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", > >>>>> - g_memdup(&s->ioport, sizeof(s->ioport)), > >>>>> - sizeof(s->ioport)); > >>>>> - port_configured = true; > >>>>> - } > >>>>> - } > >>>>> } > >>>>> > >>>>> int pvpanic_init(ISABus *bus) > >>>>> { > >>>>> - isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > >>>>> + ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); > >>>>> + PVPanicState *s = ISA_PVPANIC_DEVICE(dev); > >>>>> + FWCfgState *fw_cfg = fw_cfg_find(); > >>>>> + > >>>>> + assert(fw_cfg); > >>>> > >>>> Won't the assert fire if: > >>>> > >>>> xen_enabled() && > >>>> machine != "pc-0.10" && machine != "pc-0.11" && > >>>> machine != "pc-0.12" && machine != "pc-0.13" && > >>>> machine != "pc-q35-1.4" > >>>> > >>>> Because under the above condition "has_pvpanic" remains "true", but > >>>> fw_cfg is not initialized. > >>>> > >>>> (pc_init_pci_no_kvmclock() in "hw/i386/pc_piix.c" sets "has_pvpanic" to > >>>> "false", and claims to be "reused by xenfv", so the above condition may > >>>> be constant false.) > >>> > >>> That's what I think - if user wants pvpanic to work, fw cfg is required ATM. > >> > >> What I have in mind is the following: suppose xen is enabled and qemu is > >> started with -M pc-i440fx-1.5. > >> > >> Before the patch, the pvpanic device didn't work, but qemu didn't crash > >> either. After the patch, the assert() is triggered at startup. > >> > >> Of course, if starting qemu for xen with "-M pc-i440fx-1.5" is *already* > >> broken (for other, maybe more serious, reasons), ie. PEBKAC, then the > >> patch is correct. But I can't evaluate that condition to constant false, > >> and suppose that it's a possible configuration, under which qemu would > >> now start with an assertion failure. > >> > >> Can someone with Xen knowledge chime in? CC'ing Stefano. > >> > >> Laszlo > > > > A sane alternative is to avoid creating the pvpanic device. > > Not as easy to debug as an assert, but at least > > guest does not get reserved ports which said guest > > has no way to discover. > > Yes, I think that's exactly what happens *if* at domain creation time > the Xen userspace utilities start qemu with such a machine model that > sets "has_pvpanic" to false. I'd only like to have confirmation that the > leading comment on pc_init_pci_no_kvmclock() is up-to-date and we can > trust this code never to run on Xen. xenfv now uses pc_xen_hvm_init, that calls directly pc_init_pci, so has_pvpanic would be true. However we could easily change that if it is necessary. Even if we fix xenfv, I would like to retain the possibility to start QEMU on Xen with other QEMUMachine though. > Actually, we can figure out later, if/when it breaks under Xen. It > shouldn't be hard to fix. > > series > Reviewed-by: Laszlo Ersek <lersek@redhat.com> I really appreciate that you involved me in this discussion before committing the patches and I wouldn't want to be the cause of a delay in QEMU development. However in general I think it's reasonable to wait a couple of days for an answer when a clear possibility for breakage exists.
On 06/19/13 15:39, Stefano Stabellini wrote: > On Mon, 17 Jun 2013, Laszlo Ersek wrote: >> On 06/17/13 11:57, Michael S. Tsirkin wrote: >>> On Mon, Jun 17, 2013 at 11:35:00AM +0200, Laszlo Ersek wrote: >>>> On 06/17/13 11:19, Michael S. Tsirkin wrote: >>>>> On Mon, Jun 17, 2013 at 09:56:56AM +0200, Laszlo Ersek wrote: >>>>>> On 06/16/13 22:59, Michael S. Tsirkin wrote: >>>>>>> Avoid use of static variables: PC systems initialize pvpanic device >>>>>>> through pvpanic_init, so we can simply create the fw_cfg file at that >>>>>>> point. Others don't use fw_cfg at all. This also makes it possible to >>>>>>> assert if fw_cfg is not there rather than skipping the device silently. >>>>>>> >>>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>>>> --- >>>>>>> hw/misc/pvpanic.c | 23 ++++++++++------------- >>>>>>> 1 file changed, 10 insertions(+), 13 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c >>>>>>> index 060099b..9ed9897 100644 >>>>>>> --- a/hw/misc/pvpanic.c >>>>>>> +++ b/hw/misc/pvpanic.c >>>>>>> @@ -97,25 +97,22 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) >>>>>>> { >>>>>>> ISADevice *d = ISA_DEVICE(dev); >>>>>>> PVPanicState *s = ISA_PVPANIC_DEVICE(dev); >>>>>>> - static bool port_configured; >>>>>>> - FWCfgState *fw_cfg; >>>>>>> >>>>>>> isa_register_ioport(d, &s->io, s->ioport); >>>>>>> - >>>>>>> - if (!port_configured) { >>>>>>> - fw_cfg = fw_cfg_find(); >>>>>>> - if (fw_cfg) { >>>>>>> - fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", >>>>>>> - g_memdup(&s->ioport, sizeof(s->ioport)), >>>>>>> - sizeof(s->ioport)); >>>>>>> - port_configured = true; >>>>>>> - } >>>>>>> - } >>>>>>> } >>>>>>> >>>>>>> int pvpanic_init(ISABus *bus) >>>>>>> { >>>>>>> - isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); >>>>>>> + ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); >>>>>>> + PVPanicState *s = ISA_PVPANIC_DEVICE(dev); >>>>>>> + FWCfgState *fw_cfg = fw_cfg_find(); >>>>>>> + >>>>>>> + assert(fw_cfg); >>>>>> >>>>>> Won't the assert fire if: >>>>>> >>>>>> xen_enabled() && >>>>>> machine != "pc-0.10" && machine != "pc-0.11" && >>>>>> machine != "pc-0.12" && machine != "pc-0.13" && >>>>>> machine != "pc-q35-1.4" >>>>>> >>>>>> Because under the above condition "has_pvpanic" remains "true", but >>>>>> fw_cfg is not initialized. >>>>>> >>>>>> (pc_init_pci_no_kvmclock() in "hw/i386/pc_piix.c" sets "has_pvpanic" to >>>>>> "false", and claims to be "reused by xenfv", so the above condition may >>>>>> be constant false.) >>>>> >>>>> That's what I think - if user wants pvpanic to work, fw cfg is required ATM. >>>> >>>> What I have in mind is the following: suppose xen is enabled and qemu is >>>> started with -M pc-i440fx-1.5. >>>> >>>> Before the patch, the pvpanic device didn't work, but qemu didn't crash >>>> either. After the patch, the assert() is triggered at startup. >>>> >>>> Of course, if starting qemu for xen with "-M pc-i440fx-1.5" is *already* >>>> broken (for other, maybe more serious, reasons), ie. PEBKAC, then the >>>> patch is correct. But I can't evaluate that condition to constant false, >>>> and suppose that it's a possible configuration, under which qemu would >>>> now start with an assertion failure. >>>> >>>> Can someone with Xen knowledge chime in? CC'ing Stefano. >>>> >>>> Laszlo >>> >>> A sane alternative is to avoid creating the pvpanic device. >>> Not as easy to debug as an assert, but at least >>> guest does not get reserved ports which said guest >>> has no way to discover. >> >> Yes, I think that's exactly what happens *if* at domain creation time >> the Xen userspace utilities start qemu with such a machine model that >> sets "has_pvpanic" to false. I'd only like to have confirmation that the >> leading comment on pc_init_pci_no_kvmclock() is up-to-date and we can >> trust this code never to run on Xen. > > xenfv now uses pc_xen_hvm_init, that calls directly pc_init_pci, so > has_pvpanic would be true. However we could easily change that if it is > necessary. Even if we fix xenfv, I would like to retain the possibility > to start QEMU on Xen with other QEMUMachine though. > > >> Actually, we can figure out later, if/when it breaks under Xen. It >> shouldn't be hard to fix. >> >> series >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > I really appreciate that you involved me in this discussion before > committing the patches and I wouldn't want to be the cause of a delay > in QEMU development. However in general I think it's reasonable to wait > a couple of days for an answer when a clear possibility for breakage > exists. It's become clear to me that one can't review stuff without irritating at least one party. I chose to irritate xen developers / users rather than annoying Michael by stalling his patch. Sorry. There's no good solution for the messenger here. BTW I also asked Paul Durrant about this in the meantime [1]. He confirmed my worries and Michael posted an updated version [2]. [1] http://thread.gmane.org/gmane.comp.emulators.qemu/217364/focus=217393 [2] http://thread.gmane.org/gmane.comp.emulators.qemu/217408 Laszlo
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c index 060099b..9ed9897 100644 --- a/hw/misc/pvpanic.c +++ b/hw/misc/pvpanic.c @@ -97,25 +97,22 @@ static void pvpanic_isa_realizefn(DeviceState *dev, Error **errp) { ISADevice *d = ISA_DEVICE(dev); PVPanicState *s = ISA_PVPANIC_DEVICE(dev); - static bool port_configured; - FWCfgState *fw_cfg; isa_register_ioport(d, &s->io, s->ioport); - - if (!port_configured) { - fw_cfg = fw_cfg_find(); - if (fw_cfg) { - fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", - g_memdup(&s->ioport, sizeof(s->ioport)), - sizeof(s->ioport)); - port_configured = true; - } - } } int pvpanic_init(ISABus *bus) { - isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); + ISADevice *dev = isa_create_simple(bus, TYPE_ISA_PVPANIC_DEVICE); + PVPanicState *s = ISA_PVPANIC_DEVICE(dev); + FWCfgState *fw_cfg = fw_cfg_find(); + + assert(fw_cfg); + + fw_cfg_add_file(fw_cfg, "etc/pvpanic-port", + g_memdup(&s->ioport, sizeof(s->ioport)), + sizeof(s->ioport)); + return 0; }
Avoid use of static variables: PC systems initialize pvpanic device through pvpanic_init, so we can simply create the fw_cfg file at that point. Others don't use fw_cfg at all. This also makes it possible to assert if fw_cfg is not there rather than skipping the device silently. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/misc/pvpanic.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)