Re: [Xen-devel] [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.04.14 at 15:17, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/04/14 14:05, Jan Beulich wrote:
>>>>> On 07.04.14 at 14:47, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 03/04/14 10:41, Jan Beulich wrote:
>>>> +        if ( val & PCI_STATUS_CHECK )
>>>> +        {
>>>> +            printk(XENLOG_INFO "%04x:%02x:%02x.%u status %04x\n",
>>>> +                   seg, bus, dev, func, val);
>>> What is the purpose of this printk?  From the text alone it is not obvious.
>> It's simply to have an indication that the status register was written
>> (and that certain bits may have got cleared).
> 
> Then at the very least it should be ..."status %04x -> %04x\n", ....
> val, val & PCI_STATUS_CHECK) to identify which status bits are being
> cleared.

This obviously makes sense only when also changing the write operation
to use val & PCI_STATUS_CHECK, and then it would seem to make more
sense to print val & ~PCI_STATUS_CHECK as the second value.

>>>> +            pci_conf_write16(seg, bus, dev, func, PCI_STATUS, val);
>>> I dont think this code has any right to clear status bits other than the
>>> ones it is checking for, so the write should be "val & PCI_STATUS_CHECK"
>> Hmm, the intention is to clear all status fields that can be cleared, and
>> the if() around the write is just to avoid the printk() and the write if
>> possible. PCI_STATUS_CHECK already includes all changeable bits, and
>> I'd expect any of the few that are currently reserved to get added
>> here, should they attain a meaning of a writable one.
> 
> For forward compatibility, we must not assume anything about currently
> reserved bits which Xen doesn't know about.

In general this is correct; I'm not so certain in the particular case here.
For one, the code doesn't get used by default, but only when asked for
on the command line. If the code was found to have a problem, just
don't specify the command line option. And then, as said before, the
intention is to get the status register into a clean state. It's a status
register, so we're not going to corrupt device state by clearing a few
too many bits, and we log which bits were set at the time we did the
clearing.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel