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

[Michael S. Tsirkin]

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 <address@hidden>
> > > > ---
> > > >  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
> > 
> >