From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Add relevant vpci register handlers when assigning PCI device to a domain
and remove those when de-assigning. This allows having different
handlers for different domains, e.g. hwdom and other guests.

Use stubs for guest domains for now.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

---
Since v2:
- remove unneeded ifdefs for CONFIG_HAS_VPCI_GUEST_SUPPORT as more code
  has been eliminated from being built on x86
Since v1:
 - constify struct pci_dev where possible
 - do not open code is_system_domain()
 - simplify some code3. simplify
 - use gdprintk + error code instead of gprintk
 - gate vpci_bar_{add|remove}_handlers with CONFIG_HAS_VPCI_GUEST_SUPPORT,
   so these do not get compiled for x86
 - removed unneeded is_system_domain check
---
 xen/drivers/vpci/header.c | 72 ++++++++++++++++++++++++++++++++++-----
 xen/drivers/vpci/vpci.c   |  4 +--
 xen/include/xen/vpci.h    |  8 +++++
 3 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 3d571356397a..1ce98795fcca 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -397,6 +397,17 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg,
     pci_conf_write32(pdev->sbdf, reg, val);
 }
 
+static void guest_bar_write(const struct pci_dev *pdev, unsigned int reg,
+                            uint32_t val, void *data)
+{
+}
+
+static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg,
+                               void *data)
+{
+    return 0xffffffff;
+}
+
 static void rom_write(const struct pci_dev *pdev, unsigned int reg,
                       uint32_t val, void *data)
 {
@@ -445,14 +456,25 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
         rom->addr = val & PCI_ROM_ADDRESS_MASK;
 }
 
-static int add_bar_handlers(const struct pci_dev *pdev)
+static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
+                            uint32_t val, void *data)
+{
+}
+
+static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
+                               void *data)
+{
+    return 0xffffffff;
+}
+
+static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)
 {
     unsigned int i;
     struct vpci_header *header = &pdev->vpci->header;
     struct vpci_bar *bars = header->bars;
     int rc;
 
-    /* Setup a handler for the command register. */
+    /* Setup a handler for the command register: same for hwdom and guests. */
     rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, PCI_COMMAND,
                            2, header);
     if ( rc )
@@ -475,8 +497,13 @@ static int add_bar_handlers(const struct pci_dev *pdev)
                 rom_reg = PCI_ROM_ADDRESS;
             else
                 rom_reg = PCI_ROM_ADDRESS1;
-            rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
-                                   rom_reg, 4, &bars[i]);
+            if ( is_hwdom )
+                rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
+                                       rom_reg, 4, &bars[i]);
+            else
+                rc = vpci_add_register(pdev->vpci,
+                                       guest_rom_read, guest_rom_write,
+                                       rom_reg, 4, &bars[i]);
             if ( rc )
                 return rc;
         }
@@ -485,8 +512,13 @@ static int add_bar_handlers(const struct pci_dev *pdev)
             uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
 
             /* This is either VPCI_BAR_MEM32 or VPCI_BAR_MEM64_{LO|HI}. */
-            rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg,
-                                   4, &bars[i]);
+            if ( is_hwdom )
+                rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write,
+                                       reg, 4, &bars[i]);
+            else
+                rc = vpci_add_register(pdev->vpci,
+                                       guest_bar_read, guest_bar_write,
+                                       reg, 4, &bars[i]);
             if ( rc )
                 return rc;
         }
@@ -520,7 +552,7 @@ static int init_bars(struct pci_dev *pdev)
     }
 
     if ( pdev->ignore_bars )
-        return add_bar_handlers(pdev);
+        return add_bar_handlers(pdev, true);
 
     /* Disable memory decoding before sizing. */
     cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
@@ -582,7 +614,7 @@ static int init_bars(struct pci_dev *pdev)
                               PCI_ROM_ADDRESS_ENABLE;
     }
 
-    rc = add_bar_handlers(pdev);
+    rc = add_bar_handlers(pdev, true);
     if ( rc )
     {
         pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
@@ -593,6 +625,30 @@ static int init_bars(struct pci_dev *pdev)
 }
 REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
 
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+int vpci_bar_add_handlers(const struct domain *d, const struct pci_dev *pdev)
+{
+    int rc;
+
+    /* Remove previously added registers. */
+    vpci_remove_device_registers(pdev);
+
+    rc = add_bar_handlers(pdev, is_hardware_domain(d));
+    if ( rc )
+        gdprintk(XENLOG_ERR,
+                 "%pp: failed to add BAR handlers for dom%pd: %d\n",
+                 &pdev->sbdf, d, rc);
+    return rc;
+}
+
+int vpci_bar_remove_handlers(const struct domain *d, const struct pci_dev *pdev)
+{
+    /* Remove previously added registers. */
+    vpci_remove_device_registers(pdev);
+    return 0;
+}
+#endif
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 0fe86cb30d23..702f7b5d5dda 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -95,7 +95,7 @@ int vpci_assign_device(struct domain *d, const struct pci_dev *dev)
     if ( is_system_domain(d) || !has_vpci(d) )
         return 0;
 
