i440fx: implement reset

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>

Am 20.02.2013 16:35, schrieb David Woodhouse:
> 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>
> 
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 6c77e49..f81c6aa 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -171,6 +171,24 @@ static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +static void i440fx_reset(DeviceState *ds)
> +{
> +    PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, ds);
> +    PCII440FXState *d = DO_UPCAST(PCII440FXState, dev, dev);

Please don't use DO_UPCAST() on QOM objects, there's QOM cast macros for
this, like PCI_DEVICE(). If there's one missing for PCII440FXState (I
think I only added them for the PHB itself?), it's two lines to add.

> +    uint8_t *pci_conf = d->dev.config;
> +
> +    pci_conf[0x59] = 0x00; // Reset PAM setup

scripts/checkpatch.pl will complain about this comment style.

> +    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;
> @@ -615,6 +633,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 = {

Otherwise looks okay.

Andreas

Am 20.02.2013 17:17, schrieb Andreas Färber:
> Am 20.02.2013 16:35, schrieb David Woodhouse:
>> 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>
>>
>> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
>> index 6c77e49..f81c6aa 100644
>> --- a/hw/piix_pci.c
>> +++ b/hw/piix_pci.c
>> @@ -171,6 +171,24 @@ static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
>>      return 0;
>>  }
>>  
>> +static void i440fx_reset(DeviceState *ds)
>> +{
>> +    PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, ds);
>> +    PCII440FXState *d = DO_UPCAST(PCII440FXState, dev, dev);
> 
> Please don't use DO_UPCAST() on QOM objects, there's QOM cast macros for
> this, like PCI_DEVICE(). If there's one missing for PCII440FXState (I
> think I only added them for the PHB itself?), it's two lines to add.
> 
>> +    uint8_t *pci_conf = d->dev.config;

In the same spirit that should be dev->config, not d->dev.config BTW.

Cf. http://wiki.qemu.org/QOMConventions

Andreas

>> +
>> +    pci_conf[0x59] = 0x00; // Reset PAM setup
> 
> scripts/checkpatch.pl will complain about this comment style.
> 
>> +    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;
>> @@ -615,6 +633,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 = {
> 
> Otherwise looks okay.
> 
> Andreas

On Wed, 2013-02-20 at 17:17 +0100, Andreas Färber wrote:
> > +static void i440fx_reset(DeviceState *ds)
> > +{
> > +    PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, ds);
> > +    PCII440FXState *d = DO_UPCAST(PCII440FXState, dev, dev);
> 
> Please don't use DO_UPCAST() on QOM objects, there's QOM cast macros for
> this, like PCI_DEVICE(). If there's one missing for PCII440FXState (I
> think I only added them for the PHB itself?), it's two lines to add.

Would you care to elaborate on exactly what those two lines would look
like? Being *completely* unfamiliar with qemu code, it took me long
enough to work out the DO_UPCAST version.

> > +    uint8_t *pci_conf = d->dev.config;
> > +
> > +    pci_conf[0x59] = 0x00; // Reset PAM setup
> 
> scripts/checkpatch.pl will complain about this comment style.

Inherited from the code in PIIX3_reset. Will fix. Thanks.

Am 20.02.2013 18:24, schrieb David Woodhouse:
> On Wed, 2013-02-20 at 17:17 +0100, Andreas Färber wrote:
>>> +static void i440fx_reset(DeviceState *ds)
>>> +{
>>> +    PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, ds);
>>> +    PCII440FXState *d = DO_UPCAST(PCII440FXState, dev, dev);
>>
>> Please don't use DO_UPCAST() on QOM objects, there's QOM cast macros for
>> this, like PCI_DEVICE(). If there's one missing for PCII440FXState (I
>> think I only added them for the PHB itself?), it's two lines to add.
> 
> Would you care to elaborate on exactly what those two lines would look
> like? Being *completely* unfamiliar with qemu code, it took me long
> enough to work out the DO_UPCAST version.

