Re: [Qemu-devel] [PATCH 2/5] acpi_piix4: Only allow writes to PCI hotplug eject register

[Gleb Natapov]

On Thu, Apr 05, 2012 at 01:20:03PM +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 05, 2012 at 01:08:06PM +0300, Gleb Natapov wrote:
> > On Thu, Apr 05, 2012 at 12:53:55PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Apr 05, 2012 at 12:40:00PM +0300, Gleb Natapov wrote:
> > > > On Thu, Apr 05, 2012 at 12:37:01PM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Apr 05, 2012 at 12:12:37PM +0300, Gleb Natapov wrote:
> > > > > > On Thu, Apr 05, 2012 at 12:04:44PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Apr 05, 2012 at 10:21:18AM +0200, Igor Mammedov wrote:
> > > > > > > > On 04/05/2012 07:51 AM, Alex Williamson wrote:
> > > > > > > > >This is never read.  We can also derive bus from the write 
> > > > > > > > >handler,
> > > > > > > > >making this more inline with the other callbacks.  Note that
> > > > > > > > >pciej_write was actually called with (PCIBus *)dev->bus, which 
> > > > > > > > >is
> > > > > > > > >cast as a void* allowing us to pretend it's a BusState*.  Fix 
> > > > > > > > >this
> > > > > > > > >so we don't depend on the BusState location within PCIBus.
> > > > > > > > >
> > > > > > > > >Signed-off-by: Alex Williamson<address@hidden>
> > > > > > > > >---
> > > > > > > > >
> > > > > > > > >  docs/specs/acpi_pci_hotplug.txt |    2 +-
> > > > > > > > >  hw/acpi_piix4.c                 |   14 ++++----------
> > > > > > > > >  2 files changed, 5 insertions(+), 11 deletions(-)
> > > > > > > > >
> > > > > > > > >diff --git a/docs/specs/acpi_pci_hotplug.txt 
> > > > > > > > >b/docs/specs/acpi_pci_hotplug.txt
> > > > > > > > >index 1e2c8a2..1e61d19 100644
> > > > > > > > >--- a/docs/specs/acpi_pci_hotplug.txt
> > > > > > > > >+++ b/docs/specs/acpi_pci_hotplug.txt
> > > > > > > > >@@ -28,7 +28,7 @@ PCI device eject (IO port 0xae08-0xae0b, 
> > > > > > > > >4-byte access):
> > > > > > > > >  ----------------------------------------
> > > > > > > > >
> > > > > > > > >  Used by ACPI BIOS _EJ0 method to request device removal. One 
> > > > > > > > > bit per slot.
> > > > > > > > >-Reads return 0.
> > > > > > > > >+Read-only.
> > > > > > > > Write-only perhaps?
> > > > > > > 
> > > > > > > Yes, let's also specify what happens in practice.
> > > > > > No we shouldn't.
> > > > > > 
> > > > > > > I think it is 'Guest should never read this register, in practice
> > > > > > > 0 is returned'.
> > > > > > > 
> > > > > > In practice kitten die for each read. Unspecified behaviour is
> > > > > > unspecified.
> > > > > 
> > > > > 
> > > > > Why, what are you worried about? I just want to document what we do.
> > > > > 
> > > > You are making undefined behaviour to be defined one.
> > > > 
> > > > > The reason I want to specify behaviour on read is because down
> > > > > the road we might want to return something here. Our lives
> > > > > will be easier if we have a document which we can read
> > > > > and figure out what old qemu did.
> > > > > 
> > > > You can do all that only if behaviour is undefined. If it is defined you
> > > > can't change it.
> > > 
> > > We are doing just that constantly, just be careful.
> > > Documenting what happens will make it easier.
> > > 
> > You keeping saying that it keeps not making sense to me.
> > 
> > > > Our lives will be easier if we will leave undefined
> > > > behaviour undefined.
> > > 
> > > So yes live it undefined for guests but document what happens
> > > for ourselves. Let's make it explicit, say
> > > 'current implementation returns 0 this can
> > >  change at any time without notice.'
> > > 
> > Current behaviour is documented in the current code. If the purpose of
> > the document is to define ABI for a guest then the only thing that make
> > sense to specify is that behaviour is undefined. Actually saying
> > "register is write only" is enough for everyone to understand that reads
> > are undefined. Look at HW specs. There is no "in practice read will do
> > this and that" near write only register description.
> 
> This is because hardware is hardware, you do not
> keep developing it. We keep editing what our hardware
> does so it makes sense documenting what it does
> even if it is not guest visible.
> 
Oh yes! Intel stopped developing cpu just after 8008 :)

> > > I want to go further. For up/down I would like to
> > > document pas behaviour as well
> > > 'past implementations made the registers
> > > read-write, writing there would clobber
> > > outstanding hotplug requests.
> > > current implementation makes the register read-only,
> > > writes are discarded.
> > > '
> > Documenting things that were undocumented and were used make sense,
> > but then you can't change how they behave if you believe that there is
> > a code that relies on old behaviour.
> > > 
> > > Just undefined is vague. If someone bothered
> > > documenting the current undefined behavour
> > > with registers being writeable instead of
> > > undefined, then people would detect this
> > > as a bug and this would have been fixed.
> > > 
> > I have no idea what are you trying to say here.
> 
> I am trying to say that besides guest visible behaviour I want
> to document, separately, non guest visible behaviour.
Document it some other place. Hmm, how about doing in the code?

> For example what registers *that guest should never read*
> actually do on read.
> 
Looks at the code. Or are you going to describe everything QEMU does
internally in plane english? If not, why making an exception for acpi
code?

> This is not different from code comments really,
> I just want them in a central place because this
> is guest triggerable.
This is not guest triggerable. We do not care about guests that triggers
it and we state it explicitly in documentation that is targeted for
guest developers.

> Why is this good? Makes it easier to do things like security
> audit, or develop new features in a compatible way.
> 
I do not see how it helps for both of those. I do see how it harmful for
the later.

Look the only reason this spec exists is because there is no real HW we
emulate here. We do not write such specs for HW that have spec already.
So we design our own HW and we document it. The only reason I will look
at this spec is to check that my changes to the code does not modify
guest visible behaviour in a way that guest cannot cope with. I will not
look at the spec to check what current code does, it does not make
sense. In short the spec should describe what code should do, not what
it does and as such there is not place for "current code does that"
there.

--
                        Gleb.