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

[Michael S. Tsirkin]

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.

> > 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.
For example what registers *that guest should never read*
actually do on read.

This is not different from code comments really,
I just want them in a central place because this
is guest triggerable.
Why is this good? Makes it easier to do things like security
audit, or develop new features in a compatible way.


> --
>                       Gleb.