diff mbox

[4/4] piix_pci: Implement reset for i440FX PAM configuration

Message ID 1361396809-12973-4-git-send-email-dwmw2@infradead.org
State New
Headers show

Commit Message

David Woodhouse Feb. 20, 2013, 9:46 p.m. UTC 
From: David Woodhouse <David.Woodhouse@intel.com>

This implements reset functionality for the i440FX, resetting all the
PAM registers to their power-on defaults of no RAM access and thus
forwarding all access to the 0xc0000-0xfffff range to PCI address space
(i.e. to the actual ROM) instead of RAM.

Fixing this is sufficient to work around a KVM bug which causes it to
run 16-bit code at 0xffff0 instead of 0xfffffff0 on CPU reset. If reset
was working correctly on the i440FX, that KVM bug wouldn't have
*mattered* because the two addresses would have identical contents.

There's been much discussion about the distinction between hard reset
and soft reset, and the fact that many of our reset triggers (such as
the keyboard controller and triple-fault handler) are actually doing a
full system-wide hard reset when in fact they should be triggering
something much more limited in scope.

This patch exacerbates that existing problem only slightly, by causing
the offending triggers to reset yet another piece of hardware that they
shouldn't have been resetting. But the problem is largely theoretical
anyway; mostly limited to 80286 protected mode software which needs to
initiate a CPU reset to get back into real mode, but which *doesn't*
want a full system reset. Such software is almost certainly already
broken under Qemu anyway, because of all the *other* aspects of a full
hard reset that are already happening.

So this patch can be applied separately from any longer-term fixes to
make the 'soft' reset actually do the right thing.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
 hw/piix_pci.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Laszlo Ersek Feb. 20, 2013, 11:25 p.m. UTC | #1 
On 02/20/13 22:46, David Woodhouse wrote:

> +static void i440fx_reset(DeviceState *ds)
> +{
> +    PCIDevice *dev = PCI_DEVICE(ds);
> +    PCII440FXState *d = I440FX_PCI_DEVICE(dev);
> +    uint8_t *pci_conf = dev->config;
> +
> +    pci_conf[0x59] = 0x00; /* Reset PAM setup */
> +    pci_conf[0x5a] = 0x00;
> +    pci_conf[0x5b] = 0x00;
> +    pci_conf[0x5c] = 0x00;
> +    pci_conf[0x5d] = 0x00;
> +    pci_conf[0x5e] = 0x00;
> +    pci_conf[0x5f] = 0x00;
> +    pci_conf[0x72] = 0x02; /* And SMM */
> +
> +    i440fx_update_memory_mappings(d);
> +}

Consider

    memset(pci_conf + I440FX_PAM, 0, I440FX_PAM_SIZE);
    pci_conf[I440FX_SMRAM] = 0x02;

But I'll let you decide of course.

I tested guest reboot with this series applied, on my
unrestricted-guest-incapable host (Xeon W3550) with OVMF + SeaBIOS CSM.
It works great. Many thanks.

Series

Tested-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Laszlo
Laszlo Ersek Feb. 20, 2013, 11:28 p.m. UTC | #2 
On 02/21/13 00:25, Laszlo Ersek wrote:
> On 02/20/13 22:46, David Woodhouse wrote:
> 
>> +static void i440fx_reset(DeviceState *ds)
>> +{
>> +    PCIDevice *dev = PCI_DEVICE(ds);
>> +    PCII440FXState *d = I440FX_PCI_DEVICE(dev);
>> +    uint8_t *pci_conf = dev->config;
>> +
>> +    pci_conf[0x59] = 0x00; /* Reset PAM setup */
>> +    pci_conf[0x5a] = 0x00;
>> +    pci_conf[0x5b] = 0x00;
>> +    pci_conf[0x5c] = 0x00;
>> +    pci_conf[0x5d] = 0x00;
>> +    pci_conf[0x5e] = 0x00;
>> +    pci_conf[0x5f] = 0x00;
>> +    pci_conf[0x72] = 0x02; /* And SMM */
>> +
>> +    i440fx_update_memory_mappings(d);
>> +}
> 
> Consider
> 
>     memset(pci_conf + I440FX_PAM, 0, I440FX_PAM_SIZE);
>     pci_conf[I440FX_SMRAM] = 0x02;
> 
> But I'll let you decide of course.
> 
> I tested guest reboot with this series applied, on my
> unrestricted-guest-incapable host (Xeon W3550) with OVMF + SeaBIOS CSM.

Ah I was sure I would forget an important test attribute: the host
kernel was RHEL-6.3.z (2.6.32-279.22.1.el6.x86_64), in which KVM still
approximates real mode with vm86.

> It works great. Many thanks.
> 
> Series
> 
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

