Re: [PATCH v3 09/11] vpci/header: Reset the command register when adding devices

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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@xxxxxxxx>
>>
>> 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@xxxxxxxx>
>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>> ---
>> 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