| Message ID | 1403185941-19561-3-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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 > >
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 > > > >
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 > > > > > >
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 | \
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(-)