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

["Shamsundara Havanur, Harsha" <havanur@xxxxxxxxxx>]

On Tue, 2020-04-14 at 11:14 +0200, Jan Beulich wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> 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?

Previous commit enabled MASTER for all functions. I am bit confused
here on the consensus on enabling/disabling/retaining BME.
Should we even care about MASTER?

> 
> > > > @@ -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