-    return 0;
+    return vpci_bar_add_handlers(d, dev);
 }
 
 /* Notify vPCI that device is de-assigned from guest. */
@@ -105,7 +105,7 @@ int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
     if ( is_system_domain(d) || !has_vpci(d) )
         return 0;
 
-    return 0;
+    return vpci_bar_remove_handlers(d, dev);
 }
 #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
 
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index ecc08f2c0f65..fd822c903af5 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -57,6 +57,14 @@ uint32_t vpci_hw_read32(const struct pci_dev *pdev, unsigned int reg,
  */
 bool __must_check vpci_process_pending(struct vcpu *v);
 
+#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
+/* Add/remove BAR handlers for a domain. */
+int vpci_bar_add_handlers(const struct domain *d,
+                          const struct pci_dev *pdev);
+int vpci_bar_remove_handlers(const struct domain *d,
+                             const struct pci_dev *pdev);
+#endif
+
 struct vpci {
     /* List of vPCI handlers for a device. */
     struct list_head handlers;
-- 
2.25.1

On 30.09.2021 09:52, Oleksandr Andrushchenko wrote:
> @@ -445,14 +456,25 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
>          rom->addr = val & PCI_ROM_ADDRESS_MASK;
>  }
>  
> -static int add_bar_handlers(const struct pci_dev *pdev)
> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
> +                            uint32_t val, void *data)
> +{
> +}
> +
> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
> +                               void *data)
> +{
> +    return 0xffffffff;
> +}
> +
> +static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)

I remain unconvinced that this boolean is the best way to go here, but
I'll leave the decision there to Roger. Just a couple of nits:

> @@ -593,6 +625,30 @@ static int init_bars(struct pci_dev *pdev)
>  }
>  REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>  
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +int vpci_bar_add_handlers(const struct domain *d, const struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    /* Remove previously added registers. */
> +    vpci_remove_device_registers(pdev);
> +
> +    rc = add_bar_handlers(pdev, is_hardware_domain(d));
> +    if ( rc )
> +        gdprintk(XENLOG_ERR,
> +                 "%pp: failed to add BAR handlers for dom%pd: %d\n",

Only %pd please, as that already expands to d<num>.

> +                 &pdev->sbdf, d, rc);
> +    return rc;

Blank line please ahead of the main return statement of a function.

Jan

On 01.10.21 16:26, Jan Beulich wrote:
> On 30.09.2021 09:52, Oleksandr Andrushchenko wrote:
>> @@ -445,14 +456,25 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
>>           rom->addr = val & PCI_ROM_ADDRESS_MASK;
>>   }
>>   
>> -static int add_bar_handlers(const struct pci_dev *pdev)
>> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
>> +                            uint32_t val, void *data)
>> +{
>> +}
>> +
>> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
>> +                               void *data)
>> +{
>> +    return 0xffffffff;
>> +}
>> +
>> +static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)
> I remain unconvinced that this boolean is the best way to go here,
I can remove "bool is_hwdom" and have the checks like:

static int add_bar_handlers(const struct pci_dev *pdev)
{
...
     if ( is_hardware_domain(pdev->domain) )
         rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write,
                                PCI_COMMAND, 2, header);
     else
         rc = vpci_add_register(pdev->vpci, vpci_hw_read16, guest_cmd_write,
                                PCI_COMMAND, 2, header);
