From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

Reset the command register when passing through a PCI device:
it is possible that when passing through a PCI device its memory
decoding bits in the command register are already set. Thus, a
guest OS may not write to the command register to update memory
decoding, so guest mappings (guest's view of the BARs) are
left not updated.

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
---
Since v1:
 - do not write 0 to the command register, but respect host settings.
---
 xen/drivers/vpci/header.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 754aeb5a584f..70d911b147e1 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -451,8 +451,7 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg,
         pci_conf_write16(pdev->sbdf, reg, cmd);
 }
 
-static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
-                            uint32_t cmd, void *data)
+static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd)
 {
     /* TODO: Add proper emulation for all bits of the command register. */
 
@@ -467,14 +466,20 @@ static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
             cmd |= PCI_COMMAND_INTX_DISABLE;
         else
         {
-            uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
+            uint16_t current_cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
 
             if ( current_cmd & PCI_COMMAND_INTX_DISABLE )
                 cmd |= PCI_COMMAND_INTX_DISABLE;
         }
     }
 
-    cmd_write(pdev, reg, cmd, data);
+    return cmd;
+}
+
+static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
+                            uint32_t cmd, void *data)
+{
+    cmd_write(pdev, reg, emulate_cmd_reg(pdev, cmd), data);
 }
 
 static void bar_write(const struct pci_dev *pdev, unsigned int reg,
@@ -793,6 +798,10 @@ int vpci_bar_add_handlers(const struct domain *d, const struct pci_dev *pdev)
         gdprintk(XENLOG_ERR,
                  "%pp: failed to add BAR handlers for dom%pd: %d\n",
                  &pdev->sbdf, d, rc);
+
+    /* Reset the command register with respect to host settings. */
+    pci_conf_write16(pdev->sbdf, PCI_COMMAND, emulate_cmd_reg(pdev, 0));
+
     return rc;
 }
 
-- 
2.25.1

