Re: [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality

[Marcel Apfelbaum <marcel.a@redhat.com>]

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