| Message ID | 20230213140103.1518173-12-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | pci hotplug tracking | expand |
On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote: > In pcie_cap_slot_write_config() we check for PCI_EXP_SLTCTL_PWR_OFF in > a bad form. We should distinguish PCI_EXP_SLTCTL_PWR which is a "mask" > and PCI_EXP_SLTCTL_PWR_OFF which is value for that mask. > > Better code is in pcie_cap_slot_unplug_request_cb() and in > pcie_cap_update_power(). Let's use same pattern everywhere. To simplify > things add also a helper. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru> > --- > hw/pci/pcie.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index db8360226f..90faf0710a 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -39,6 +39,11 @@ > #define PCIE_DEV_PRINTF(dev, fmt, ...) \ > PCIE_DPRINTF("%s:%x "fmt, (dev)->name, (dev)->devfn, ## __VA_ARGS__) > > +static bool pcie_sltctl_powered_off(uint16_t sltctl) > +{ > + return (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF > + && (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF; > +} > > /*************************************************************************** > * pci express capability helper functions > @@ -395,6 +400,7 @@ static void pcie_cap_update_power(PCIDevice *hotplug_dev) > > if (sltcap & PCI_EXP_SLTCAP_PCP) { > power = (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_ON; > + /* Don't we need to check also (sltctl & PCI_EXP_SLTCTL_PIC) ? */ > } > > pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > @@ -579,8 +585,7 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev, > return; > } > > - if (((sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF) && > - ((sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF)) { > + if (pcie_sltctl_powered_off(sltctl)) { > /* slot is powered off -> unplug without round-trip to the guest */ > pcie_cap_slot_do_unplug(hotplug_pdev); > hotplug_event_notify(hotplug_pdev); > @@ -770,10 +775,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > * this is a work around for guests that overwrite > * control of powered off slots before powering them on. > */ > - if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > - (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF && > - (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) || > - (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PWR_IND_OFF)) { > + if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_off(val) && > + !pcie_sltctl_powered_off(old_slt_ctl)) > + { > pcie_cap_slot_do_unplug(dev); > } > pcie_cap_update_power(dev);
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index db8360226f..90faf0710a 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -39,6 +39,11 @@ #define PCIE_DEV_PRINTF(dev, fmt, ...) \ PCIE_DPRINTF("%s:%x "fmt, (dev)->name, (dev)->devfn, ## __VA_ARGS__) +static bool pcie_sltctl_powered_off(uint16_t sltctl) +{ + return (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF + && (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF; +} /*************************************************************************** * pci express capability helper functions @@ -395,6 +400,7 @@ static void pcie_cap_update_power(PCIDevice *hotplug_dev) if (sltcap & PCI_EXP_SLTCAP_PCP) { power = (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_ON; + /* Don't we need to check also (sltctl & PCI_EXP_SLTCTL_PIC) ? */ } pci_for_each_device(sec_bus, pci_bus_num(sec_bus), @@ -579,8 +585,7 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev, return; } - if (((sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF) && - ((sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF)) { + if (pcie_sltctl_powered_off(sltctl)) { /* slot is powered off -> unplug without round-trip to the guest */ pcie_cap_slot_do_unplug(hotplug_pdev); hotplug_event_notify(hotplug_pdev); @@ -770,10 +775,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev, * this is a work around for guests that overwrite * control of powered off slots before powering them on. */ - if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && - (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF && - (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) || - (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PWR_IND_OFF)) { + if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_off(val) && + !pcie_sltctl_powered_off(old_slt_ctl)) + { pcie_cap_slot_do_unplug(dev); } pcie_cap_update_power(dev);