Is this going to be better?
>   but
> I'll leave the decision there to Roger. Just a couple of nits:
>
>> @@ -593,6 +625,30 @@ static int init_bars(struct pci_dev *pdev)
>>   }
>>   REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>>   
>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +int vpci_bar_add_handlers(const struct domain *d, const struct pci_dev *pdev)
>> +{
>> +    int rc;
>> +
>> +    /* Remove previously added registers. */
>> +    vpci_remove_device_registers(pdev);
>> +
>> +    rc = add_bar_handlers(pdev, is_hardware_domain(d));
>> +    if ( rc )
>> +        gdprintk(XENLOG_ERR,
>> +                 "%pp: failed to add BAR handlers for dom%pd: %d\n",
> Only %pd please, as that already expands to d<num>.
Good catch, thank you!
>
>> +                 &pdev->sbdf, d, rc);
>> +    return rc;
> Blank line please ahead of the main return statement of a function.
Will add
>
> Jan
>
Thank you,
Oleksandr

On 04.10.2021 07:58, Oleksandr Andrushchenko wrote:
> 
> 
> On 01.10.21 16:26, Jan Beulich wrote:
>> On 30.09.2021 09:52, Oleksandr Andrushchenko wrote:
>>> @@ -445,14 +456,25 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
>>>           rom->addr = val & PCI_ROM_ADDRESS_MASK;
>>>   }
>>>   
>>> -static int add_bar_handlers(const struct pci_dev *pdev)
>>> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
>>> +                            uint32_t val, void *data)
>>> +{
>>> +}
>>> +
>>> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
>>> +                               void *data)
>>> +{
>>> +    return 0xffffffff;
>>> +}
>>> +
>>> +static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)
>> I remain unconvinced that this boolean is the best way to go here,
> I can remove "bool is_hwdom" and have the checks like:
> 
> static int add_bar_handlers(const struct pci_dev *pdev)
> {
> ...
>      if ( is_hardware_domain(pdev->domain) )
>          rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write,
>                                 PCI_COMMAND, 2, header);
>      else
>          rc = vpci_add_register(pdev->vpci, vpci_hw_read16, guest_cmd_write,
>                                 PCI_COMMAND, 2, header);
> Is this going to be better?

Marginally (plus you'd need to prove that pdev->domain can never be NULL
when making it here). "I remain unconvinced" was rather referring to our
prior discussion.

Jan

On Thu, Oct 07, 2021 at 09:22:36AM +0200, Jan Beulich wrote:
> On 04.10.2021 07:58, Oleksandr Andrushchenko wrote:
> > 
> > 
> > On 01.10.21 16:26, Jan Beulich wrote:
> >> On 30.09.2021 09:52, Oleksandr Andrushchenko wrote:
> >>> @@ -445,14 +456,25 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
> >>>           rom->addr = val & PCI_ROM_ADDRESS_MASK;
> >>>   }
> >>>   
> >>> -static int add_bar_handlers(const struct pci_dev *pdev)
> >>> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
> >>> +                            uint32_t val, void *data)
> >>> +{
> >>> +}
> >>> +
> >>> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
> >>> +                               void *data)
> >>> +{
> >>> +    return 0xffffffff;
> >>> +}
> >>> +
> >>> +static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)
> >> I remain unconvinced that this boolean is the best way to go here,
> > I can remove "bool is_hwdom" and have the checks like:
> > 
> > static int add_bar_handlers(const struct pci_dev *pdev)
> > {
> > ...
> >      if ( is_hardware_domain(pdev->domain) )
> >          rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write,
> >                                 PCI_COMMAND, 2, header);
> >      else
> >          rc = vpci_add_register(pdev->vpci, vpci_hw_read16, guest_cmd_write,
> >                                 PCI_COMMAND, 2, header);
> > Is this going to be better?
> 
> Marginally (plus you'd need to prove that pdev->domain can never be NULL
> when making it here).

I think it would an anomaly to try to setup vPCI handlers for a device
without pdev->domain being set. I'm quite sure other vPCI code already
relies on pdev->domain being set.

As I said in another reply I'm not convinced though that splitting
add_bar_handlers is the right thing to do.

Thanks, Roger.

On 13.10.2021 17:38, Roger Pau Monné wrote:
> On Thu, Oct 07, 2021 at 09:22:36AM +0200, Jan Beulich wrote:
>> On 04.10.2021 07:58, Oleksandr Andrushchenko wrote:
>>>
>>>
>>> On 01.10.21 16:26, Jan Beulich wrote:
>>>> On 30.09.2021 09:52, Oleksandr Andrushchenko wrote:
>>>>> @@ -445,14 +456,25 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
>>>>>           rom->addr = val & PCI_ROM_ADDRESS_MASK;
>>>>>   }
>>>>>   
>>>>> -static int add_bar_handlers(const struct pci_dev *pdev)
>>>>> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
>>>>> +                            uint32_t val, void *data)
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
>>>>> +                               void *data)
>>>>> +{
>>>>> +    return 0xffffffff;
>>>>> +}
>>>>> +
>>>>> +static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)
>>>> I remain unconvinced that this boolean is the best way to go here,
>>> I can remove "bool is_hwdom" and have the checks like:
>>>
>>> static int add_bar_handlers(const struct pci_dev *pdev)
>>> {
>>> ...
>>>      if ( is_hardware_domain(pdev->domain) )
>>>          rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write,
>>>                                 PCI_COMMAND, 2, header);
>>>      else
>>>          rc = vpci_add_register(pdev->vpci, vpci_hw_read16, guest_cmd_write,
>>>                                 PCI_COMMAND, 2, header);
>>> Is this going to be better?
>>
>> Marginally (plus you'd need to prove that pdev->domain can never be NULL
>> when making it here).
> 
> I think it would an anomaly to try to setup vPCI handlers for a device
> without pdev->domain being set. I'm quite sure other vPCI code already
> relies on pdev->domain being set.

