| Message ID | 20120405055113.31461.45258.stgit@bling.home |
|---|---|
| State | New |
| Headers | show |
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.williamson@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? > > PCI removability status (IO port 0xae0c-0xae0f, 4-byte access): > ----------------------------------------------- > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c > index 44d1423..6ee832a 100644 > --- a/hw/acpi_piix4.c > +++ b/hw/acpi_piix4.c > @@ -487,15 +487,11 @@ static uint32_t pci_down_read(void *opaque, uint32_t addr) > return val; > } > > -static uint32_t pciej_read(void *opaque, uint32_t addr) > -{ > - PIIX4_DPRINTF("pciej read %x\n", addr); > - return 0; > -} > - what will happen if bios tries to read from reg? > static void pciej_write(void *opaque, uint32_t addr, uint32_t val) > { > - BusState *bus = opaque; > + PIIX4PMState *s = opaque; > + PCIDevice *dev =&s->dev; > + BusState *bus = qdev_get_parent_bus(&dev->qdev); > DeviceState *qdev, *next; > int slot = ffs(val) - 1; > > @@ -507,7 +503,6 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val) > } > } > > - > PIIX4_DPRINTF("pciej write %x<== %d\n", addr, val); > } > > @@ -547,8 +542,7 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s) > register_ioport_read(PCI_UP_BASE, 4, 4, pci_up_read, s); > register_ioport_read(PCI_DOWN_BASE, 4, 4, pci_down_read, s); > > - register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, bus); > - register_ioport_read(PCI_EJ_BASE, 4, 4, pciej_read, bus); > + register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s); > > register_ioport_write(PCI_RMV_BASE, 4, 4, pcirmv_write, s); > register_ioport_read(PCI_RMV_BASE, 4, 4, pcirmv_read, s); >
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.williamson@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. I think it is 'Guest should never read this register, in practice 0 is returned'. > > > > PCI removability status (IO port 0xae0c-0xae0f, 4-byte access): > > ----------------------------------------------- > >diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c > >index 44d1423..6ee832a 100644 > >--- a/hw/acpi_piix4.c > >+++ b/hw/acpi_piix4.c > >@@ -487,15 +487,11 @@ static uint32_t pci_down_read(void *opaque, uint32_t addr) > > return val; > > } > > > >-static uint32_t pciej_read(void *opaque, uint32_t addr) > >-{ > >- PIIX4_DPRINTF("pciej read %x\n", addr); > >- return 0; > >-} > >- > what will happen if bios tries to read from reg? I think it currently gets 0. > > static void pciej_write(void *opaque, uint32_t addr, uint32_t val) > > { > >- BusState *bus = opaque; > >+ PIIX4PMState *s = opaque; > >+ PCIDevice *dev =&s->dev; > >+ BusState *bus = qdev_get_parent_bus(&dev->qdev); > > DeviceState *qdev, *next; > > int slot = ffs(val) - 1; > > > >@@ -507,7 +503,6 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val) > > } > > } > > > >- > > PIIX4_DPRINTF("pciej write %x<== %d\n", addr, val); > > } > > > >@@ -547,8 +542,7 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s) > > register_ioport_read(PCI_UP_BASE, 4, 4, pci_up_read, s); > > register_ioport_read(PCI_DOWN_BASE, 4, 4, pci_down_read, s); > > > >- register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, bus); > >- register_ioport_read(PCI_EJ_BASE, 4, 4, pciej_read, bus); > >+ register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s); > > > > register_ioport_write(PCI_RMV_BASE, 4, 4, pcirmv_write, s); > > register_ioport_read(PCI_RMV_BASE, 4, 4, pcirmv_read, s); > > > > -- > ----- > Igor
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.williamson@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. -- Gleb.
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.williamson@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. 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. > -- > Gleb.
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.williamson@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. Our lives will be easier if we will leave undefined behaviour undefined. -- Gleb.
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.williamson@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. > 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.' 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. ' 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. > -- > Gleb.
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.williamson@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. > 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.
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.williamson@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. > > 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.
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.williamson@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. -- Gleb.
On Thu, 2012-04-05 at 10:21 +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.williamson@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? Oops, copy/paste. Will fix. > > > > PCI removability status (IO port 0xae0c-0xae0f, 4-byte access): > > ----------------------------------------------- > > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c > > index 44d1423..6ee832a 100644 > > --- a/hw/acpi_piix4.c > > +++ b/hw/acpi_piix4.c > > @@ -487,15 +487,11 @@ static uint32_t pci_down_read(void *opaque, uint32_t addr) > > return val; > > } > > > > -static uint32_t pciej_read(void *opaque, uint32_t addr) > > -{ > > - PIIX4_DPRINTF("pciej read %x\n", addr); > > - return 0; > > -} > > - > what will happen if bios tries to read from reg? -1. Thanks, Alex > > static void pciej_write(void *opaque, uint32_t addr, uint32_t val) > > { > > - BusState *bus = opaque; > > + PIIX4PMState *s = opaque; > > + PCIDevice *dev =&s->dev; > > + BusState *bus = qdev_get_parent_bus(&dev->qdev); > > DeviceState *qdev, *next; > > int slot = ffs(val) - 1; > > > > @@ -507,7 +503,6 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val) > > } > > } > > > > - > > PIIX4_DPRINTF("pciej write %x<== %d\n", addr, val); > > } > > > > @@ -547,8 +542,7 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s) > > register_ioport_read(PCI_UP_BASE, 4, 4, pci_up_read, s); > > register_ioport_read(PCI_DOWN_BASE, 4, 4, pci_down_read, s); > > > > - register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, bus); > > - register_ioport_read(PCI_EJ_BASE, 4, 4, pciej_read, bus); > > + register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s); > > > > register_ioport_write(PCI_RMV_BASE, 4, 4, pcirmv_write, s); > > register_ioport_read(PCI_RMV_BASE, 4, 4, pcirmv_read, s); > > >
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.williamson@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
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.williamson@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?
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. PCI removability status (IO port 0xae0c-0xae0f, 4-byte access): ----------------------------------------------- diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c index 44d1423..6ee832a 100644 --- a/hw/acpi_piix4.c +++ b/hw/acpi_piix4.c @@ -487,15 +487,11 @@ static uint32_t pci_down_read(void *opaque, uint32_t addr) return val; } -static uint32_t pciej_read(void *opaque, uint32_t addr) -{ - PIIX4_DPRINTF("pciej read %x\n", addr); - return 0; -} - static void pciej_write(void *opaque, uint32_t addr, uint32_t val) { - BusState *bus = opaque; + PIIX4PMState *s = opaque; + PCIDevice *dev = &s->dev; + BusState *bus = qdev_get_parent_bus(&dev->qdev); DeviceState *qdev, *next; int slot = ffs(val) - 1; @@ -507,7 +503,6 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val) } } - PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val); } @@ -547,8 +542,7 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s) register_ioport_read(PCI_UP_BASE, 4, 4, pci_up_read, s); register_ioport_read(PCI_DOWN_BASE, 4, 4, pci_down_read, s); - register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, bus); - register_ioport_read(PCI_EJ_BASE, 4, 4, pciej_read, bus); + register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s); register_ioport_write(PCI_RMV_BASE, 4, 4, pcirmv_write, s); register_ioport_read(PCI_RMV_BASE, 4, 4, pcirmv_read, s);
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.williamson@redhat.com> --- docs/specs/acpi_pci_hotplug.txt | 2 +- hw/acpi_piix4.c | 14 ++++---------- 2 files changed, 5 insertions(+), 11 deletions(-)