On Thu, Sep 30, 2021 at 10:52:21AM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> Reset the command register when passing through a PCI device:
> it is possible that when passing through a PCI device its memory
> decoding bits in the command register are already set. Thus, a
> guest OS may not write to the command register to update memory
> decoding, so guest mappings (guest's view of the BARs) are
> left not updated.
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Michal Orzel <michal.orzel@arm.com>
> ---
> Since v1:
>  - do not write 0 to the command register, but respect host settings.
> ---
>  xen/drivers/vpci/header.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> index 754aeb5a584f..70d911b147e1 100644
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -451,8 +451,7 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg,
>          pci_conf_write16(pdev->sbdf, reg, cmd);
>  }
>  
> -static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
> -                            uint32_t cmd, void *data)
> +static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd)
>  {
>      /* TODO: Add proper emulation for all bits of the command register. */
>  
> @@ -467,14 +466,20 @@ static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
>              cmd |= PCI_COMMAND_INTX_DISABLE;
>          else
>          {
> -            uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
> +            uint16_t current_cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);

Either we keep reg here or we drop the parameter altogether from the
function prototype. Having one caller pass 0 while the other passing
PCI_COMMAND is confusing. The more that the parameter is now
effectively unused.

>  
>              if ( current_cmd & PCI_COMMAND_INTX_DISABLE )
>                  cmd |= PCI_COMMAND_INTX_DISABLE;
>          }
>      }
>  
> -    cmd_write(pdev, reg, cmd, data);
> +    return cmd;
> +}
> +
> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
> +                            uint32_t cmd, void *data)
> +{
> +    cmd_write(pdev, reg, emulate_cmd_reg(pdev, cmd), data);
>  }
>  
>  static void bar_write(const struct pci_dev *pdev, unsigned int reg,
> @@ -793,6 +798,10 @@ int vpci_bar_add_handlers(const struct domain *d, const struct pci_dev *pdev)
>          gdprintk(XENLOG_ERR,
>                   "%pp: failed to add BAR handlers for dom%pd: %d\n",
>                   &pdev->sbdf, d, rc);
> +
> +    /* Reset the command register with respect to host settings. */
> +    pci_conf_write16(pdev->sbdf, PCI_COMMAND, emulate_cmd_reg(pdev, 0));

I think we likely want to unset the memory and IO decoding bits from
the command register, as the guest view of the BAR address is
currently forced to 0, and not mapped into the guest p2m.

Thanks, Roger.

Hi, Roger!

On 26.10.21 14:00, Roger Pau Monné wrote:
> On Thu, Sep 30, 2021 at 10:52:21AM +0300, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>
>> Reset the command register when passing through a PCI device:
>> it is possible that when passing through a PCI device its memory
>> decoding bits in the command register are already set. Thus, a
>> guest OS may not write to the command register to update memory
>> decoding, so guest mappings (guest's view of the BARs) are
>> left not updated.
>>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> Reviewed-by: Michal Orzel <michal.orzel@arm.com>
>> ---
>> Since v1:
>>   - do not write 0 to the command register, but respect host settings.
>> ---
>>   xen/drivers/vpci/header.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index 754aeb5a584f..70d911b147e1 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -451,8 +451,7 @@ static void cmd_write(const struct pci_dev *pdev, unsigned int reg,
>>           pci_conf_write16(pdev->sbdf, reg, cmd);
>>   }
>>   
>> -static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
>> -                            uint32_t cmd, void *data)
>> +static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd)
>>   {
>>       /* TODO: Add proper emulation for all bits of the command register. */
>>   
>> @@ -467,14 +466,20 @@ static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
>>               cmd |= PCI_COMMAND_INTX_DISABLE;
>>           else
>>           {
>> -            uint16_t current_cmd = pci_conf_read16(pdev->sbdf, reg);
>> +            uint16_t current_cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> Either we keep reg here or we drop the parameter altogether from the
> function prototype. Having one caller pass 0 while the other passing
> PCI_COMMAND is confusing. The more that the parameter is now
> effectively unused.
This is probably because git diff isn't really helpful here in showing the change:
static uint32_t emulate_cmd_reg(const struct pci_dev *pdev, uint32_t cmd)
{
     /* TODO: Add proper emulation for all bits of the command register. */

     if ( (cmd & PCI_COMMAND_INTX_DISABLE) == 0 )
     {
         /*
          * Guest wants to enable INTx. It can't be enabled if:
          *  - host has INTx disabled
          *  - MSI/MSI-X enabled
          */
         if ( pdev->vpci->msi->enabled )
             cmd |= PCI_COMMAND_INTX_DISABLE;
         else
         {
             uint16_t current_cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);

             if ( current_cmd & PCI_COMMAND_INTX_DISABLE )
                 cmd |= PCI_COMMAND_INTX_DISABLE;
         }
     }

     return cmd;
}

So, reg is not used here and cmd is the desired value of the PCI_COMMAND
register. So, I see no confusion here.
>>   
>>               if ( current_cmd & PCI_COMMAND_INTX_DISABLE )
>>                   cmd |= PCI_COMMAND_INTX_DISABLE;
>>           }
>>       }
>>   
>> -    cmd_write(pdev, reg, cmd, data);
>> +    return cmd;
>> +}
>> +
>> +static void guest_cmd_write(const struct pci_dev *pdev, unsigned int reg,
>> +                            uint32_t cmd, void *data)
>> +{
>> +    cmd_write(pdev, reg, emulate_cmd_reg(pdev, cmd), data);
>>   }
>>   
>>   static void bar_write(const struct pci_dev *pdev, unsigned int reg,
>> @@ -793,6 +798,10 @@ int vpci_bar_add_handlers(const struct domain *d, const struct pci_dev *pdev)
>>           gdprintk(XENLOG_ERR,
>>                    "%pp: failed to add BAR handlers for dom%pd: %d\n",
>>                    &pdev->sbdf, d, rc);
>> +
>> +    /* Reset the command register with respect to host settings. */
>> +    pci_conf_write16(pdev->sbdf, PCI_COMMAND, emulate_cmd_reg(pdev, 0));
> I think we likely want to unset the memory and IO decoding bits from
> the command register, as the guest view of the BAR address is
> currently forced to 0, and not mapped into the guest p2m.
By passing 0 here as the desired value of the PCI_COMMAND register
we do that. The emulation code will take care of that.
>
> Thanks, Roger.
Thank you,
Oleksandr