These stand of course.
L.
Andreas Färber Feb. 20, 2013, 11:31 p.m. UTC | #3 
Am 20.02.2013 22:46, schrieb David Woodhouse:
> From: David Woodhouse <David.Woodhouse@intel.com>
> 
> This implements reset functionality for the i440FX, resetting all the
> PAM registers to their power-on defaults of no RAM access and thus
> forwarding all access to the 0xc0000-0xfffff range to PCI address space
> (i.e. to the actual ROM) instead of RAM.
> 
> Fixing this is sufficient to work around a KVM bug which causes it to
> run 16-bit code at 0xffff0 instead of 0xfffffff0 on CPU reset. If reset
> was working correctly on the i440FX, that KVM bug wouldn't have
> *mattered* because the two addresses would have identical contents.
> 
> There's been much discussion about the distinction between hard reset
> and soft reset, and the fact that many of our reset triggers (such as
> the keyboard controller and triple-fault handler) are actually doing a
> full system-wide hard reset when in fact they should be triggering
> something much more limited in scope.
> 
> This patch exacerbates that existing problem only slightly, by causing
> the offending triggers to reset yet another piece of hardware that they
> shouldn't have been resetting. But the problem is largely theoretical
> anyway; mostly limited to 80286 protected mode software which needs to
> initiate a CPU reset to get back into real mode, but which *doesn't*
> want a full system reset. Such software is almost certainly already
> broken under Qemu anyway, because of all the *other* aspects of a full
> hard reset that are already happening.
> 
> So this patch can be applied separately from any longer-term fixes to
> make the 'soft' reset actually do the right thing.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> ---
>  hw/piix_pci.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 2eeb739..9e6eca0 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -175,6 +175,24 @@ static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +static void i440fx_reset(DeviceState *ds)
> +{
> +    PCIDevice *dev = PCI_DEVICE(ds);
> +    PCII440FXState *d = I440FX_PCI_DEVICE(dev);
> +    uint8_t *pci_conf = dev->config;
> +
> +    pci_conf[0x59] = 0x00; /* Reset PAM setup */
> +    pci_conf[0x5a] = 0x00;
> +    pci_conf[0x5b] = 0x00;
> +    pci_conf[0x5c] = 0x00;
> +    pci_conf[0x5d] = 0x00;
> +    pci_conf[0x5e] = 0x00;
> +    pci_conf[0x5f] = 0x00;
> +    pci_conf[0x72] = 0x02; /* And SMM */
> +
> +    i440fx_update_memory_mappings(d);
> +}
> +
>  static int i440fx_post_load(void *opaque, int version_id)
>  {
>      PCII440FXState *d = opaque;
> @@ -621,6 +639,7 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
>      dc->desc = "Host bridge";
>      dc->no_user = 1;
>      dc->vmsd = &vmstate_i440fx;
> +    dc->reset = i440fx_reset;
>  }
>  
>  static const TypeInfo i440fx_info = {

Cute,

Reviewed-by: Andreas Färber <afaerber@suse.de>

A minor suggestion since you seem to be preparing a v2 would be to do
I440FX_PCI_DEVICE(ds), but I don't see dev becoming unused anytime soon,
therefore moot.

Andreas
David Woodhouse Feb. 20, 2013, 11:34 p.m. UTC | #4 
On Thu, 2013-02-21 at 00:31 +0100, Andreas Färber wrote:
> 
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> 
> A minor suggestion since you seem to be preparing a v2 would be to do
> I440FX_PCI_DEVICE(ds), but I don't see dev becoming unused anytime
> soon, therefore moot.

Well it's only used because you told me *not* to use d->dev :)

But OK. The tree at git.infradead.org/users/dwmw2/qemu.git now has that
change, Laszlo's Reviewed-by: and Tested-by: on all four patches, and
yours on three of the four. Thanks.
Andreas Färber Feb. 20, 2013, 11:40 p.m. UTC | #5 
Am 21.02.2013 00:34, schrieb David Woodhouse:
> On Thu, 2013-02-21 at 00:31 +0100, Andreas Färber wrote:
>>
>>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>
>> A minor suggestion since you seem to be preparing a v2 would be to do
>> I440FX_PCI_DEVICE(ds), but I don't see dev becoming unused anytime
>> soon, therefore moot.
> 
> Well it's only used because you told me *not* to use d->dev :)
> 
> But OK. The tree at git.infradead.org/users/dwmw2/qemu.git now has that
> change, Laszlo's Reviewed-by: and Tested-by: on all four patches, and
> yours on three of the four. Thanks.

You're welcome. On your master branch you may add my Rb on the 4th too.
Just not mentioning it here to avoid confusing patches tool.

Thank you,

Andreas
Paolo Bonzini Feb. 21, 2013, 7:46 a.m. UTC | #6 
Il 21/02/2013 00:34, David Woodhouse ha scritto:
> On Thu, 2013-02-21 at 00:31 +0100, Andreas Färber wrote:
>>
>>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>
>> A minor suggestion since you seem to be preparing a v2 would be to do
>> I440FX_PCI_DEVICE(ds), but I don't see dev becoming unused anytime
>> soon, therefore moot.
> 
> Well it's only used because you told me *not* to use d->dev :)
> 
> But OK. The tree at git.infradead.org/users/dwmw2/qemu.git now has that
> change, Laszlo's Reviewed-by: and Tested-by: on all four patches, and
> yours on three of the four. Thanks.

You may go ahead and send a pull request (note that we do send the other
patches together with a pull request, just the cover letter has [PULL
0/4] and the "git request-pull" output).

Paolo
David Woodhouse Feb. 21, 2013, 9:17 a.m. UTC | #7 
On Thu, 2013-02-21 at 08:46 +0100, Paolo Bonzini wrote:
> You may go ahead and send a pull request (note that we do send the other
> patches together with a pull request, just the cover letter has [PULL
> 0/4] and the "git request-pull" output).

Thanks. I suppose I'd better test that I haven't broken suspend/resume
first. I don't care about OS/2 or VDISK, but it's vaguely possible that
suspend/resume might be another "reset" user which actually *would* be
offended if the PAM configuration got reset, causing the BIOS to do a
complete reinit.

If that breaks, this will have to wait for the 'soft-reset' bits that
Anthony is looking at.
David Woodhouse Feb. 21, 2013, 11:02 a.m. UTC | #8 
On Thu, 2013-02-21 at 09:17 +0000, David Woodhouse wrote:
> Thanks. I suppose I'd better test that I haven't broken suspend/resume
> first. I don't care about OS/2 or VDISK, but it's vaguely possible that
> suspend/resume might be another "reset" user which actually *would* be
> offended if the PAM configuration got reset, causing the BIOS to do a
> complete reinit.
> 
> If that breaks, this will have to wait for the 'soft-reset' bits that
> Anthony is looking at.

Ah crap, it breaks. Suspend/resume now does a full PAM reset, so SeaBIOS
doesn't see that it's a resume and goes through a full hardware init and
reboot.
Laszlo Ersek Feb. 21, 2013, 5:48 p.m. UTC | #9 
On 02/21/13 12:02, David Woodhouse wrote:
> On Thu, 2013-02-21 at 09:17 +0000, David Woodhouse wrote:
>> Thanks. I suppose I'd better test that I haven't broken suspend/resume
>> first. I don't care about OS/2 or VDISK, but it's vaguely possible that
>> suspend/resume might be another "reset" user which actually *would* be
>> offended if the PAM configuration got reset, causing the BIOS to do a
>> complete reinit.
>>
>> If that breaks, this will have to wait for the 'soft-reset' bits that
>> Anthony is looking at.
> 
> Ah crap, it breaks. Suspend/resume now does a full PAM reset, so SeaBIOS
> doesn't see that it's a resume and goes through a full hardware init and
> reboot.

:(

But at least now I understand why the PAM regs were not reset before,
and why SeaBIOS has to jump through the qemu_prep_reset() hoop... I'm
afraid people had this entire discussion before :(

I'm sorry I couldn't think of this earlier... May be related to the fact
that I could never really appreciate the "suspend in a VM" thing.

Laszlo
diff mbox

Patch

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 2eeb739..9e6eca0 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -175,6 +175,24 @@  static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
     return 0;
 }
 
+static void i440fx_reset(DeviceState *ds)
+{
+    PCIDevice *dev = PCI_DEVICE(ds);
+    PCII440FXState *d = I440FX_PCI_DEVICE(dev);
+    uint8_t *pci_conf = dev->config;
+
+    pci_conf[0x59] = 0x00; /* Reset PAM setup */
+    pci_conf[0x5a] = 0x00;
+    pci_conf[0x5b] = 0x00;
+    pci_conf[0x5c] = 0x00;
+    pci_conf[0x5d] = 0x00;
+    pci_conf[0x5e] = 0x00;
+    pci_conf[0x5f] = 0x00;
+    pci_conf[0x72] = 0x02; /* And SMM */
+
+    i440fx_update_memory_mappings(d);
+}
+
 static int i440fx_post_load(void *opaque, int version_id)
 {
     PCII440FXState *d = opaque;
@@ -621,6 +639,7 @@  static void i440fx_class_init(ObjectClass *klass, void *data)
     dc->desc = "Host bridge";
     dc->no_user = 1;
     dc->vmsd = &vmstate_i440fx;
+    dc->reset = i440fx_reset;
 }
 
 static const TypeInfo i440fx_info = {