Re: [XEN PATCH v2] hvmloader: Enable MMIO and I/O decode, after all resource allocation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2020 11:00, Shamsundara Havanur, Harsha wrote:
> On Tue, 2020-04-14 at 09:42 +0200, Jan Beulich wrote:
>> On 13.04.2020 23:33, Harsha Shamsundara Havanur wrote:
>>> @@ -289,9 +293,22 @@ void pci_setup(void)
>>>                     devfn>>3, devfn&7, 'A'+pin-1, isa_irq);
>>>          }
>>>
>>> -        /* Enable bus mastering. */
>>> +        /*
>>> +         * Disable bus mastering, memory and I/O space, which is
>>> typical device
>>> +         * reset state. It is recommended that BAR programming be
>>> done whilst
>>> +         * decode bits are cleared to avoid incorrect mappings
>>> being created,
>>> +         * when 64-bit memory BAR is programmed first by writing
>>> the lower half
>>> +         * and then the upper half, which first maps to an address
>>> under 4G
>>> +         * replacing any RAM mapped in that address, which is not
>>> restored
>>> +         * back after the upper half is written and PCI memory is
>>> correctly
>>> +         * mapped to its intended high mem address.
>>> +         *
>>> +         * Capture the state of bus master to restore it back once
>>> BAR
>>> +         * programming is completed.
>>> +         */
>>>          cmd = pci_readw(devfn, PCI_COMMAND);
>>> -        cmd |= PCI_COMMAND_MASTER;
>>> +        pci_devfn_decode_type[devfn] = cmd & ~(PCI_COMMAND_MEMORY
>>> | PCI_COMMAND_IO);
>>> +        cmd &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY |
>>> PCI_COMMAND_IO);
>>
>> The disabling of MASTER was put under question in v1 already.
> 
> Disabling of MASTER is done whilst programming BARs and it is restored
> back to its previous value in the loop at the end of pci_setup
> function.

Yet didn't Andrew indicate he knows of devices which get upset if
MASTER _ever_ gets cleared?

>>> @@ -526,10 +542,13 @@ void pci_setup(void)
>>>           * has IO enabled, even if there is no I/O BAR on that
>>>           * particular device.
>>>           */
>>> -        cmd = pci_readw(vga_devfn, PCI_COMMAND);
>>> -        cmd |= PCI_COMMAND_IO;
>>> -        pci_writew(vga_devfn, PCI_COMMAND, cmd);
>>> +        pci_devfn_decode_type[vga_devfn] |= PCI_COMMAND_IO;
>>>      }
>>> +
>>> +    /* Enable memory and I/O space. Restore saved BUS MASTER state
>>> */
>>> +    for ( devfn = 0; devfn < 256; devfn++ )
>>> +        if ( pci_devfn_decode_type[devfn] )
>>> +            pci_writew(devfn, PCI_COMMAND,
>>> pci_devfn_decode_type[devfn]);
>>
>> You effectively clear the upper 8 bits here, rather than retaining
>> them.
>>
> In fact cmd is a 32 bit value and I am retaining only lower 8 bits of
> it. This will be corrected in v3.

PCI_COMMAND is a 16-bit field. The adjacent 16 bits in the same 32 bit
word are PCI_STATUS.

Jan