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

[Gleb Natapov]

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.

> 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.

--
                        Gleb.