Re: [Qemu-devel] [PATCH 4/4] piix_pci: Implement reset for i440FX PAM configuration

[Andreas Färber]

Am 20.02.2013 22:46, schrieb David Woodhouse:
> From: David Woodhouse <address@hidden>
> 
> 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 <address@hidden>
> ---
>  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 <address@hidden>

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg