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<alex.william...@redhat.com> > > > > > > > > > > >--- > > > > > > > > > > > > > > > > > > > > > > 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