Quite likely, and my point wasn't to request dealing with the NULL case
by adding a check here. I really meant "prove", mainly recalling that
another patch (in another related series?) altered code around the
setting of pdev->domain in pci_add_device(). It would need to be assured
that whatever goes on there guarantees pdev->domain to have got set.

Jan

On Thu, Sep 30, 2021 at 10:52:16AM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Add relevant vpci register handlers when assigning PCI device to a domain
> and remove those when de-assigning. This allows having different
> handlers for different domains, e.g. hwdom and other guests.
> 
> Use stubs for guest domains for now.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> ---
> Since v2:
> - remove unneeded ifdefs for CONFIG_HAS_VPCI_GUEST_SUPPORT as more code
>   has been eliminated from being built on x86
> Since v1:
>  - constify struct pci_dev where possible
>  - do not open code is_system_domain()
>  - simplify some code3. simplify
>  - use gdprintk + error code instead of gprintk
>  - gate vpci_bar_{add|remove}_handlers with CONFIG_HAS_VPCI_GUEST_SUPPORT,
>    so these do not get compiled for x86
>  - removed unneeded is_system_domain check
> ---
>  xen/drivers/vpci/header.c | 72 ++++++++++++++++++++++++++++++++++-----
>  xen/drivers/vpci/vpci.c   |  4 +--
>  xen/include/xen/vpci.h    |  8 +++++
>  3 files changed, 74 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 3d571356397a..1ce98795fcca 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -397,6 +397,17 @@ static void bar_write(const struct pci_dev *pdev, unsigned int reg,
>      pci_conf_write32(pdev->sbdf, reg, val);
>  }
>  
> +static void guest_bar_write(const struct pci_dev *pdev, unsigned int reg,
> +                            uint32_t val, void *data)
> +{
> +}
> +
> +static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int reg,
> +                               void *data)
> +{
> +    return 0xffffffff;
> +}
> +
>  static void rom_write(const struct pci_dev *pdev, unsigned int reg,
>                        uint32_t val, void *data)
>  {
> @@ -445,14 +456,25 @@ static void rom_write(const struct pci_dev *pdev, unsigned int reg,
>          rom->addr = val & PCI_ROM_ADDRESS_MASK;
>  }
>  
> -static int add_bar_handlers(const struct pci_dev *pdev)
> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
> +                            uint32_t val, void *data)
> +{
> +}
> +
> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int reg,
> +                               void *data)
> +{
> +    return 0xffffffff;
> +}

FWIW, I would also be fine with introducing the code for those
handlers at the same time.

> +
> +static int add_bar_handlers(const struct pci_dev *pdev, bool is_hwdom)

I would rather use is_hardware_domain(pdev->domain) than passing a
boolean here, no need to duplicate data which is already available
from the pdev.

>  {
>      unsigned int i;
>      struct vpci_header *header = &pdev->vpci->header;
>      struct vpci_bar *bars = header->bars;
>      int rc;
>  
> -    /* Setup a handler for the command register. */
> +    /* Setup a handler for the command register: same for hwdom and guests. */
>      rc = vpci_add_register(pdev->vpci, vpci_hw_read16, cmd_write, PCI_COMMAND,
>                             2, header);
>      if ( rc )
> @@ -475,8 +497,13 @@ static int add_bar_handlers(const struct pci_dev *pdev)
>                  rom_reg = PCI_ROM_ADDRESS;
>              else
>                  rom_reg = PCI_ROM_ADDRESS1;
> -            rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
> -                                   rom_reg, 4, &bars[i]);
> +            if ( is_hwdom )
> +                rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rom_write,
> +                                       rom_reg, 4, &bars[i]);
> +            else
> +                rc = vpci_add_register(pdev->vpci,
> +                                       guest_rom_read, guest_rom_write,
> +                                       rom_reg, 4, &bars[i]);

I think you could use:

