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

[Alexey G <x1917x@xxxxxxxxx>]

On Tue, 20 Mar 2018 09:20:01 +0000
Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:

>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 :)).

OK, will do.

>> >> +        {
>> >> +            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.

It will depend on future QEMU/hvmloader changes a bit, but I think
switching to the comment about the default values instead of
initialization should be good.

>
>Thanks, Roger.

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