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_andrushche...@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_andrushche...@epam.com>
>> Reviewed-by: Michal Orzel <michal.or...@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
