* [Qemu-devel] [PATCH 0/3] hw/pcie: better hotplug/hotunplug support
@ 2014-06-19 13:52 Marcel Apfelbaum
2014-06-19 13:52 ` [Qemu-devel] [PATCH 1/3] hw/pcie: corrected a debug message Marcel Apfelbaum
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Marcel Apfelbaum @ 2014-06-19 13:52 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
Hotplug triggers both 'present detect change' and
'attention button pressed'.
Hotunplug starts by triggering 'attention button pressed',
then waits for the OS to power off the device and only
then detaches it.
patch 1/3: trivial debug message fix
patch 2/3: enable 'power controller' to receive power events from guests
patch 3/3: the actual hotplug/hotunplug implementation.
Tested with Linux and Windows guests and with an e1000 with "PCIe" capability.
Notes:
Windows requires devices to be pci express in order to enable
hotplug functionality, so we need to think about converting
virtio devices to pci express.
Linux outputs a "Surprise Removal/Addition" info message because
we trigger 2 events in the same time, however this warning
can be disregarded or a kernel patch submitted for our scenario.
Marcel Apfelbaum (3):
hw/pcie: corrected a debug message
hw/pcie: implement power controller functionality
hw/pcie: better hotplug/hotunplug support
hw/pci/pcie.c | 37 ++++++++++++++++++++++++++++++-------
include/hw/pci/pcie_regs.h | 2 ++
2 files changed, 32 insertions(+), 7 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH 1/3] hw/pcie: corrected a debug message 2014-06-19 13:52 [Qemu-devel] [PATCH 0/3] hw/pcie: better hotplug/hotunplug support Marcel Apfelbaum @ 2014-06-19 13:52 ` Marcel Apfelbaum 2014-06-19 13:52 ` [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality Marcel Apfelbaum 2014-06-19 13:52 ` [Qemu-devel] [PATCH 3/3] hw/pcie: better hotplug/hotunplug support Marcel Apfelbaum 2 siblings, 0 replies; 16+ messages in thread From: Marcel Apfelbaum @ 2014-06-19 13:52 UTC (permalink / raw) To: qemu-devel; +Cc: mst Trivial issue, discovered while debugging. Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- hw/pci/pcie.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 02cde6f..ae92f00 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -224,7 +224,7 @@ static void pcie_cap_slot_hotplug_common(PCIDevice *hotplug_dev, *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap; uint16_t sltsta = pci_get_word(*exp_cap + PCI_EXP_SLTSTA); - PCIE_DEV_PRINTF(PCI_DEVICE(dev), "hotplug state: %d\n", state); + PCIE_DEV_PRINTF(PCI_DEVICE(dev), "hotplug state: 0x%x\n", sltsta); if (sltsta & PCI_EXP_SLTSTA_EIS) { /* the slot is electromechanically locked. * This error is propagated up to qdev and then to HMP/QMP. -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality 2014-06-19 13:52 [Qemu-devel] [PATCH 0/3] hw/pcie: better hotplug/hotunplug support Marcel Apfelbaum 2014-06-19 13:52 ` [Qemu-devel] [PATCH 1/3] hw/pcie: corrected a debug message Marcel Apfelbaum @ 2014-06-19 13:52 ` Marcel Apfelbaum 2014-06-19 14:39 ` Michael S. Tsirkin 2014-06-19 13:52 ` [Qemu-devel] [PATCH 3/3] hw/pcie: better hotplug/hotunplug support Marcel Apfelbaum 2 siblings, 1 reply; 16+ messages in thread From: Marcel Apfelbaum @ 2014-06-19 13:52 UTC (permalink / raw) To: qemu-devel; +Cc: mst It is needed by hot-unplug in order to get an indication from the OS when the device can be physically detached. Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- hw/pci/pcie.c | 15 ++++++++++++++- include/hw/pci/pcie_regs.h | 2 ++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index ae92f00..f8bf515 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -292,16 +292,19 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) PCI_EXP_SLTCAP_HPC | PCI_EXP_SLTCAP_PIP | PCI_EXP_SLTCAP_AIP | + PCI_EXP_SLTCAP_PCP | PCI_EXP_SLTCAP_ABP); pci_word_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCTL, PCI_EXP_SLTCTL_PIC | + PCI_EXP_SLTCTL_PCC | PCI_EXP_SLTCTL_AIC); pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCTL, PCI_EXP_SLTCTL_PIC_OFF | PCI_EXP_SLTCTL_AIC_OFF); pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL, PCI_EXP_SLTCTL_PIC | + PCI_EXP_SLTCTL_PCC | PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE | @@ -327,21 +330,31 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) void pcie_cap_slot_reset(PCIDevice *dev) { uint8_t *exp_cap = dev->config + dev->exp.exp_cap; + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; + int pic; + + pic = slot_dev ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF; PCIE_DEV_PRINTF(dev, "reset\n"); pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL, PCI_EXP_SLTCTL_EIC | PCI_EXP_SLTCTL_PIC | + PCI_EXP_SLTCTL_PCC | PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_PDCE | PCI_EXP_SLTCTL_ABPE); pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, - PCI_EXP_SLTCTL_PIC_OFF | + pic | PCI_EXP_SLTCTL_AIC_OFF); + if (!slot_dev) { + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, + PCI_EXP_SLTCTL_PCC); + } + pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_EIS |/* on reset, the lock is released */ diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h index 4d123d9..652d9fc 100644 --- a/include/hw/pci/pcie_regs.h +++ b/include/hw/pci/pcie_regs.h @@ -57,6 +57,8 @@ #define PCI_EXP_SLTCTL_PIC_SHIFT (ffs(PCI_EXP_SLTCTL_PIC) - 1) #define PCI_EXP_SLTCTL_PIC_OFF \ (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_PIC_SHIFT) +#define PCI_EXP_SLTCTL_PIC_ON \ + (PCI_EXP_SLTCTL_IND_ON << PCI_EXP_SLTCTL_PIC_SHIFT) #define PCI_EXP_SLTCTL_SUPPORTED \ (PCI_EXP_SLTCTL_ABPE | \ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality 2014-06-19 13:52 ` [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality Marcel Apfelbaum @ 2014-06-19 14:39 ` Michael S. Tsirkin 2014-06-19 20:56 ` Paolo Bonzini 2014-06-22 10:47 ` Marcel Apfelbaum 0 siblings, 2 replies; 16+ messages in thread From: Michael S. Tsirkin @ 2014-06-19 14:39 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: qemu-devel On Thu, Jun 19, 2014 at 04:52:20PM +0300, Marcel Apfelbaum wrote: > It is needed by hot-unplug in order to get an indication > from the OS when the device can be physically detached. > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > --- > hw/pci/pcie.c | 15 ++++++++++++++- > include/hw/pci/pcie_regs.h | 2 ++ > 2 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index ae92f00..f8bf515 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -292,16 +292,19 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) > PCI_EXP_SLTCAP_HPC | > PCI_EXP_SLTCAP_PIP | > PCI_EXP_SLTCAP_AIP | > + PCI_EXP_SLTCAP_PCP | > PCI_EXP_SLTCAP_ABP); > > pci_word_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCTL, > PCI_EXP_SLTCTL_PIC | > + PCI_EXP_SLTCTL_PCC | > PCI_EXP_SLTCTL_AIC); > pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCTL, > PCI_EXP_SLTCTL_PIC_OFF | > PCI_EXP_SLTCTL_AIC_OFF); > pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL, > PCI_EXP_SLTCTL_PIC | > + PCI_EXP_SLTCTL_PCC | > PCI_EXP_SLTCTL_AIC | > PCI_EXP_SLTCTL_HPIE | > PCI_EXP_SLTCTL_CCIE | Need to disable for compat types? > @@ -327,21 +330,31 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) > void pcie_cap_slot_reset(PCIDevice *dev) > { > uint8_t *exp_cap = dev->config + dev->exp.exp_cap; > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; What does this mean? Downstream port? A switch can have multiple downstream ports at any slot #. > + int pic; uint16_t please. > + > + pic = slot_dev ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF; > > PCIE_DEV_PRINTF(dev, "reset\n"); > > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL, > PCI_EXP_SLTCTL_EIC | > PCI_EXP_SLTCTL_PIC | > + PCI_EXP_SLTCTL_PCC | > PCI_EXP_SLTCTL_AIC | > PCI_EXP_SLTCTL_HPIE | > PCI_EXP_SLTCTL_CCIE | > PCI_EXP_SLTCTL_PDCE | > PCI_EXP_SLTCTL_ABPE); > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, > - PCI_EXP_SLTCTL_PIC_OFF | > + pic | > PCI_EXP_SLTCTL_AIC_OFF); > > + if (!slot_dev) { > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, > + PCI_EXP_SLTCTL_PCC); I dislike it when we clear bits then set them back. Please just add else here. > + } > + Need to disable for compat types? > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > PCI_EXP_SLTSTA_EIS |/* on reset, > the lock is released */ > diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h > index 4d123d9..652d9fc 100644 > --- a/include/hw/pci/pcie_regs.h > +++ b/include/hw/pci/pcie_regs.h > @@ -57,6 +57,8 @@ > #define PCI_EXP_SLTCTL_PIC_SHIFT (ffs(PCI_EXP_SLTCTL_PIC) - 1) > #define PCI_EXP_SLTCTL_PIC_OFF \ > (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_PIC_SHIFT) > +#define PCI_EXP_SLTCTL_PIC_ON \ > + (PCI_EXP_SLTCTL_IND_ON << PCI_EXP_SLTCTL_PIC_SHIFT) > > #define PCI_EXP_SLTCTL_SUPPORTED \ > (PCI_EXP_SLTCTL_ABPE | \ > -- > 1.8.3.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality 2014-06-19 14:39 ` Michael S. Tsirkin @ 2014-06-19 20:56 ` Paolo Bonzini 2014-06-22 10:47 ` Marcel Apfelbaum 1 sibling, 0 replies; 16+ messages in thread From: Paolo Bonzini @ 2014-06-19 20:56 UTC (permalink / raw) To: Michael S. Tsirkin, Marcel Apfelbaum; +Cc: qemu-devel Il 19/06/2014 16:39, Michael S. Tsirkin ha scritto: > > pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL, > > PCI_EXP_SLTCTL_PIC | > > + PCI_EXP_SLTCTL_PCC | > > PCI_EXP_SLTCTL_AIC | > > PCI_EXP_SLTCTL_HPIE | > > PCI_EXP_SLTCTL_CCIE | > > Need to disable for compat types? Do we support PCI-to-PCIe bridges? Otherwise, AHCI breaks migration and the only PCIe machine always has an AHCI controller, so I'd say no. Paolo ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality 2014-06-19 14:39 ` Michael S. Tsirkin 2014-06-19 20:56 ` Paolo Bonzini @ 2014-06-22 10:47 ` Marcel Apfelbaum 2014-06-22 10:52 ` Michael S. Tsirkin 1 sibling, 1 reply; 16+ messages in thread From: Marcel Apfelbaum @ 2014-06-22 10:47 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Thu, 2014-06-19 at 17:39 +0300, Michael S. Tsirkin wrote: > On Thu, Jun 19, 2014 at 04:52:20PM +0300, Marcel Apfelbaum wrote: > > It is needed by hot-unplug in order to get an indication > > from the OS when the device can be physically detached. > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > --- > > hw/pci/pcie.c | 15 ++++++++++++++- > > include/hw/pci/pcie_regs.h | 2 ++ > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > index ae92f00..f8bf515 100644 > > --- a/hw/pci/pcie.c > > +++ b/hw/pci/pcie.c > > @@ -292,16 +292,19 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) > > PCI_EXP_SLTCAP_HPC | > > PCI_EXP_SLTCAP_PIP | > > PCI_EXP_SLTCAP_AIP | > > + PCI_EXP_SLTCAP_PCP | > > PCI_EXP_SLTCAP_ABP); > > > > pci_word_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCTL, > > PCI_EXP_SLTCTL_PIC | > > + PCI_EXP_SLTCTL_PCC | > > PCI_EXP_SLTCTL_AIC); > > pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCTL, > > PCI_EXP_SLTCTL_PIC_OFF | > > PCI_EXP_SLTCTL_AIC_OFF); > > pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL, > > PCI_EXP_SLTCTL_PIC | > > + PCI_EXP_SLTCTL_PCC | > > PCI_EXP_SLTCTL_AIC | > > PCI_EXP_SLTCTL_HPIE | > > PCI_EXP_SLTCTL_CCIE | > > Need to disable for compat types? Does Paolo's explanation answer your question? > > > > @@ -327,21 +330,31 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) > > void pcie_cap_slot_reset(PCIDevice *dev) > > { > > uint8_t *exp_cap = dev->config + dev->exp.exp_cap; > > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > > What does this mean? > Downstream port? Yes > A switch can have multiple downstream ports at any slot #. It doesn't matter how many devices are under this slot, we need only one to power up the slot. The question here was "Do we have at least one device attahc to slot? If yes, power up the slot." > > > + int pic; > > uint16_t please. Sure, > > > + > > + pic = slot_dev ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF; > > > > PCIE_DEV_PRINTF(dev, "reset\n"); > > > > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL, > > PCI_EXP_SLTCTL_EIC | > > PCI_EXP_SLTCTL_PIC | > > + PCI_EXP_SLTCTL_PCC | > > PCI_EXP_SLTCTL_AIC | > > PCI_EXP_SLTCTL_HPIE | > > PCI_EXP_SLTCTL_CCIE | > > PCI_EXP_SLTCTL_PDCE | > > PCI_EXP_SLTCTL_ABPE); > > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, > > - PCI_EXP_SLTCTL_PIC_OFF | > > + pic | > > PCI_EXP_SLTCTL_AIC_OFF); > > > > + if (!slot_dev) { > > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, > > + PCI_EXP_SLTCTL_PCC); > > I dislike it when we clear bits then set them back. > Please just add else here. Sure, > > > + } > > + > > Need to disable for compat types? As above, Thanks for the review, Marcel > > > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > PCI_EXP_SLTSTA_EIS |/* on reset, > > the lock is released */ > > diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h > > index 4d123d9..652d9fc 100644 > > --- a/include/hw/pci/pcie_regs.h > > +++ b/include/hw/pci/pcie_regs.h > > @@ -57,6 +57,8 @@ > > #define PCI_EXP_SLTCTL_PIC_SHIFT (ffs(PCI_EXP_SLTCTL_PIC) - 1) > > #define PCI_EXP_SLTCTL_PIC_OFF \ > > (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_PIC_SHIFT) > > +#define PCI_EXP_SLTCTL_PIC_ON \ > > + (PCI_EXP_SLTCTL_IND_ON << PCI_EXP_SLTCTL_PIC_SHIFT) > > > > #define PCI_EXP_SLTCTL_SUPPORTED \ > > (PCI_EXP_SLTCTL_ABPE | \ > > -- > > 1.8.3.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality 2014-06-22 10:47 ` Marcel Apfelbaum @ 2014-06-22 10:52 ` Michael S. Tsirkin 2014-06-22 11:11 ` Michael S. Tsirkin 0 siblings, 1 reply; 16+ messages in thread From: Michael S. Tsirkin @ 2014-06-22 10:52 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: qemu-devel On Sun, Jun 22, 2014 at 01:47:24PM +0300, Marcel Apfelbaum wrote: > On Thu, 2014-06-19 at 17:39 +0300, Michael S. Tsirkin wrote: > > On Thu, Jun 19, 2014 at 04:52:20PM +0300, Marcel Apfelbaum wrote: > > > It is needed by hot-unplug in order to get an indication > > > from the OS when the device can be physically detached. > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > --- > > > hw/pci/pcie.c | 15 ++++++++++++++- > > > include/hw/pci/pcie_regs.h | 2 ++ > > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > index ae92f00..f8bf515 100644 > > > --- a/hw/pci/pcie.c > > > +++ b/hw/pci/pcie.c > > > @@ -292,16 +292,19 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) > > > PCI_EXP_SLTCAP_HPC | > > > PCI_EXP_SLTCAP_PIP | > > > PCI_EXP_SLTCAP_AIP | > > > + PCI_EXP_SLTCAP_PCP | > > > PCI_EXP_SLTCAP_ABP); > > > > > > pci_word_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCTL, > > > PCI_EXP_SLTCTL_PIC | > > > + PCI_EXP_SLTCTL_PCC | > > > PCI_EXP_SLTCTL_AIC); > > > pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCTL, > > > PCI_EXP_SLTCTL_PIC_OFF | > > > PCI_EXP_SLTCTL_AIC_OFF); > > > pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL, > > > PCI_EXP_SLTCTL_PIC | > > > + PCI_EXP_SLTCTL_PCC | > > > PCI_EXP_SLTCTL_AIC | > > > PCI_EXP_SLTCTL_HPIE | > > > PCI_EXP_SLTCTL_CCIE | > > > > Need to disable for compat types? > Does Paolo's explanation answer your question? Kind of hacky - we do have compat work for q35. So I'd prefer consistency. > > > > > > > @@ -327,21 +330,31 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) > > > void pcie_cap_slot_reset(PCIDevice *dev) > > > { > > > uint8_t *exp_cap = dev->config + dev->exp.exp_cap; > > > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > > > > What does this mean? > > Downstream port? > Yes well it's not really clear, needs a comment. > > A switch can have multiple downstream ports at any slot #. > It doesn't matter how many devices are under this slot, we need only > one to power up the slot. The question here was "Do we have at least > one device attahc to slot? If yes, power up the slot." You need a loop for that I think. There's no guarantee the device is at devfn=0. > > > > > + int pic; > > > > uint16_t please. > Sure, > > > > > > + > > > + pic = slot_dev ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF; > > > > > > PCIE_DEV_PRINTF(dev, "reset\n"); > > > > > > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL, > > > PCI_EXP_SLTCTL_EIC | > > > PCI_EXP_SLTCTL_PIC | > > > + PCI_EXP_SLTCTL_PCC | > > > PCI_EXP_SLTCTL_AIC | > > > PCI_EXP_SLTCTL_HPIE | > > > PCI_EXP_SLTCTL_CCIE | > > > PCI_EXP_SLTCTL_PDCE | > > > PCI_EXP_SLTCTL_ABPE); > > > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, > > > - PCI_EXP_SLTCTL_PIC_OFF | > > > + pic | > > > PCI_EXP_SLTCTL_AIC_OFF); > > > > > > + if (!slot_dev) { > > > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, > > > + PCI_EXP_SLTCTL_PCC); > > > > I dislike it when we clear bits then set them back. > > Please just add else here. > Sure, > > > > > > + } > > > + > > > > Need to disable for compat types? > As above, > > Thanks for the review, > Marcel > > > > > > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > PCI_EXP_SLTSTA_EIS |/* on reset, > > > the lock is released */ > > > diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h > > > index 4d123d9..652d9fc 100644 > > > --- a/include/hw/pci/pcie_regs.h > > > +++ b/include/hw/pci/pcie_regs.h > > > @@ -57,6 +57,8 @@ > > > #define PCI_EXP_SLTCTL_PIC_SHIFT (ffs(PCI_EXP_SLTCTL_PIC) - 1) > > > #define PCI_EXP_SLTCTL_PIC_OFF \ > > > (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_PIC_SHIFT) > > > +#define PCI_EXP_SLTCTL_PIC_ON \ > > > + (PCI_EXP_SLTCTL_IND_ON << PCI_EXP_SLTCTL_PIC_SHIFT) > > > > > > #define PCI_EXP_SLTCTL_SUPPORTED \ > > > (PCI_EXP_SLTCTL_ABPE | \ > > > -- > > > 1.8.3.1 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality 2014-06-22 10:52 ` Michael S. Tsirkin @ 2014-06-22 11:11 ` Michael S. Tsirkin 2014-06-22 11:17 ` Marcel Apfelbaum 0 siblings, 1 reply; 16+ messages in thread From: Michael S. Tsirkin @ 2014-06-22 11:11 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: qemu-devel On Sun, Jun 22, 2014 at 01:52:46PM +0300, Michael S. Tsirkin wrote: > On Sun, Jun 22, 2014 at 01:47:24PM +0300, Marcel Apfelbaum wrote: > > On Thu, 2014-06-19 at 17:39 +0300, Michael S. Tsirkin wrote: > > > On Thu, Jun 19, 2014 at 04:52:20PM +0300, Marcel Apfelbaum wrote: > > > > It is needed by hot-unplug in order to get an indication > > > > from the OS when the device can be physically detached. > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > --- > > > > hw/pci/pcie.c | 15 ++++++++++++++- > > > > include/hw/pci/pcie_regs.h | 2 ++ > > > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > > index ae92f00..f8bf515 100644 > > > > --- a/hw/pci/pcie.c > > > > +++ b/hw/pci/pcie.c > > > > @@ -292,16 +292,19 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) > > > > PCI_EXP_SLTCAP_HPC | > > > > PCI_EXP_SLTCAP_PIP | > > > > PCI_EXP_SLTCAP_AIP | > > > > + PCI_EXP_SLTCAP_PCP | > > > > PCI_EXP_SLTCAP_ABP); > > > > > > > > pci_word_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCTL, > > > > PCI_EXP_SLTCTL_PIC | > > > > + PCI_EXP_SLTCTL_PCC | > > > > PCI_EXP_SLTCTL_AIC); > > > > pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCTL, > > > > PCI_EXP_SLTCTL_PIC_OFF | > > > > PCI_EXP_SLTCTL_AIC_OFF); > > > > pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL, > > > > PCI_EXP_SLTCTL_PIC | > > > > + PCI_EXP_SLTCTL_PCC | > > > > PCI_EXP_SLTCTL_AIC | > > > > PCI_EXP_SLTCTL_HPIE | > > > > PCI_EXP_SLTCTL_CCIE | > > > > > > Need to disable for compat types? > > Does Paolo's explanation answer your question? > > Kind of hacky - we do have compat work for > q35. So I'd prefer consistency. > > > > > > > > > > > @@ -327,21 +330,31 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) > > > > void pcie_cap_slot_reset(PCIDevice *dev) > > > > { > > > > uint8_t *exp_cap = dev->config + dev->exp.exp_cap; > > > > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > > > > > > What does this mean? > > > Downstream port? > > Yes > > well it's not really clear, needs a comment. > > > > A switch can have multiple downstream ports at any slot #. > > It doesn't matter how many devices are under this slot, we need only > > one to power up the slot. The question here was "Do we have at least > > one device attahc to slot? If yes, power up the slot." > > You need a loop for that I think. There's no guarantee the > device is at devfn=0. Unless something guarantees this is a downstream port, if yes maybe add an assert? slot_dev is also not a very good variable name. How about bool populated = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; ? This is all you care about ... And maybe add a comment /* Downstream ports enforce device number 0. */ > > > > > > > + int pic; > > > > > > uint16_t please. > > Sure, > > > > > > > > > + > > > > + pic = slot_dev ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF; > > > > > > > > PCIE_DEV_PRINTF(dev, "reset\n"); > > > > > > > > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL, > > > > PCI_EXP_SLTCTL_EIC | > > > > PCI_EXP_SLTCTL_PIC | > > > > + PCI_EXP_SLTCTL_PCC | > > > > PCI_EXP_SLTCTL_AIC | > > > > PCI_EXP_SLTCTL_HPIE | > > > > PCI_EXP_SLTCTL_CCIE | > > > > PCI_EXP_SLTCTL_PDCE | > > > > PCI_EXP_SLTCTL_ABPE); > > > > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, > > > > - PCI_EXP_SLTCTL_PIC_OFF | > > > > + pic | > > > > PCI_EXP_SLTCTL_AIC_OFF); > > > > > > > > + if (!slot_dev) { > > > > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, > > > > + PCI_EXP_SLTCTL_PCC); > > > > > > I dislike it when we clear bits then set them back. > > > Please just add else here. > > Sure, > > > > > > > > > + } > > > > + > > > > > > Need to disable for compat types? > > As above, > > > > Thanks for the review, > > Marcel > > > > > > > > > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > > PCI_EXP_SLTSTA_EIS |/* on reset, > > > > the lock is released */ > > > > diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h > > > > index 4d123d9..652d9fc 100644 > > > > --- a/include/hw/pci/pcie_regs.h > > > > +++ b/include/hw/pci/pcie_regs.h > > > > @@ -57,6 +57,8 @@ > > > > #define PCI_EXP_SLTCTL_PIC_SHIFT (ffs(PCI_EXP_SLTCTL_PIC) - 1) > > > > #define PCI_EXP_SLTCTL_PIC_OFF \ > > > > (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_PIC_SHIFT) > > > > +#define PCI_EXP_SLTCTL_PIC_ON \ > > > > + (PCI_EXP_SLTCTL_IND_ON << PCI_EXP_SLTCTL_PIC_SHIFT) > > > > > > > > #define PCI_EXP_SLTCTL_SUPPORTED \ > > > > (PCI_EXP_SLTCTL_ABPE | \ > > > > -- > > > > 1.8.3.1 > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality 2014-06-22 11:11 ` Michael S. Tsirkin @ 2014-06-22 11:17 ` Marcel Apfelbaum 0 siblings, 0 replies; 16+ messages in thread From: Marcel Apfelbaum @ 2014-06-22 11:17 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Sun, 2014-06-22 at 14:11 +0300, Michael S. Tsirkin wrote: > On Sun, Jun 22, 2014 at 01:52:46PM +0300, Michael S. Tsirkin wrote: > > On Sun, Jun 22, 2014 at 01:47:24PM +0300, Marcel Apfelbaum wrote: > > > On Thu, 2014-06-19 at 17:39 +0300, Michael S. Tsirkin wrote: > > > > On Thu, Jun 19, 2014 at 04:52:20PM +0300, Marcel Apfelbaum wrote: > > > > > It is needed by hot-unplug in order to get an indication > > > > > from the OS when the device can be physically detached. > > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > --- > > > > > hw/pci/pcie.c | 15 ++++++++++++++- > > > > > include/hw/pci/pcie_regs.h | 2 ++ > > > > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > > > index ae92f00..f8bf515 100644 > > > > > --- a/hw/pci/pcie.c > > > > > +++ b/hw/pci/pcie.c > > > > > @@ -292,16 +292,19 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) > > > > > PCI_EXP_SLTCAP_HPC | > > > > > PCI_EXP_SLTCAP_PIP | > > > > > PCI_EXP_SLTCAP_AIP | > > > > > + PCI_EXP_SLTCAP_PCP | > > > > > PCI_EXP_SLTCAP_ABP); > > > > > > > > > > pci_word_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCTL, > > > > > PCI_EXP_SLTCTL_PIC | > > > > > + PCI_EXP_SLTCTL_PCC | > > > > > PCI_EXP_SLTCTL_AIC); > > > > > pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCTL, > > > > > PCI_EXP_SLTCTL_PIC_OFF | > > > > > PCI_EXP_SLTCTL_AIC_OFF); > > > > > pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL, > > > > > PCI_EXP_SLTCTL_PIC | > > > > > + PCI_EXP_SLTCTL_PCC | > > > > > PCI_EXP_SLTCTL_AIC | > > > > > PCI_EXP_SLTCTL_HPIE | > > > > > PCI_EXP_SLTCTL_CCIE | > > > > > > > > Need to disable for compat types? > > > Does Paolo's explanation answer your question? > > > > Kind of hacky - we do have compat work for > > q35. So I'd prefer consistency. > > > > > > > > > > > > > > > @@ -327,21 +330,31 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) > > > > > void pcie_cap_slot_reset(PCIDevice *dev) > > > > > { > > > > > uint8_t *exp_cap = dev->config + dev->exp.exp_cap; > > > > > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > > > > > > > > What does this mean? > > > > Downstream port? > > > Yes > > > > well it's not really clear, needs a comment. > > > > > > A switch can have multiple downstream ports at any slot #. > > > It doesn't matter how many devices are under this slot, we need only > > > one to power up the slot. The question here was "Do we have at least > > > one device attahc to slot? If yes, power up the slot." > > > > You need a loop for that I think. There's no guarantee the > > device is at devfn=0. > > Unless something guarantees this is a downstream port, > if yes maybe add an assert? > slot_dev is also not a very good variable name. > How about > bool populated = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > ? > This is all you care about ... > And maybe add a comment > /* Downstream ports enforce device number 0. */ I'll use that, thanks! Marcel > > > > > > > > > > + int pic; > > > > > > > > uint16_t please. > > > Sure, > > > > > > > > > > > > + > > > > > + pic = slot_dev ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF; > > > > > > > > > > PCIE_DEV_PRINTF(dev, "reset\n"); > > > > > > > > > > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL, > > > > > PCI_EXP_SLTCTL_EIC | > > > > > PCI_EXP_SLTCTL_PIC | > > > > > + PCI_EXP_SLTCTL_PCC | > > > > > PCI_EXP_SLTCTL_AIC | > > > > > PCI_EXP_SLTCTL_HPIE | > > > > > PCI_EXP_SLTCTL_CCIE | > > > > > PCI_EXP_SLTCTL_PDCE | > > > > > PCI_EXP_SLTCTL_ABPE); > > > > > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, > > > > > - PCI_EXP_SLTCTL_PIC_OFF | > > > > > + pic | > > > > > PCI_EXP_SLTCTL_AIC_OFF); > > > > > > > > > > + if (!slot_dev) { > > > > > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, > > > > > + PCI_EXP_SLTCTL_PCC); > > > > > > > > I dislike it when we clear bits then set them back. > > > > Please just add else here. > > > Sure, > > > > > > > > > > > > + } > > > > > + > > > > > > > > Need to disable for compat types? > > > As above, > > > > > > Thanks for the review, > > > Marcel > > > > > > > > > > > > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > > > PCI_EXP_SLTSTA_EIS |/* on reset, > > > > > the lock is released */ > > > > > diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h > > > > > index 4d123d9..652d9fc 100644 > > > > > --- a/include/hw/pci/pcie_regs.h > > > > > +++ b/include/hw/pci/pcie_regs.h > > > > > @@ -57,6 +57,8 @@ > > > > > #define PCI_EXP_SLTCTL_PIC_SHIFT (ffs(PCI_EXP_SLTCTL_PIC) - 1) > > > > > #define PCI_EXP_SLTCTL_PIC_OFF \ > > > > > (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_PIC_SHIFT) > > > > > +#define PCI_EXP_SLTCTL_PIC_ON \ > > > > > + (PCI_EXP_SLTCTL_IND_ON << PCI_EXP_SLTCTL_PIC_SHIFT) > > > > > > > > > > #define PCI_EXP_SLTCTL_SUPPORTED \ > > > > > (PCI_EXP_SLTCTL_ABPE | \ > > > > > -- > > > > > 1.8.3.1 > > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 3/3] hw/pcie: better hotplug/hotunplug support 2014-06-19 13:52 [Qemu-devel] [PATCH 0/3] hw/pcie: better hotplug/hotunplug support Marcel Apfelbaum 2014-06-19 13:52 ` [Qemu-devel] [PATCH 1/3] hw/pcie: corrected a debug message Marcel Apfelbaum 2014-06-19 13:52 ` [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality Marcel Apfelbaum @ 2014-06-19 13:52 ` Marcel Apfelbaum 2014-06-19 14:43 ` Michael S. Tsirkin 2 siblings, 1 reply; 16+ messages in thread From: Marcel Apfelbaum @ 2014-06-19 13:52 UTC (permalink / raw) To: qemu-devel; +Cc: mst Hotplug triggers both 'present detect change' and 'attention button pressed'. Hotunplug starts by triggering 'attention button pressed', then waits for the OS to power off the device and only then detaches it. Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- hw/pci/pcie.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index f8bf515..9cfd93d 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -258,7 +258,8 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_PDS); - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); + pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), + PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP); } void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, @@ -268,10 +269,7 @@ void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); - object_unparent(OBJECT(dev)); - pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, - PCI_EXP_SLTSTA_PDS); - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); + pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); } /* pci express slot for pci express root/downstream port @@ -389,6 +387,18 @@ void pcie_cap_slot_write_config(PCIDevice *dev, sltsta); } + if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && + ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; + if (slot_dev) { + object_unparent(OBJECT(slot_dev)); + pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, + PCI_EXP_SLTSTA_PDS); + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, + PCI_EXP_SLTSTA_PDC); + } + } + hotplug_event_notify(dev); /* -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] hw/pcie: better hotplug/hotunplug support 2014-06-19 13:52 ` [Qemu-devel] [PATCH 3/3] hw/pcie: better hotplug/hotunplug support Marcel Apfelbaum @ 2014-06-19 14:43 ` Michael S. Tsirkin 2014-06-22 10:54 ` Marcel Apfelbaum 0 siblings, 1 reply; 16+ messages in thread From: Michael S. Tsirkin @ 2014-06-19 14:43 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: qemu-devel On Thu, Jun 19, 2014 at 04:52:21PM +0300, Marcel Apfelbaum wrote: > Hotplug triggers both 'present detect change' and > 'attention button pressed'. > > Hotunplug starts by triggering 'attention button pressed', > then waits for the OS to power off the device and only > then detaches it. > Pls not here that current code is broken: it does surprise removal which crashes guests. > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > --- > hw/pci/pcie.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index f8bf515..9cfd93d 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -258,7 +258,8 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > PCI_EXP_SLTSTA_PDS); > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > + pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), > + PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP); > } > > void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > @@ -268,10 +269,7 @@ void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); > > - object_unparent(OBJECT(dev)); > - pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > - PCI_EXP_SLTSTA_PDS); > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > + pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); > } > > /* pci express slot for pci express root/downstream port > @@ -389,6 +387,18 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > sltsta); > } > pls add code comments explaining the logic here. > + if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > + ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > + if (slot_dev) { Here you want to remove all devices behind the bridge? You need to do this for all functions, not just function 0. > + object_unparent(OBJECT(slot_dev)); > + pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > + PCI_EXP_SLTSTA_PDS); > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > + PCI_EXP_SLTSTA_PDC); These bits need to be cleared in any case? > + } > + } > + > hotplug_event_notify(dev); > > /* > -- > 1.8.3.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] hw/pcie: better hotplug/hotunplug support 2014-06-19 14:43 ` Michael S. Tsirkin @ 2014-06-22 10:54 ` Marcel Apfelbaum 2014-06-22 11:03 ` Michael S. Tsirkin 0 siblings, 1 reply; 16+ messages in thread From: Marcel Apfelbaum @ 2014-06-22 10:54 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Thu, 2014-06-19 at 17:43 +0300, Michael S. Tsirkin wrote: > On Thu, Jun 19, 2014 at 04:52:21PM +0300, Marcel Apfelbaum wrote: > > Hotplug triggers both 'present detect change' and > > 'attention button pressed'. > > > > Hotunplug starts by triggering 'attention button pressed', > > then waits for the OS to power off the device and only > > then detaches it. > > > > Pls not here that current code is broken: it does surprise removal which > crashes guests. I'll add a note, sure. > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > --- > > hw/pci/pcie.c | 20 +++++++++++++++----- > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > index f8bf515..9cfd93d 100644 > > --- a/hw/pci/pcie.c > > +++ b/hw/pci/pcie.c > > @@ -258,7 +258,8 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > > PCI_EXP_SLTSTA_PDS); > > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > > + pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), > > + PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP); > > } > > > > void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > @@ -268,10 +269,7 @@ void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); > > > > - object_unparent(OBJECT(dev)); > > - pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > - PCI_EXP_SLTSTA_PDS); > > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > > + pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); > > } > > > > /* pci express slot for pci express root/downstream port > > @@ -389,6 +387,18 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > > sltsta); > > } > > > > pls add code comments explaining the logic here. I thought is clear :( Basically: If the device is present, power indicator off and power controller off, it is safe to detach the device. > > > + if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > > + ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { > > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > > + if (slot_dev) { > > Here you want to remove all devices behind the bridge? Yes, but since it is PCIe we only have one device, but we may have a multi-function device... > You need to do this for all functions, not just function 0. So bus devices are actually functions... OK, I'll run a loop here. > > > + object_unparent(OBJECT(slot_dev)); > > + pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > + PCI_EXP_SLTSTA_PDS); > > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > > + PCI_EXP_SLTSTA_PDC); > > These bits need to be cleared in any case? No, "PDS on" means the devices is present, so we clear it only after the OS powers it off. > > > + } > > + } > > + > > hotplug_event_notify(dev); > > > > /* > > -- > > 1.8.3.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] hw/pcie: better hotplug/hotunplug support 2014-06-22 10:54 ` Marcel Apfelbaum @ 2014-06-22 11:03 ` Michael S. Tsirkin 2014-06-22 11:11 ` Marcel Apfelbaum 0 siblings, 1 reply; 16+ messages in thread From: Michael S. Tsirkin @ 2014-06-22 11:03 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: qemu-devel On Sun, Jun 22, 2014 at 01:54:06PM +0300, Marcel Apfelbaum wrote: > On Thu, 2014-06-19 at 17:43 +0300, Michael S. Tsirkin wrote: > > On Thu, Jun 19, 2014 at 04:52:21PM +0300, Marcel Apfelbaum wrote: > > > Hotplug triggers both 'present detect change' and > > > 'attention button pressed'. > > > > > > Hotunplug starts by triggering 'attention button pressed', > > > then waits for the OS to power off the device and only > > > then detaches it. > > > > > > > Pls not here that current code is broken: it does surprise removal which > > crashes guests. > I'll add a note, sure. > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > --- > > > hw/pci/pcie.c | 20 +++++++++++++++----- > > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > index f8bf515..9cfd93d 100644 > > > --- a/hw/pci/pcie.c > > > +++ b/hw/pci/pcie.c > > > @@ -258,7 +258,8 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > > > PCI_EXP_SLTSTA_PDS); > > > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > > > + pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), > > > + PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP); > > > } > > > > > > void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > @@ -268,10 +269,7 @@ void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); > > > > > > - object_unparent(OBJECT(dev)); > > > - pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > - PCI_EXP_SLTSTA_PDS); > > > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > > > + pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); > > > } > > > > > > /* pci express slot for pci express root/downstream port > > > @@ -389,6 +387,18 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > > > sltsta); > > > } > > > > > > > pls add code comments explaining the logic here. > I thought is clear :( > Basically: If the device is present, power indicator off and power > controller off, it is safe to detach the device. Sure, put this in the comment. > > > > > + if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > > > + ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { > > > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > > > + if (slot_dev) { > > > > Here you want to remove all devices behind the bridge? > Yes, but since it is PCIe we only have one device, > but we may have a multi-function device... Exactly. Up to 256 functions. > > You need to do this for all functions, not just function 0. > So bus devices are actually functions... OK, I'll run a loop here. > > > > > > + object_unparent(OBJECT(slot_dev)); > > > + pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > + PCI_EXP_SLTSTA_PDS); > > > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > > > + PCI_EXP_SLTSTA_PDC); > > > > These bits need to be cleared in any case? > No, "PDS on" means the devices is present, so we clear it only after the > OS powers it off. So why not clear PDS unconditionally? > > > > > + } > > > + } > > > + > > > hotplug_event_notify(dev); > > > > > > /* > > > -- > > > 1.8.3.1 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] hw/pcie: better hotplug/hotunplug support 2014-06-22 11:03 ` Michael S. Tsirkin @ 2014-06-22 11:11 ` Marcel Apfelbaum 2014-06-22 11:12 ` Michael S. Tsirkin 0 siblings, 1 reply; 16+ messages in thread From: Marcel Apfelbaum @ 2014-06-22 11:11 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Sun, 2014-06-22 at 14:03 +0300, Michael S. Tsirkin wrote: > On Sun, Jun 22, 2014 at 01:54:06PM +0300, Marcel Apfelbaum wrote: > > On Thu, 2014-06-19 at 17:43 +0300, Michael S. Tsirkin wrote: > > > On Thu, Jun 19, 2014 at 04:52:21PM +0300, Marcel Apfelbaum wrote: > > > > Hotplug triggers both 'present detect change' and > > > > 'attention button pressed'. > > > > > > > > Hotunplug starts by triggering 'attention button pressed', > > > > then waits for the OS to power off the device and only > > > > then detaches it. > > > > > > > > > > Pls not here that current code is broken: it does surprise removal which > > > crashes guests. > > I'll add a note, sure. > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > --- > > > > hw/pci/pcie.c | 20 +++++++++++++++----- > > > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > > index f8bf515..9cfd93d 100644 > > > > --- a/hw/pci/pcie.c > > > > +++ b/hw/pci/pcie.c > > > > @@ -258,7 +258,8 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > > > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > > > > PCI_EXP_SLTSTA_PDS); > > > > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > > > > + pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), > > > > + PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP); > > > > } > > > > > > > > void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > @@ -268,10 +269,7 @@ void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > > > pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); > > > > > > > > - object_unparent(OBJECT(dev)); > > > > - pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > > - PCI_EXP_SLTSTA_PDS); > > > > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > > > > + pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); > > > > } > > > > > > > > /* pci express slot for pci express root/downstream port > > > > @@ -389,6 +387,18 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > > > > sltsta); > > > > } > > > > > > > > > > pls add code comments explaining the logic here. > > I thought is clear :( > > Basically: If the device is present, power indicator off and power > > controller off, it is safe to detach the device. > > Sure, put this in the comment. > > > > > > > > + if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > > > > + ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { > > > > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > > > > + if (slot_dev) { > > > > > > Here you want to remove all devices behind the bridge? > > Yes, but since it is PCIe we only have one device, > > but we may have a multi-function device... > > Exactly. Up to 256 functions. > > > > You need to do this for all functions, not just function 0. > > So bus devices are actually functions... OK, I'll run a loop here. > > > > > > > > > + object_unparent(OBJECT(slot_dev)); > > > > + pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > > + PCI_EXP_SLTSTA_PDS); > > > > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > > > > + PCI_EXP_SLTSTA_PDC); > > > > > > These bits need to be cleared in any case? > > No, "PDS on" means the devices is present, so we clear it only after the > > OS powers it off. > > So why not clear PDS unconditionally? Because we are 'allowed' to remove the device only when the OS tells us the power is off. Before that, it can result in data loss, since the device driver is not yet unloaded. > > > > > > > + } > > > > + } > > > > + > > > > hotplug_event_notify(dev); > > > > > > > > /* > > > > -- > > > > 1.8.3.1 > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] hw/pcie: better hotplug/hotunplug support 2014-06-22 11:11 ` Marcel Apfelbaum @ 2014-06-22 11:12 ` Michael S. Tsirkin 2014-06-22 11:24 ` Marcel Apfelbaum 0 siblings, 1 reply; 16+ messages in thread From: Michael S. Tsirkin @ 2014-06-22 11:12 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: qemu-devel On Sun, Jun 22, 2014 at 02:11:05PM +0300, Marcel Apfelbaum wrote: > On Sun, 2014-06-22 at 14:03 +0300, Michael S. Tsirkin wrote: > > On Sun, Jun 22, 2014 at 01:54:06PM +0300, Marcel Apfelbaum wrote: > > > On Thu, 2014-06-19 at 17:43 +0300, Michael S. Tsirkin wrote: > > > > On Thu, Jun 19, 2014 at 04:52:21PM +0300, Marcel Apfelbaum wrote: > > > > > Hotplug triggers both 'present detect change' and > > > > > 'attention button pressed'. > > > > > > > > > > Hotunplug starts by triggering 'attention button pressed', > > > > > then waits for the OS to power off the device and only > > > > > then detaches it. > > > > > > > > > > > > > Pls not here that current code is broken: it does surprise removal which > > > > crashes guests. > > > I'll add a note, sure. > > > > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > --- > > > > > hw/pci/pcie.c | 20 +++++++++++++++----- > > > > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > > > index f8bf515..9cfd93d 100644 > > > > > --- a/hw/pci/pcie.c > > > > > +++ b/hw/pci/pcie.c > > > > > @@ -258,7 +258,8 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > > > > > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > > > > > PCI_EXP_SLTSTA_PDS); > > > > > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > > > > > + pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), > > > > > + PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP); > > > > > } > > > > > > > > > > void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > @@ -268,10 +269,7 @@ void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > > > > > pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); > > > > > > > > > > - object_unparent(OBJECT(dev)); > > > > > - pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > > > - PCI_EXP_SLTSTA_PDS); > > > > > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > > > > > + pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); > > > > > } > > > > > > > > > > /* pci express slot for pci express root/downstream port > > > > > @@ -389,6 +387,18 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > > > > > sltsta); > > > > > } > > > > > > > > > > > > > pls add code comments explaining the logic here. > > > I thought is clear :( > > > Basically: If the device is present, power indicator off and power > > > controller off, it is safe to detach the device. > > > > Sure, put this in the comment. > > > > > > > > > > > + if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > > > > > + ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { > > > > > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > > > > > + if (slot_dev) { > > > > > > > > Here you want to remove all devices behind the bridge? > > > Yes, but since it is PCIe we only have one device, > > > but we may have a multi-function device... > > > > Exactly. Up to 256 functions. > > > > > > You need to do this for all functions, not just function 0. > > > So bus devices are actually functions... OK, I'll run a loop here. > > > > > > > > > > > > + object_unparent(OBJECT(slot_dev)); > > > > > + pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > > > + PCI_EXP_SLTSTA_PDS); > > > > > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > > > > > + PCI_EXP_SLTSTA_PDC); > > > > > > > > These bits need to be cleared in any case? > > > No, "PDS on" means the devices is present, so we clear it only after the > > > OS powers it off. > > > > So why not clear PDS unconditionally? > Because we are 'allowed' to remove the device only when the OS tells us > the power is off. Before that, it can result in data loss, > since the device driver is not yet unloaded. Yes. But if (slot_dev) seems not to be needed: if slot is empty we can clear PDC. > > > > > > > > > > + } > > > > > + } > > > > > + > > > > > hotplug_event_notify(dev); > > > > > > > > > > /* > > > > > -- > > > > > 1.8.3.1 > > > > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 3/3] hw/pcie: better hotplug/hotunplug support 2014-06-22 11:12 ` Michael S. Tsirkin @ 2014-06-22 11:24 ` Marcel Apfelbaum 0 siblings, 0 replies; 16+ messages in thread From: Marcel Apfelbaum @ 2014-06-22 11:24 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel On Sun, 2014-06-22 at 14:12 +0300, Michael S. Tsirkin wrote: > On Sun, Jun 22, 2014 at 02:11:05PM +0300, Marcel Apfelbaum wrote: > > On Sun, 2014-06-22 at 14:03 +0300, Michael S. Tsirkin wrote: > > > On Sun, Jun 22, 2014 at 01:54:06PM +0300, Marcel Apfelbaum wrote: > > > > On Thu, 2014-06-19 at 17:43 +0300, Michael S. Tsirkin wrote: > > > > > On Thu, Jun 19, 2014 at 04:52:21PM +0300, Marcel Apfelbaum wrote: > > > > > > Hotplug triggers both 'present detect change' and > > > > > > 'attention button pressed'. > > > > > > > > > > > > Hotunplug starts by triggering 'attention button pressed', > > > > > > then waits for the OS to power off the device and only > > > > > > then detaches it. > > > > > > > > > > > > > > > > Pls not here that current code is broken: it does surprise removal which > > > > > crashes guests. > > > > I'll add a note, sure. > > > > > > > > > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > > > --- > > > > > > hw/pci/pcie.c | 20 +++++++++++++++----- > > > > > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > > > > index f8bf515..9cfd93d 100644 > > > > > > --- a/hw/pci/pcie.c > > > > > > +++ b/hw/pci/pcie.c > > > > > > @@ -258,7 +258,8 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > > > > > > > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > > > > > > PCI_EXP_SLTSTA_PDS); > > > > > > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > > > > > > + pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), > > > > > > + PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP); > > > > > > } > > > > > > > > > > > > void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > @@ -268,10 +269,7 @@ void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > > > > > > > pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); > > > > > > > > > > > > - object_unparent(OBJECT(dev)); > > > > > > - pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > > > > - PCI_EXP_SLTSTA_PDS); > > > > > > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > > > > > > + pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); > > > > > > } > > > > > > > > > > > > /* pci express slot for pci express root/downstream port > > > > > > @@ -389,6 +387,18 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > > > > > > sltsta); > > > > > > } > > > > > > > > > > > > > > > > pls add code comments explaining the logic here. > > > > I thought is clear :( > > > > Basically: If the device is present, power indicator off and power > > > > controller off, it is safe to detach the device. > > > > > > Sure, put this in the comment. > > > > > > > > > > > > > > + if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > > > > > > + ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { > > > > > > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > > > > > > + if (slot_dev) { > > > > > > > > > > Here you want to remove all devices behind the bridge? > > > > Yes, but since it is PCIe we only have one device, > > > > but we may have a multi-function device... > > > > > > Exactly. Up to 256 functions. > > > > > > > > You need to do this for all functions, not just function 0. > > > > So bus devices are actually functions... OK, I'll run a loop here. > > > > > > > > > > > > > > > + object_unparent(OBJECT(slot_dev)); > > > > > > + pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > > > > + PCI_EXP_SLTSTA_PDS); > > > > > > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > > > > > > + PCI_EXP_SLTSTA_PDC); > > > > > > > > > > These bits need to be cleared in any case? > > > > No, "PDS on" means the devices is present, so we clear it only after the > > > > OS powers it off. > > > > > > So why not clear PDS unconditionally? > > Because we are 'allowed' to remove the device only when the OS tells us > > the power is off. Before that, it can result in data loss, > > since the device driver is not yet unloaded. > > Yes. But if (slot_dev) seems not to be needed: > if slot is empty we can clear PDC. You mean PDS, PDC is the event we send to the OS. The main reason I added this "if clause" is to protect against multiple OS events of 'power controller off' and 'power indicator off', but now I see that indeed I don't need that because I check "sltsta & PCI_EXP_SLTSTA_PDS". I will remove that if. Thanks, Marcel > > > > > > > > > > > > > > > > + } > > > > > > + } > > > > > > + > > > > > > hotplug_event_notify(dev); > > > > > > > > > > > > /* > > > > > > -- > > > > > > 1.8.3.1 > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-06-22 11:25 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-19 13:52 [Qemu-devel] [PATCH 0/3] hw/pcie: better hotplug/hotunplug support Marcel Apfelbaum 2014-06-19 13:52 ` [Qemu-devel] [PATCH 1/3] hw/pcie: corrected a debug message Marcel Apfelbaum 2014-06-19 13:52 ` [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality Marcel Apfelbaum 2014-06-19 14:39 ` Michael S. Tsirkin 2014-06-19 20:56 ` Paolo Bonzini 2014-06-22 10:47 ` Marcel Apfelbaum 2014-06-22 10:52 ` Michael S. Tsirkin 2014-06-22 11:11 ` Michael S. Tsirkin 2014-06-22 11:17 ` Marcel Apfelbaum 2014-06-19 13:52 ` [Qemu-devel] [PATCH 3/3] hw/pcie: better hotplug/hotunplug support Marcel Apfelbaum 2014-06-19 14:43 ` Michael S. Tsirkin 2014-06-22 10:54 ` Marcel Apfelbaum 2014-06-22 11:03 ` Michael S. Tsirkin 2014-06-22 11:11 ` Marcel Apfelbaum 2014-06-22 11:12 ` Michael S. Tsirkin 2014-06-22 11:24 ` Marcel Apfelbaum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).