static void i440fx_reset(DeviceState *dev)
{
    PCIDevice *pci = PCI_DEVICE(dev);
    PCII440FXState *d = I440FX_PCI_WHATEVER_CAST_MACRO_NAME(dev);

HTH,
Andreas

On Wed, 2013-02-20 at 18:42 +0100, Andreas Färber wrote:
> 
>     PCII440FXState *d = I440FX_PCI_WHATEVER_CAST_MACRO_NAME(dev);

Yeah, that's the easy bit. The bit that I'm vaguely confused by would be
something like

#define I440FX_PCI_WHATEVER_CAST_MACRO_NAME(obj) \
    OBJECT_CHECK(PCII440FXState, (obj), "i440FX") 

Where that "i440FX" should probably be turned into a macro of its own?

And then all the *existing* uses of DO_UPCAST should be fixed to use it
too? Such as in i440fx_write_config() ?

Il 20/02/2013 19:04, David Woodhouse ha scritto:
>> >     PCII440FXState *d = I440FX_PCI_WHATEVER_CAST_MACRO_NAME(dev);
> Yeah, that's the easy bit. The bit that I'm vaguely confused by would be
> something like
> 
> #define I440FX_PCI_WHATEVER_CAST_MACRO_NAME(obj) \
>     OBJECT_CHECK(PCII440FXState, (obj), "i440FX") 
> 
> Where that "i440FX" should probably be turned into a macro of its own?

Yes, TYPE_PCI_I440FX_STATE.

> And then all the *existing* uses of DO_UPCAST should be fixed to use it
> too? Such as in i440fx_write_config() ?

Yes, that would be helpful.  But I would not block your patch for the
casts, since i440FX is not yet converted.

Paolo

Am 20.02.2013 19:04, schrieb David Woodhouse:
> On Wed, 2013-02-20 at 18:42 +0100, Andreas Färber wrote:
>>
>>     PCII440FXState *d = I440FX_PCI_WHATEVER_CAST_MACRO_NAME(dev);
> 
> Yeah, that's the easy bit. The bit that I'm vaguely confused by would be
> something like
> 
> #define I440FX_PCI_WHATEVER_CAST_MACRO_NAME(obj) \
>     OBJECT_CHECK(PCII440FXState, (obj), "i440FX") 
> 
> Where that "i440FX" should probably be turned into a macro of its own?

Yes, a #define TYPE_SOMETHING_THAT_MAKES_SENSE "i440FX" a line above and
then use .name = TYPE_..., in the TypeInfo as well.

With proper names in both places obviously. :)

> And then all the *existing* uses of DO_UPCAST should be fixed to use it
> too? Such as in i440fx_write_config() ?

Outside the scope of this patch.

But if you're feeling like contributing, then generally there will be
uses to review and convert, yes. Every rule has its exceptions like hot
paths such as timers, interrupts and read/write, which usually just do
MyType *s = opaque;, but instance_init, initfn, (un)realize and reset
are clear candidates.

Cheers,
Andreas

Am 20.02.2013 19:14, schrieb Paolo Bonzini:
> Il 20/02/2013 19:04, David Woodhouse ha scritto:
>>>>     PCII440FXState *d = I440FX_PCI_WHATEVER_CAST_MACRO_NAME(dev);
>> Yeah, that's the easy bit. The bit that I'm vaguely confused by would be
>> something like
>>
>> #define I440FX_PCI_WHATEVER_CAST_MACRO_NAME(obj) \
>>     OBJECT_CHECK(PCII440FXState, (obj), "i440FX") 
>>
>> Where that "i440FX" should probably be turned into a macro of its own?
> 
> Yes, TYPE_PCI_I440FX_STATE.

We don't use _STATE names and it's not a PCI version of i440fx, as
template compare Raven PHB:
http://git.qemu.org/?p=qemu.git;a=blob;f=hw/prep_pci.c;h=52ee5d94019c880bd8915fe64f72922e0921f342;hb=HEAD

I'd suggest:

#define TYPE_I440FX_PCI_DEVICE "i440FX"
#define I440FX_PCI_DEVICE(obj) \
     OBJECT_CHECK(PCII440FXState, (obj), "i440FX")

foo(Object *obj)...
PCII440FXState *s = I440FX_PCI_DEVICE(obj);

TypeInfo ...
.name = TYPE_I440FX_PCI_DEVICE,

Note: Not in VMState though in case that matches textually.

Andreas

> 
>> And then all the *existing* uses of DO_UPCAST should be fixed to use it
>> too? Such as in i440fx_write_config() ?
> 
> Yes, that would be helpful.  But I would not block your patch for the
> casts, since i440FX is not yet converted.
> 
> Paolo
>

On Wed, 2013-02-20 at 19:20 +0100, Andreas Färber wrote:
> > And then all the *existing* uses of DO_UPCAST should be fixed to use it
> > too? Such as in i440fx_write_config() ?
> 
> Outside the scope of this patch.

Well, maybe. But I might as well start by cleaning up the rest of the
file that was setting me such a bad example. And then maybe I'll be
familiar enough with the current coding requirements that I'll be
capable of making the simple change I *want* without it being heckled to
death :)

On Wed, 2013-02-20 at 20:25 +0000, David Woodhouse wrote:
> On Wed, 2013-02-20 at 19:20 +0100, Andreas Färber wrote:
> > > And then all the *existing* uses of DO_UPCAST should be fixed to
> use it
> > > too? Such as in i440fx_write_config() ?
> > 
> > Outside the scope of this patch.
> 
> Well, maybe. But I might as well start by cleaning up the rest of the
> file that was setting me such a bad example. And then maybe I'll be
> familiar enough with the current coding requirements that I'll be
> capable of making the simple change I *want* without it being heckled
> to death :)

I've posted a patch series to fix most of this. What I *haven't* done is
the instances DO_UPCAST(PIIX3State, …) because there are *two* PIIX3
classes with the same PIIX3State, so any sanity checking on the base
class would have to cope with it being *either* "PIIX3" or "PIIX3-xen".

Admittedly, two of them are in the creation of each and we *do* know
which one we have. But two aren't...

 http://git.infradead.org/users/dwmw2/qemu.git (gitweb) or
 git://git.infradead.org/users/dwmw2/qemu.git