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 09:12:31AM -0600, Alex Williamson wrote:
> On Thu, 2012-04-05 at 13:48 +0300, Gleb Natapov wrote:
> > 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.
> 
> Wow, missed quite a discussion on this overnight.  Based on the patch,
> obviously I agree that we should not be trying to define undefined
> behavior.  Up/down was clearly broken.  Any kind of read, modify, write
> from the guest can race with qemu, so I think it's justified to change
> the behavior.  The hotplug capable register was always read-only, so I'm
> really not changing anything there except clearly defining it.  The
> eject register supports read for absolutely no reason, so I'd like to
> remove that support before anyone actually depends on it and then we
> have to carry a return 0 read function on forever.  Alternatively, we
> could take this opportunity to define the read side of the eject
> register as a hotplug implementation version or features register.  Any
> preference for that?  Thanks,
> 
> Alex

I prefer it to be a version or features register.
As we are compatible with old BIOS we can keep it
at 0 for now, exactly as it was, right?


-- 
MST