Re: [Xen-devel] [RFC PATCH 06/12] hvmloader: add basic Q35 support

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Mar 20, 2018 at 09:44:33AM +1000, Alexey G wrote:
> On Mon, 19 Mar 2018 15:30:14 +0000
> Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> 
> >On Tue, Mar 13, 2018 at 04:33:51AM +1000, Alexey Gerasimenko wrote:
> >> +    {
> >> +    case 0x0300:  
> >
> >All this values need to be defines documented somewhere.
> 
> Agree... although it was not me who introduced all these hardcoded PCI
> class values. :) I'll change these numbers into newly added pci_regs.h
> #defines in the non-functional patch.

Right. I've realized that later. If you place this code moment in a
separate patch without any other modifications I won't complain about
the lack of defines (although it would be nice to have them :)).

> >> +        {
> >> +            pci_writeb(PCI_ICH9_LPC_DEVFN, 0x60 + link, isa_irq);
> >> +
> >> +            /* PIRQE..PIRQH are unused */
> >> +            pci_writeb(PCI_ICH9_LPC_DEVFN, 0x68 + link, 0x80);  
> >
> >According to the spec 0x80 is the default value for this registers, do
> >you really need to write it?
> >
> >Is maybe QEMU not correctly setting the default value?
> 
> Won't agree here. We're initializing PIRQ[n] routing in this
> fragment, it's better not to rely on any values but simply initialize
> all PIRQ[n]_ROUT registers, this makes it explicit.
> 
> Even if it is unnecessary due to defaults it's more obvious to set
> these registers to our own values than to force a reader to either look
> up their emulation in QEMU code or read the ICH9 pdf to confirm
> assumptions.

But if you start doing this, you should do it for all the registers.
Why is PIRQE..PIRQH routing special that you need to re-write the
default value? But not SIRQ_CNTL for example?

I think a comment noting that the default value for those registers is
what we expect (0x80 - Interrupt Routing Disabled) would be better.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel