Re: [PATCH for-4.17 6/6] vpci: refuse BAR writes only if the BAR is mapped

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.10.2022 17:04, Roger Pau Monné wrote:
> On Mon, Oct 24, 2022 at 01:51:03PM +0200, Jan Beulich wrote:
>> On 20.10.2022 11:46, Roger Pau Monne wrote:
>>> Writes to the BARs are ignored if memory decoding is enabled for the
>>> device, and the same happen with ROM BARs if the write is an attempt
>>> to change the position of the BAR without disabling it first.
>>>
>>> The reason of ignoring such writes is a limitation in Xen, as it would
>>> need to unmap the BAR, change the address, and remap the BAR at the
>>> new position, which the current logic doesn't support.
>>>
>>> Some devices however seem to have the memory decoding bit hardcoded to
>>> enabled, and attempts to disable it don't get reflected on the
>>> command register.
>>
>> This isn't compliant with the spec, is it? It looks to contradict both
>> "When a 0 is written to this register, the device is logically
>> disconnected from the PCI bus for all accesses except configuration
>> accesses" and "Devices typically power up with all 0's in this
>> register, but Section 6.6 explains some exceptions" (quoting from the
>> old 3.0 spec, which I have readily to hand). The referenced section
>> then says "Such devices are required to support the Command register
>> disabling function described in Section 6.2.2".
> 
> It's unclear to me whether setting the bit to 0 is plain ignored, or
> just fails to be reflected on the command register.
> 
>> How does any arbitrary OS go about sizing the BARs of such a device?
> 
> AFAICT from Linux behavior, an OS would just set to 0 the memory
> decoding command register bit and write the value, but there's no
> check afterwards that the returned value from a read of the register
> still has memory decoding disabled.   Xen does exactly the same:
> attempt to toggle the bit but don't check the value written.

Sure - both assume that the bit is writable in the first place. Yet
altering the BARs when the write had no effect is not really correct.

>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -388,7 +388,7 @@ static void cf_check bar_write(
>>>      else
>>>          val &= PCI_BASE_ADDRESS_MEM_MASK;
>>>  
>>> -    if ( pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY )
>>> +    if ( bar->enabled )
>>>      {
>>>          /* If the value written is the current one avoid printing a 
>>> warning. */
>>>          if ( val != (uint32_t)(bar->addr >> (hi ? 32 : 0)) )
>>> @@ -425,7 +425,7 @@ static void cf_check rom_write(
>>>      uint16_t cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
>>>      bool new_enabled = val & PCI_ROM_ADDRESS_ENABLE;
>>>  
>>> -    if ( (cmd & PCI_COMMAND_MEMORY) && header->rom_enabled && new_enabled )
>>> +    if ( rom->enabled && new_enabled )
>>>      {
>>>          gprintk(XENLOG_WARNING,
>>>                  "%pp: ignored ROM BAR write with memory decoding 
>>> enabled\n",
>>
>> The log message wording then wants adjustment, I guess?
> 
> Hm, I think the message is fine for the purposes here, vPCI is indeed
> ignoring a write with memory decoding enabled, or else rom->enabled
> would be false.
> 
> Or are you arguing that the message is already wrong in the current
> context and should instead be:
> 
> "ignored ROM BAR write with memory decoding and ROM enabled"

No, my point rather was that we're no longer checking for memory decoding
to be disabled. Aiui we still require that the guest has cleared its view
of the bit, so I guess the messages could still be viewed as applicable.
Personally I'd prefer disambiguation.

Jan