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

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

On Wed, 2020-04-15 at 09:13 +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 19:15, Harsha Shamsundara Havanur wrote:
> > @@ -120,6 +121,11 @@ void pci_setup(void)
> >       */
> >      bool allow_memory_relocate = 1;
> > 
> > +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_MEMOR
> > Y
> > +            != PCI_COMMAND_MEMORY);
> > +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_IO
> > +            != PCI_COMMAND_IO);
> 
> This still isn't in line with our default style, you will want eg:
> 
>     BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_MEMORY
> !=
>                  PCI_COMMAND_MEMORY);
>     BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_IO !=
>                  PCI_COMMAND_IO);
> 
> > @@ -208,6 +214,20 @@ void pci_setup(void)
> >              break;
> >          }
> > 
> > +        /*
> > +         * Disable memory and I/O decode,
> > +         * 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.
> > +         */
> 
> Please can you bring this comment into shape? The comma on the first
> line doesn't fit with the capital letter the second line starts with.
> Plus, if you really mean what is now on the second line to start on a
> new one, then please also insert a line consisting of just * at the
> properly indented position between the two. Additionally there's at
> least one line exceeding the 80-chars-per-line limit.
> 
> > @@ -289,10 +309,6 @@ void pci_setup(void)
> >                     devfn>>3, devfn&7, 'A'+pin-1, isa_irq);
> >          }
> > 
> > -        /* Enable bus mastering. */
> > -        cmd = pci_readw(devfn, PCI_COMMAND);
> > -        cmd |= PCI_COMMAND_MASTER;
> > -        pci_writew(devfn, PCI_COMMAND, cmd);
> 
> The movement of this wants mentioning in the description.
> 
> > @@ -526,10 +538,17 @@ 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 bus master, memory and I/O decode. */
> > +    for ( devfn = 0; devfn < 256; devfn++ )
> > +        if ( pci_devfn_decode_type[devfn] )
> > +        {
> > +            cmd = pci_readw(devfn, PCI_COMMAND);
> > +            cmd |= (PCI_COMMAND_MASTER |
> > pci_devfn_decode_type[devfn]);
> > +            pci_writew(devfn, PCI_COMMAND, cmd);
> > +        }
> 
> This still regresses the setting of MASTER afaict: You only set
> that bit now if either IO or MEMORY would also get set. But be
> sure to honor the original code not doing the write when vendor/
> device IDs are all ones.
> 
If condition ensures that for devices with vendor/device IDs all ones
are skipped as it would evaluate to false. But this would also skip
enabling Bus master for devices whose vendor/device IDs are not all
ones but IO or memory BARs are not programmed for them. Is there a
possibility of this happening?
> Jan