Re: [PATCH v4 11/16] pcie: introduce pcie_sltctl_powered_off() helper

[Anton Kuchin]

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);