else if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) )
    rc = vpci_add_register(...
else
    ASSERT_UNREACHABLE();

And then guard the guest_ handlers with CONFIG_HAS_VPCI_GUEST_SUPPORT.

>              if ( rc )
>                  return rc;
>          }
> @@ -485,8 +512,13 @@ static int add_bar_handlers(const struct pci_dev *pdev)
>              uint8_t reg = PCI_BASE_ADDRESS_0 + i * 4;
>  
>              /* This is either VPCI_BAR_MEM32 or VPCI_BAR_MEM64_{LO|HI}. */
> -            rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg,
> -                                   4, &bars[i]);
> +            if ( is_hwdom )
> +                rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write,
> +                                       reg, 4, &bars[i]);
> +            else
> +                rc = vpci_add_register(pdev->vpci,
> +                                       guest_bar_read, guest_bar_write,
> +                                       reg, 4, &bars[i]);
>              if ( rc )
>                  return rc;
>          }
> @@ -520,7 +552,7 @@ static int init_bars(struct pci_dev *pdev)
>      }
>  
>      if ( pdev->ignore_bars )
> -        return add_bar_handlers(pdev);
> +        return add_bar_handlers(pdev, true);
>  
>      /* Disable memory decoding before sizing. */
>      cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> @@ -582,7 +614,7 @@ static int init_bars(struct pci_dev *pdev)
>                                PCI_ROM_ADDRESS_ENABLE;
>      }
>  
> -    rc = add_bar_handlers(pdev);
> +    rc = add_bar_handlers(pdev, true);
>      if ( rc )
>      {
>          pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
> @@ -593,6 +625,30 @@ static int init_bars(struct pci_dev *pdev)
>  }
>  REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
>  
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +int vpci_bar_add_handlers(const struct domain *d, const struct pci_dev *pdev)
> +{
> +    int rc;
> +
> +    /* Remove previously added registers. */
> +    vpci_remove_device_registers(pdev);

Shouldn't this be done by vpci_assign_device as a preparation for
assigning the device?

> +
> +    rc = add_bar_handlers(pdev, is_hardware_domain(d));

Also this model seems to assume that vPCI will require the hardware
domain to have owned the device before it being assigned to a guest,
but for example when using a PV dom0 that won't be the case, and hence
we would need the vPCI fields to be filled when assigning to a guest.

Hence I wonder whether we shouldn't do a full re-initialization when
assigning to a guest instead of this partial one.

> +    if ( rc )
> +        gdprintk(XENLOG_ERR,
> +                 "%pp: failed to add BAR handlers for dom%pd: %d\n",
> +                 &pdev->sbdf, d, rc);
> +    return rc;
> +}
> +
> +int vpci_bar_remove_handlers(const struct domain *d, const struct pci_dev *pdev)
> +{
> +    /* Remove previously added registers. */
> +    vpci_remove_device_registers(pdev);
> +    return 0;
> +}
> +#endif
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 0fe86cb30d23..702f7b5d5dda 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -95,7 +95,7 @@ int vpci_assign_device(struct domain *d, const struct pci_dev *dev)
>      if ( is_system_domain(d) || !has_vpci(d) )
>          return 0;
>  
> -    return 0;
> +    return vpci_bar_add_handlers(d, dev);
>  }
>  
>  /* Notify vPCI that device is de-assigned from guest. */
> @@ -105,7 +105,7 @@ int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
>      if ( is_system_domain(d) || !has_vpci(d) )
>          return 0;
>  
> -    return 0;
> +    return vpci_bar_remove_handlers(d, dev);

I think it would be better to use something similar to
REGISTER_VPCI_INIT here, otherwise this will need to be modified every
time a new capability is handled by Xen.

Maybe we could reuse or expand REGISTER_VPCI_INIT adding another field
to be used for guest initialization?

>  }
>  #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>  
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index ecc08f2c0f65..fd822c903af5 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -57,6 +57,14 @@ uint32_t vpci_hw_read32(const struct pci_dev *pdev, unsigned int reg,
>   */
>  bool __must_check vpci_process_pending(struct vcpu *v);
>  
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +/* Add/remove BAR handlers for a domain. */
> +int vpci_bar_add_handlers(const struct domain *d,
> +                          const struct pci_dev *pdev);
> +int vpci_bar_remove_handlers(const struct domain *d,
> +                             const struct pci_dev *pdev);
> +#endif

This would then go away if we implement a mechanism similar to
REGISTER_VPCI_INIT.

Thanks, Roger.

>> +    if ( rc )
>> +        gdprintk(XENLOG_ERR,
>> +                 "%pp: failed to add BAR handlers for dom%pd: %d\n",
>> +                 &pdev->sbdf, d, rc);
>> +    return rc;
>> +}
>> +
>> +int vpci_bar_remove_handlers(const struct domain *d, const struct pci_dev *pdev)
>> +{
>> +    /* Remove previously added registers. */
>> +    vpci_remove_device_registers(pdev);
>> +    return 0;
>> +}
>> +#endif
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 0fe86cb30d23..702f7b5d5dda 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -95,7 +95,7 @@ int vpci_assign_device(struct domain *d, const struct pci_dev *dev)
>>       if ( is_system_domain(d) || !has_vpci(d) )
>>           return 0;
>>   
>> -    return 0;
>> +    return vpci_bar_add_handlers(d, dev);
>>   }
>>   
>>   /* Notify vPCI that device is de-assigned from guest. */
>> @@ -105,7 +105,7 @@ int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
>>       if ( is_system_domain(d) || !has_vpci(d) )
>>           return 0;
>>   
>> -    return 0;
>> +    return vpci_bar_remove_handlers(d, dev);
> I think it would be better to use something similar to
> REGISTER_VPCI_INIT here, otherwise this will need to be modified every
> time a new capability is handled by Xen.
>
> Maybe we could reuse or expand REGISTER_VPCI_INIT adding another field
> to be used for guest initialization?
>
>>   }
>>   #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>>   
>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>> index ecc08f2c0f65..fd822c903af5 100644
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -57,6 +57,14 @@ uint32_t vpci_hw_read32(const struct pci_dev *pdev, unsigned int reg,
>>    */
>>   bool __must_check vpci_process_pending(struct vcpu *v);
>>   
>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +/* Add/remove BAR handlers for a domain. */
>> +int vpci_bar_add_handlers(const struct domain *d,
>> +                          const struct pci_dev *pdev);
>> +int vpci_bar_remove_handlers(const struct domain *d,
>> +                             const struct pci_dev *pdev);
>> +#endif
> This would then go away if we implement a mechanism similar to
> REGISTER_VPCI_INIT.
>
> Thanks, Roger.
Ok, so I can extend REGISTER_VPCI_INIT with an action parameter:

"There are number of actions to be taken while first initializing vPCI
for a PCI device or when the device is assigned to a guest or when it
is de-assigned and so on.
Every time a new action is needed during these steps we need to call some
relevant function to handle that. Make it is easier to track the required
steps by extending REGISTER_VPCI_INIT machinery with an action parameter
which shows which exactly step/action is being performed."

So, we have

-typedef int vpci_register_init_t(struct pci_dev *dev);
+enum VPCI_INIT_ACTION {
+  VPCI_INIT_ADD,
+  VPCI_INIT_ASSIGN,
+  VPCI_INIT_DEASSIGN,
+};
+
+typedef int vpci_register_init_t(struct pci_dev *dev,
+                                 enum VPCI_INIT_ACTION action);

and, for example,

@@ -452,6 +452,9 @@ static int init_bars(struct pci_dev *pdev)
      struct vpci_bar *bars = header->bars;
      int rc;

+    if ( action != VPCI_INIT_ADD )
+        return 0;
+

I was thinking about adding dedicated machinery similar to REGISTER_VPCI_INIT,
e.g. REGISTER_VPCI_{ASSIGN|DEASSIGN} + dedicated sections in the linker scripts,
but it seems not worth it: these steps are only executed at device init/assign/deassign,
so extending the existing approach doesn't seem to hurt performance much.

Please let me know if this is what you mean, so I can re-work the relevant code.

Thank you,
Oleksandr

On Mon, Nov 01, 2021 at 09:18:17AM +0000, Oleksandr Andrushchenko wrote:
> 
> >> +    if ( rc )
> >> +        gdprintk(XENLOG_ERR,
> >> +                 "%pp: failed to add BAR handlers for dom%pd: %d\n",
> >> +                 &pdev->sbdf, d, rc);
> >> +    return rc;
> >> +}
> >> +
> >> +int vpci_bar_remove_handlers(const struct domain *d, const struct pci_dev *pdev)
> >> +{
> >> +    /* Remove previously added registers. */
> >> +    vpci_remove_device_registers(pdev);
> >> +    return 0;
> >> +}
> >> +#endif
> >> +
> >>   /*
> >>    * Local variables:
> >>    * mode: C
> >> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> >> index 0fe86cb30d23..702f7b5d5dda 100644
> >> --- a/xen/drivers/vpci/vpci.c
> >> +++ b/xen/drivers/vpci/vpci.c
> >> @@ -95,7 +95,7 @@ int vpci_assign_device(struct domain *d, const struct pci_dev *dev)
> >>       if ( is_system_domain(d) || !has_vpci(d) )
> >>           return 0;
> >>   
> >> -    return 0;
> >> +    return vpci_bar_add_handlers(d, dev);
> >>   }
> >>   
> >>   /* Notify vPCI that device is de-assigned from guest. */
> >> @@ -105,7 +105,7 @@ int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
> >>       if ( is_system_domain(d) || !has_vpci(d) )
> >>           return 0;
> >>   
> >> -    return 0;
> >> +    return vpci_bar_remove_handlers(d, dev);
> > I think it would be better to use something similar to
> > REGISTER_VPCI_INIT here, otherwise this will need to be modified every
> > time a new capability is handled by Xen.
> >
> > Maybe we could reuse or expand REGISTER_VPCI_INIT adding another field
> > to be used for guest initialization?
> >
> >>   }
> >>   #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
> >>   
> >> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> >> index ecc08f2c0f65..fd822c903af5 100644
> >> --- a/xen/include/xen/vpci.h
> >> +++ b/xen/include/xen/vpci.h
> >> @@ -57,6 +57,14 @@ uint32_t vpci_hw_read32(const struct pci_dev *pdev, unsigned int reg,
> >>    */
> >>   bool __must_check vpci_process_pending(struct vcpu *v);
> >>   
> >> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> >> +/* Add/remove BAR handlers for a domain. */
> >> +int vpci_bar_add_handlers(const struct domain *d,
> >> +                          const struct pci_dev *pdev);
> >> +int vpci_bar_remove_handlers(const struct domain *d,
> >> +                             const struct pci_dev *pdev);
> >> +#endif
> > This would then go away if we implement a mechanism similar to
> > REGISTER_VPCI_INIT.
> >
> > Thanks, Roger.
> Ok, so I can extend REGISTER_VPCI_INIT with an action parameter:
> 
> "There are number of actions to be taken while first initializing vPCI
> for a PCI device or when the device is assigned to a guest or when it
> is de-assigned and so on.
> Every time a new action is needed during these steps we need to call some
> relevant function to handle that. Make it is easier to track the required
> steps by extending REGISTER_VPCI_INIT machinery with an action parameter
> which shows which exactly step/action is being performed."
> 
> So, we have
> 
> -typedef int vpci_register_init_t(struct pci_dev *dev);
> +enum VPCI_INIT_ACTION {
> +  VPCI_INIT_ADD,
> +  VPCI_INIT_ASSIGN,
> +  VPCI_INIT_DEASSIGN,
> +};
> +
> +typedef int vpci_register_init_t(struct pci_dev *dev,
> +                                 enum VPCI_INIT_ACTION action);
> 
> and, for example,
> 
> @@ -452,6 +452,9 @@ static int init_bars(struct pci_dev *pdev)
>       struct vpci_bar *bars = header->bars;
>       int rc;
> 
> +    if ( action != VPCI_INIT_ADD )
> +        return 0;
> +
> 
> I was thinking about adding dedicated machinery similar to REGISTER_VPCI_INIT,
> e.g. REGISTER_VPCI_{ASSIGN|DEASSIGN} + dedicated sections in the linker scripts,
> but it seems not worth it: these steps are only executed at device init/assign/deassign,
> so extending the existing approach doesn't seem to hurt performance much.
> 
> Please let me know if this is what you mean, so I can re-work the relevant code.

I'm afraid I'm still unsure whether we need an explicit helper to
execute when assigning a device, rather than just using the current
init helpers (init_bars &c).

You said that sizing the BARs when assigning to a domU was not
possible [0], but I'm missing an explanation of why it's not possible,
as I think that won't be an issue on x86 [1].

Thanks, Roger.

[0] https://lore.kernel.org/xen-devel/368bf4b5-f9fd-76a6-294e-dbb93a18e73f@epam.com/
[1] https://lore.kernel.org/xen-devel/YXlxmdYdwptakDDK@Air-de-Roger/

On 02.11.21 12:03, Roger Pau Monné wrote:
> On Mon, Nov 01, 2021 at 09:18:17AM +0000, Oleksandr Andrushchenko wrote:
>>>> +    if ( rc )
>>>> +        gdprintk(XENLOG_ERR,
>>>> +                 "%pp: failed to add BAR handlers for dom%pd: %d\n",
>>>> +                 &pdev->sbdf, d, rc);
>>>> +    return rc;
>>>> +}
>>>> +
>>>> +int vpci_bar_remove_handlers(const struct domain *d, const struct pci_dev *pdev)
>>>> +{
>>>> +    /* Remove previously added registers. */
>>>> +    vpci_remove_device_registers(pdev);
>>>> +    return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>>    /*
>>>>     * Local variables:
>>>>     * mode: C
>>>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>>>> index 0fe86cb30d23..702f7b5d5dda 100644
>>>> --- a/xen/drivers/vpci/vpci.c
>>>> +++ b/xen/drivers/vpci/vpci.c
>>>> @@ -95,7 +95,7 @@ int vpci_assign_device(struct domain *d, const struct pci_dev *dev)
>>>>        if ( is_system_domain(d) || !has_vpci(d) )
>>>>            return 0;
>>>>    
>>>> -    return 0;
>>>> +    return vpci_bar_add_handlers(d, dev);
>>>>    }
>>>>    
>>>>    /* Notify vPCI that device is de-assigned from guest. */
>>>> @@ -105,7 +105,7 @@ int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
>>>>        if ( is_system_domain(d) || !has_vpci(d) )
>>>>            return 0;
>>>>    
>>>> -    return 0;
>>>> +    return vpci_bar_remove_handlers(d, dev);
>>> I think it would be better to use something similar to
>>> REGISTER_VPCI_INIT here, otherwise this will need to be modified every
>>> time a new capability is handled by Xen.
>>>
>>> Maybe we could reuse or expand REGISTER_VPCI_INIT adding another field
>>> to be used for guest initialization?
>>>
>>>>    }
>>>>    #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>>>>    
>>>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>>>> index ecc08f2c0f65..fd822c903af5 100644
>>>> --- a/xen/include/xen/vpci.h
>>>> +++ b/xen/include/xen/vpci.h
>>>> @@ -57,6 +57,14 @@ uint32_t vpci_hw_read32(const struct pci_dev *pdev, unsigned int reg,
>>>>     */
>>>>    bool __must_check vpci_process_pending(struct vcpu *v);
>>>>    
>>>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>>>> +/* Add/remove BAR handlers for a domain. */
>>>> +int vpci_bar_add_handlers(const struct domain *d,
>>>> +                          const struct pci_dev *pdev);
>>>> +int vpci_bar_remove_handlers(const struct domain *d,
>>>> +                             const struct pci_dev *pdev);
>>>> +#endif
>>> This would then go away if we implement a mechanism similar to
>>> REGISTER_VPCI_INIT.
>>>
>>> Thanks, Roger.
>> Ok, so I can extend REGISTER_VPCI_INIT with an action parameter:
>>
>> "There are number of actions to be taken while first initializing vPCI
>> for a PCI device or when the device is assigned to a guest or when it
>> is de-assigned and so on.
>> Every time a new action is needed during these steps we need to call some
>> relevant function to handle that. Make it is easier to track the required
>> steps by extending REGISTER_VPCI_INIT machinery with an action parameter
>> which shows which exactly step/action is being performed."
>>
>> So, we have
>>
>> -typedef int vpci_register_init_t(struct pci_dev *dev);
>> +enum VPCI_INIT_ACTION {
>> +  VPCI_INIT_ADD,
>> +  VPCI_INIT_ASSIGN,
>> +  VPCI_INIT_DEASSIGN,
>> +};
>> +
>> +typedef int vpci_register_init_t(struct pci_dev *dev,
>> +                                 enum VPCI_INIT_ACTION action);
>>
>> and, for example,
>>
>> @@ -452,6 +452,9 @@ static int init_bars(struct pci_dev *pdev)
>>        struct vpci_bar *bars = header->bars;
>>        int rc;
>>
>> +    if ( action != VPCI_INIT_ADD )
>> +        return 0;
>> +
>>
>> I was thinking about adding dedicated machinery similar to REGISTER_VPCI_INIT,
>> e.g. REGISTER_VPCI_{ASSIGN|DEASSIGN} + dedicated sections in the linker scripts,
>> but it seems not worth it: these steps are only executed at device init/assign/deassign,
>> so extending the existing approach doesn't seem to hurt performance much.
>>
>> Please let me know if this is what you mean, so I can re-work the relevant code.
> I'm afraid I'm still unsure whether we need an explicit helper to
> execute when assigning a device, rather than just using the current
> init helpers (init_bars &c).
>
> You said that sizing the BARs when assigning to a domU was not
> possible [0], but I'm missing an explanation of why it's not possible,
> as I think that won't be an issue on x86 [1].
I am in the process of re-working this and the relevant patches.
At the moment I have those helpers, but it seems I can remove them.
Once I finish the series I (most probably) will remove those.
>
> Thanks, Roger.
>
> [0] https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/368bf4b5-f9fd-76a6-294e-dbb93a18e73f@epam.com/__;!!GF_29dbcQIUBPA!mGz2uzJKNZsMr3R8awokkSOjo8ETjOS9N-JVkTIOJW5BYxvKgtZrKamPJq59I5u2GCDnsY4dQQ$ [lore[.]kernel[.]org]
> [1] https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/YXlxmdYdwptakDDK@Air-de-Roger/__;!!GF_29dbcQIUBPA!mGz2uzJKNZsMr3R8awokkSOjo8ETjOS9N-JVkTIOJW5BYxvKgtZrKamPJq59I5u2GCAHHkrD1g$ [lore[.]kernel[.]org]